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

Use old GCC for CI #1058

Closed
tschwinge opened this issue Mar 23, 2022 · 11 comments · Fixed by #1059
Closed

Use old GCC for CI #1058

tschwinge opened this issue Mar 23, 2022 · 11 comments · Fixed by #1059

Comments

@tschwinge
Copy link
Member

We should think about having older versions of gcc compile us in CI or buildbot at some point!

Originally posted by @CohenArthur in #1057 (review)

@tschwinge
Copy link
Member Author

Actually I had wondered about the same thing. For ongoing development, everyone's probably using at least moderatly modern versions of the tools (like GCC used to compiler GCC/Rust). So, we cover those re testing.

Shouldn't then the CI system use old tools, to make sure that things work on that end, too?

@tschwinge
Copy link
Member Author

GCC 4.8 is the minimum version to be used for building current GCC.

@CohenArthur
Copy link
Member

I think it would be a good idea. I don't know how easy it would be to do in our CI. I personally develop with GCC 11.2 right now, but I'd more than happy if the CI was using GCC 4.8 only. Or we could have multiple builds, one with a modern version of GCC (i.e. whatever Ubuntu has) and one specifically for "backporting" testing

@tschwinge
Copy link
Member Author

Per https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#choosing-github-hosted-runners, we could use ubuntu-18.04 (bionic), and that one still provides GCC 4.8 packages: https://packages.ubuntu.com/bionic/gcc-4.8 etc.

@tschwinge
Copy link
Member Author

tschwinge commented Mar 24, 2022

@CohenArthur, per #1059 (thanks!) you're proposing to (a) add a separate GCC 4.8 job, instead of (b) re-purposing the existing one to GCC 4.8. I'm not objecting, but I'd like to understand/work out what's the benefit of (a) over (b)?

  • ❔ More testing. But as I said:

    For ongoing development, everyone's probably using at least moderatly modern versions of the tools (like GCC used to compiler GCC/Rust). So, we cover those re testing.

    ..., so not sure if that's all too useful? (We can't test "everything" anyway.)

  • 👎 More resource usage.


If doing (a), should we also add the #1026 checking here? (I'd say: yes. Needs a separate baseline file, obviously)


If doing (a), should we make this (1) "complete" (in particular, also add a test suite run here) vs. make this (2) as minimal as possible?

In theory, the test suite results should be exactly the same -- once GCC/Rust has been built, the "old GCC" and "new GCC" variants should behave exactly the same. On the other hand, GCC(/Rust) is implemented in C++, and you never know... ;-) So we could use this to verify that behave exactly the same is actually happening. But not sure if it's worth over (2), which would mean to only build GCC parts that actually use the "initial" GCC 4.8 compiler, as we're really only interested in GCC 4.8 successfully compiling the gcc/rust/ files -- other "old GCC verification" is assumed to be covered elsewhere (in GCC upstream).

If (2), then indeed (a) is a good variant, as that'll then be really quick to build: instead of standard make, we'd then only do make all-gcc, to only build the compilers (but not the target libraries, which use the compilers just built).

@CohenArthur
Copy link
Member

@tschwinge thanks for the detailed answer! I chose to implement it as a separate job for a few reasons, but I don't think they are objectively right or that we should keep it like so. They are simply "personal" preferences, which I thought would be a good base :)

I did not add the job to bors' configuration file, because I wanted it to be something separate and not necessarily mandatory for merging. I thought of this step as more of a "warning" as in: "This change does not allow gccrs to be compiled under the minimum supported GCC version anymore. We will need to look into it at some point". I believe that we should still be able to merge something for now, even if it might not be compilable using an older gcc. I could be completely off the mark on this, and I think it's a bit contradictory with my pushing -Werror in the CI forward, but I will expand on that later.

I believe that having GCC 4.8 as the main build step (your (b)) would make for bad user experience. I absolutely hate having to rely on github's CI to get necessary feedback on my changes, and would like the CI to mirror my local development environment: If my changes build on my machine using the latest GCC, then they should pass the main build step (Again, not saying this is objectively right: This is personal preference). Finding errors in the CI logs is not a good experience, and it must be even worse for new contributors. Furthermore, it is extremely slow: We only have two cores on the CI's machine. We also perform a clean build each time, as opposed to a (returning) contributors's local dev environment.

Trying to fix these errors is not a good experience either: You have to either have gcc-4.8 (and multilibs) available and perform a clean-build, or keep a separate 4.8 build directory. I personally would prefer to not have an older version of gcc and multilibs installed, as I regularly mismanage my system and end up with cryptic compilation errors :D The other solution is to launch an ubuntu-bionic container, which works, but is a pain. I'd also rather fix multiple of these issues at the same time since the workflow is not as pleasant with a separate container, but that's personal preference once again.

Regarding more resources being used: Since we are a public repository, we do have infinite CI minutes. If you are talking about the heavier environmental impact, then yeah, this is definitely worse.

Regarding testing: I am all for C++ being distrusted as much as possible! We should enable other forms of testing. I'm sure us misusing the older version of libstdc++ could make for some crashes or UB. We also have new warnings that remain to be fixed.

Regarding -Werror: warnings are easy to see and fix in your local dev environment. When we enforce -Werror on the CI, we simply ask contributors to pay attention to bright purple lines when building their branch. If we enforce builds using gcc-4.8, we enforce contributors to either have a gcc-4.8 environment, or to fish the errors from the CI's bland logs where warnings, errors, notes and successes have the same appearance.

We should also fix the new warnings that occur when building with gcc-4.8, and have another baseline file. Sadly, there are quite a few of them, and as pointed out before, they're annoying to fish out from the CI logs :) I'll probably have a crack at them using a container at some point.

@tschwinge
Copy link
Member Author

@CohenArthur, many thanks, that's exactly the discussion I was looking for.

You assert that a contributor's local development environment reasonably well matches the CI system's -- agreed -- and that it's thus likely to run into the same errors/warnings locally, and thus reasonably easy to address them. In contrast to GCC 4.8 specialities, which normal people don't use locally. (Well, I'm one of those who do -- for general GCC upstream "old GCC" build testing, so it was natural for me to also apply that for (some of!) my GCC/Rust builds.) So, yes, I do follow your rationale there, thanks.

What I think had confused me: I had not understood that your new build-and-check-older-gcc per the #1059 changes would not also apply to bors testing. But now I found that .github/bors.toml explicitly only requests build-and-check -- so this now makes sense, too. That means, the idea is that build-and-check-older-gcc is run for every Pull Request (as already seen in #1059), but won't affect mergeability by bors.

@CohenArthur
Copy link
Member

That means, the idea is that build-and-check-older-gcc is run for every Pull Request (as already seen in #1059), but won't affect mergeability by bors.

Exactly! Again, I don't think this is objectively the best answer. What do you think about it? I think we should also wait for Philip's opinion before deciding on a strategy for having an older-gcc build step in the CI

@tschwinge
Copy link
Member Author

I think I agree.

And I'm pretty sure that @philberty will say something like "fine with whatever you guys agree on" ;-) -- but not intending to put words into this mouth, of course.

@dkm
Copy link
Member

dkm commented Mar 25, 2022

I absolutely hate having to rely on github's CI to get necessary feedback on my changes,
...
Finding errors in the CI logs is not a good experience, and it must be even worse for new contributors.

I think mjw would be fine with this statement :). One thing to keep in mind is that without a github account, you don't have access to CI log.

@philberty
Copy link
Member

@tschwinge @CohenArthur @dkm Thanks guys for doing a great job here, I think the idea to add CI for GCC4.8 is great.

For the GCC bootstrap build I made this to only run upon merges to master so that it wouldn't inhibit work in PR's. I kind of see this as something similar, but if the job is ran in parallel with the normal PR CI job then maybe its ok and isn't too annoying.

I personally like all the automation we have but I know it can be cumbersome :)

@bors bors bot closed this as completed in bd1f435 Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants