Add initial CI: Build steps for supported compilers #462
Conversation
|
Thanks! I confess I don't understand CI and GitHub actions well, but I'm open to trying this... Would someone else who does understand it well please review this PR and give some commentary? Also, in case it's useful, here are the flags I'm currently using for GCC, Clang, and MSVC in my local "make sure cppfront compiles clean at high warnings levels on the major compilers" tests:
|
hsutter
added
the
question - further information requested
|
In fact a couple of months back I proposed simple GitHub CI that included a bash script for running tests (see #150 and #151). I did not go for CMake as I understood the goal was to limit dependencies in the project. Still, at that point @hsutter said it was to early a stage to already include that. Depending on how far we are ready to go e.g. to be able to run tests on both Linux, Windows and Mac we could look into an analogue of a bash script written e.g. in PowerShell or go all the way to a CMake that would allow for configuring builds and tests on all three platforms. This, however, adds a dependency to CMake and to an actual build system, like Ninja. Another thing to consider about CI on GitHub are the limits. The free package includes 2000 CI/CD minutes/month. In addition only on Linux each minute counts as one towards the monthly limit. One minute of running time on Windows counts as 2. On mac it is even worse as every minute counts as 10. The CI is quite configurable, but care should be taken when planning all that. To give some perspective: on my WSL Ubuntu a single compilation of cppfront with g++10 takes ~12s and with clang++-12 ~7s. In the current proposition the CI action that compiles cppfront itself on multiple platforms with multiple compilers is run on every push to any branch. This might be problematic for contributors who like to push often to their work-in-progress branches as the builds of each such push in their own forks will count towards their own CI limit. As much as running CI on every push is cool, I would propose to start with CI runs only on pushes to the main branch to evaluate the usage. If the 2000 minutes run out too quickly e.g. the number of compilation options (compiler versions, C++ standards) could be limited. If not addition of test runs should be considered. Alternatively, the CI actions could be executed on pull requests. The main branch could be protected from direct pushes - all changes would have to go through pull requests. The pull requests would be tested before merging. Failures would prevent merging of breaking changes into main. In addition e.g. nightly builds of the main branch itself could be scheduled. Having said all that I agree a discussion is needed as to the nature/complication of CI for cppfront and a path to get there needs to be outlined. |
|
I agree that we should keep the build process as simple as possible, I don't think at this time we need something like CMake or Ninja, perhaps as the complexity of the experiment grows we might consilder alternatives, but I think just having to run a single command to get a working cppfront has its beauty to it (its what drove me to test it to begin with!). For usage limits, I'd say there's no need to worry about it as Actions usage is free for public repositories on standard Github-hosted runners or self-hosted runners. Even if the 2000 minutes limit was there I think Herb should still be good to go with current activity in the repo. Indeed, the idea is to start the discussion again about automating the mundane parts (such as checking that every compiler works and regression/passthrough tests pass) so that developers can focus on the important implementation bits! I just felt like doing this after seeing #460, #308, etc. |
|
My bad, I re-read the GitHub pricing page and in deep public repos have no limits. Given that this is the case I agree that the proposed workflow is a good idea. I saw you added the warning level flags suggested above. To simplify things you used the same flags for both clang and g++ which are a union of what was suggested for each compiler, but that looks reasonable. I have just one more question to @hsutter: Given that this PR will likely go in, would you be interested in having another GitHub workflow that runs tests? I agree with @DyXel that CMake seems overkill at this stage, but what would you say for a simple bash script for Linux that runs regression tests and performs basic checks? The script would come in handy for contributors like your own I could add another workflow that runs the tests that would be an evolution of #150. |
.github/workflows/ci.yml
Outdated
| @@ -0,0 +1,70 @@ | |||
| name: cppfront CI | |||
There was a problem hiding this comment.
[Suggestion] I think the general practice with GitHub workflows is to make both the YAML file name and the name displayed in the GitHub UI more descriptive. How about something like build-cppfront.yaml and:
| name: cppfront CI | |
| name: Multi-platform Build of cppfront |
or something like that?
.github/workflows/ci.yml
Outdated
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| runs-on: [ubuntu-20.04, ubuntu-22.04, macos-latest] |
There was a problem hiding this comment.
[Question] What is the added value of running older compilers on ubuntu-20.04 and the newer ones on ubuntu-22.04? Running all the versioned g++ and clang variants on ubuntu-22.04 and skipping ubuntu-20.04 altogether would shorten the list of exclusions by half, wouldn't it?
There was a problem hiding this comment.
Yeah, you're right. Simplified the flow by only using ubuntu-latest!
| # Unversioned clang++ doesn't run on ubuntu | ||
| - runs-on: ubuntu-latest | ||
| compiler: clang++ |
There was a problem hiding this comment.
Besides this one,
I think you can remove all other exclusions
by changing c++23 to c++2b,
removing macos-latest from runs-on,
and adding
include:
- runs-on: macos-latest
compiler: clang++There was a problem hiding this comment.
I did some testing by changing c++23 to c++2b and managed to simplify the matrix by using include as suggested. Summary:
gcc-10doesn't recognize eitherc++2borc++23clang++-12does recognizec++2bbut fails to compile, doesn't recognizec++23clang++-14same as above
The only exclusion now is gcc-10 on the c++2b configuration, also, I have attached the build logs of the failed builds since this doesn't seem intentional (why would it compile with c++20 but not c++2b?).
7_build-unix-like (ubuntu-latest, clang++-12, c++2b).txt
9_build-unix-like (ubuntu-latest, clang++-14, c++2b).txt
There was a problem hiding this comment.
I am not sure about using c++2b with compiler versions that do not support c++23 (as I would be with c++2a instead of c++20). There is a good reason for this lack of support - the standard is not fully implemented in the particular compiler version. A build failure might actually be the result of shortcomings of the compiler rather than issues in the code. It is true there might be bugs even with full support, but those are much less likely.
There was a problem hiding this comment.
I will leave up to Herb to decide if we support partial implementations, what I find weird here is that c++2b > c++20, yet c++20 compiles but c++2b does not. I would expect for cppfront to still compile if the user opts-in to a partial implementations that is newer than c++20...
There was a problem hiding this comment.
cppfront should compile with newer standards.
I'm still neglecting this particular failure to compile cppfront with Clang using C++23.
There was a problem hiding this comment.
Do we disable those 2 explicitly then for the time being? I can reference the issue in a comment in that case.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Opened #818 to workaround llvm/llvm-project#58206.
| - uses: actions/checkout@v3 | ||
| - name: Install compiler | ||
| if: startsWith(matrix.runs-on, 'ubuntu') | ||
| run: sudo apt-get install -y ${{ matrix.compiler }} |
There was a problem hiding this comment.
Should this be $CXX instead of ${{ matrix.compiler }} for consistency, or can that not be used there?
There was a problem hiding this comment.
I think it could be referenced directly yeah, but defining the CXX envvar is also super common (specially when using build systems that expect those vars to be set).
There was a problem hiding this comment.
I think it could be referenced directly yeah
I was actually saying the opposite:
run: sudo apt-get install -y $CXX
|
This could have been the first step to eventually help prevent #744. |
|
Looks like c1d3e0e also fixed some things that created unclean tests. CI would be great! |
|
Thanks for your patience on this one. Okay, this PR is just adding a single So I think my main question should be: If I merge this PR, what will be the visible result in this repo? Is there any reason not to merge this now?
OK, I'm ready to have a workflow that runs tests, ideally if it's as lightweight and noninvasive as this one which just adds a file. |
|
Hey hello, indeed it's been a while. To your questions:
Yes, in fact, you can just disable the Actions at any point via the Actions tab. IIRC, for PRs, I think you need to give explicit permissions to enable running first for non-contributors (if anybody knows how to double-check please feel free to share).
When when you commit or if anybody opens a PR (and if you give them permissions), the CI will attempt to build cppfront for the matrix of compilers specified in the yaml and it will show if said builds were successful for that commit or PR, as in, if they managed to produce the cppfront executable. The action will only succeed if all currently selected compilers built, which makes trivial to see if the code change introduced any compilation problem for any compiler.
Hmm, I would say that this could potentially be merged as-is if nobody has arguments against it, and then later we can add regression testing steps (I'd be happy to help in that case). |
Great! What I have so far in a slightly outdated branch is a single .yaml file with the workflow, similar to the one in this PR + a single bash script for running regression tests. I made this bash script portable, so that it can also run tests on Windows through Git Bash. As a result the very same script can be used to run regression tests on Mac, Linux and Windows, both locally and through GitHub actions. If that sounds interesting I can update the mentioned branch and open a PR. |
|
I'll take that as "yes this is ready to merge" and I'll do that. After merge, I evidently need to enable Actions... presumably the UI will be intuitive, otherwise I'll have questions :) |
|
Here's what I see -- any ideas? Maybe this is about the "let's disable MacOS for now" comment above?
|
|
If you navigate the erroring build (the one with the red X) ( https://github.com/hsutter/cppfront/actions/runs/7234428378/job/19710793888 ), you can see the problem: |
|
The usual I thought Apple Clang supported ranges, as of 14.0.3? Maybe that one isn't being built in C++20 mode, or this is using a pre-14.0.3 compiler? |
From the same job. Seems like the macOS runner has 14.0.0 as latest. I don't know how much we care about supporting that specific version. You could find a workaround, or if that is too tedious/not worth, you can disable that job run in the yaml by removing the inclusion in lines 29-32. |
|
😁 "Teach a man to fish..." OK, looks like Apple Clang 14.0.3 is only available since Xcode 14.3, released in March 2023. So the VM/devbox must not be updated yet, I guess. |
|
At first I disabled it, then I re-enabled it and just removed the use of That's the only use of And yes, this PR exposed that. 🎉 Thanks! |
|
By the way, my recommendation if you don't want to start doing "fix compilation on X compiler" commits (or force pushes 💀 ) for the rest of times (I know, been there!), I would suggest first comitting to your own branches and/or opening PRs and then merging to main after CI completes. That way you also follow the flow that everybody else does, and then later you can enforce it (for example, by marking the main branch as protected). |
|
Say, n00b question here: If I want to enable MacOS Apple Clang 12 testing, can I just change |
|
Assuming the runner has it installed, yes, you can follow the pattern used for Ubuntu. It does seem like the latest runner for macOS only has Clang 14.0.0 and Clang 15.0.0 though 😦 ( https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md ) |
This PR adds a initial Github Actions workflow to compile cppfront for several platforms, compilers and compiler versions.
Compilers tested: gcc 10, gcc 11, gcc 12, clang 12, clang 14, Apple's clang 14 and Microsoft's vs2022.
Benefits of adopting CI right now:
If interested we could move onto automating regression testing and passthrough testing next.