-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
CI: Check MSRV in CI #37152
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
CI: Check MSRV in CI #37152
Conversation
This seems fine to me, but I think it makes sense that @sagudev looks at it. I'm mainly wondering about the overhead of the extra CI build. |
Since we don't actually use the artifact, can we do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10BC0Could we run it with --all-targets
and remove
servo/.github/workflows/linux.yml
Line 186 in 5580704
- name: Build libservo with examples |
No, because we want to test other OS too. I think we should simply have libservo workflow that will also be MSRV check at the same time.
I was wondering that too, but the cargo check documentation says:
If we could find out which errors those are and if they are relevant to us, then we could switch, but since I don't know I would prefer to take the conservative approach. |
There's some info in rust-lang/cargo#8650 (comment). Essentially, errors or lints in MIR passes are not reported with |
Could you elaborate a bit on what you envision? Have a seperate workflow to build libservo with our MSRV? And this check should be run for all OSs we support? |
Yeah this. Although we might want to run this in same workflow as normal build (to reuse cache and avoid double bootstrap). The point is that we do libservo and msrv check together, as msrv is only really relevant for embeddeders. |
What do you think about moving libservo build into a seperate job, since the |
Ah, you are right. Then that's the way to go. |
@sagudev It seems to me that |
I think it would be enough to have them only on full try run (so MQ and push). |
I think once the default toolchain and the minimum toolchain diverge, we will probably need to run at least the linux msrv / libservo job earlier, to avoid MQ failures. I'm not sure if we should run it on every push though, or rely on a label instead. |
8eb680a
to
1df2449
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but needs rebase.
As previously proposed on zulip and discussed in the coordination meeting, add a check to CI to see if servo still compiles with our minimum supported Rust version. To avoid requiring changes, we define our MSRV as the current version we are using now. This does not prevent us from updating the default compiler version, which we should still do, to get benefits like faster compile times, newer lints and making sure crown stays up-to-date. We simply test that libservo compiles in CI on Linux, since libservo (and dependencies) is what embedders would care about. We also don't need mach for this, so we just use cargo build. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
As previously proposed on zulip and discussed in the coordination meeting, add a check to CI to see if
servo still compiles with our minimum supported Rust version.
To avoid requiring changes, we define our MSRV as the current version we are using now (1.85.0).
This does not prevent us from updating the default compiler version, which we should still do, to
get benefits like faster compile times, newer lints and making sure crown stays up-to-date.
We simply test that libservo compiles in CI, since libservo (and dependencies) is what embedders would care about. We also don't need mach (or bootstrap!) for this, so we just use cargo build.
Testing: This PR adds a CI test.
./mach try windows-build-libservo linux-build-libservo mac-build-libservo