Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gcc devshell #84

Merged
merged 6 commits into from
Aug 17, 2023
Merged

Gcc devshell #84

merged 6 commits into from
Aug 17, 2023

Conversation

jacg
Copy link
Owner

@jacg jacg commented Aug 17, 2023

Adds a second devShell which uses gcc instead of clang. This means we can now do

  1. nix develop <path-to-flake>
  2. nix develop <path-to-flake>#gcc
  3. nix develop <path-to-flake>#clang

1&3 give a clang environment, 2 gives a gcc environment.

This could be useful for users who have a strong preference, but, above all, for allowing us to test both gcc and clang on GHA.

However, I'm unsure about how we should organize the workflows? Specifically, should the gcc and clang tests be run in

  1. the same job
  2. two separate jobs

?

  • Pro 1
    • trivial to set up
    • uses less CPU and network (doesn't need to boostrap the GHA environment twice)
  • Pro 2
    • reports gcc failures separately from clang failures
    • runs in parallel, so completes more quickly

Note: this sits on top of the as-yet-unmerged #82.

Closes #17.

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

FFS ... allow-fail has meaning in the GHA matrix. Why am I miserably failing to find any documentation on it?

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

... because it doesn't have meaning in GHA, I invented it as a convenient name (for use in continue-on-error) it would seem.

@jacg jacg force-pushed the gcc-devshell branch 3 times, most recently from 79a4db7 to 118e640 Compare August 17, 2023 13:22
@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

We now have this matrix:

    continue-on-error: ${{ matrix.devshell == 'gcc' }}
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-22.04, macos-12]
        py: [311]
        devshell: [ clang, gcc ]

so 4 jobs run (2 OSes x 2 compilers), and those on gcc are allowed to fail.

Curiously, the gcc job only fails on Linux, not Darwin. Is this because on macOS we're compiling with clang, even when in the gcc environment?

One slight wart is that we run nix-shell on both compilers' jobs, but nix-shell always uses the default (clang). It should be easy to switch it off on gcc.

@jacg jacg mentioned this pull request Aug 17, 2023
2 tasks
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just this one thing

client_side_tests/client_fetch_content/CMakeLists.txt Outdated Show resolved Hide resolved
@gonzaponte
Copy link
Collaborator

we're talking about at test that is spuriously passing and has just been merged in #82?

Yes.

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

I've lost track. Do we want to do anything more with this, or shall we merge?

@gonzaponte
Copy link
Collaborator

We want to skip the test, right?

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

I must be missing something: I don't see what this test has to do with this PR. What's more, it seems to be failing on master.

@gonzaponte
Copy link
Collaborator

I don't see what this test has to do with this PR

Ugh, nothing. I got lost in the many conversations. This can be merged.

it seems to be failing on master.

Ugh x2. Now I'm puzzled.

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

I got lost in the many conversations.

Yup, me too.

@gonzaponte
Copy link
Collaborator

gonzaponte commented Aug 17, 2023

it seems to be failing on master.

Ugh x2. Now I'm puzzled.

I'm trying to undo the set -e in my remote. If it works I will push it to master and we will deal with that on #88. But that shouldn't stop this from being merged.

@jacg
Copy link
Owner Author

jacg commented Aug 17, 2023

I've just push a disabling of that test. It's getting in the way too much, right now.

@gonzaponte
Copy link
Collaborator

I'm trying to undo the set -e in my remote. If it works I will push it to master and we will deal with that on #88. But that shouldn't stop this from being merged.

I've also pushed this.

@jacg jacg merged commit 99de86a into master Aug 17, 2023
4 of 5 checks passed
@jacg jacg deleted the gcc-devshell branch August 17, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider running GHA with gcc in addition to clang
2 participants