-
Notifications
You must be signed in to change notification settings - Fork 167
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
Comments
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? |
GCC 4.8 is the minimum version to be used for building current GCC. |
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 |
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. |
@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)?
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 If (2), then indeed (a) is a good variant, as that'll then be really quick to build: instead of standard |
@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. |
@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 |
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 |
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. |
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. |
@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 :) |
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)
The text was updated successfully, but these errors were encountered: