E53D CI: Check MSRV in CI by jschwe · Pull Request #37152 · servo/servo · GitHub
[go: up one dir, main page]

Skip to content

Conversation

jschwe
Copy link
Member
@jschwe jschwe commented May 27, 2025

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

@mrobinson
Copy link
Member

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.

@simonwuelker
Copy link
Contributor

Since we don't actually use the artifact, can we do cargo check instead?

Copy link
Member
@sagudev sagudev left a comment

Choose a reason for hiding this comment

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

10BC0

Could we run it with --all-targets and remove

- 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.

@jschwe
Copy link
Member Author
jschwe commented Jun 1, 2025

Since we don't actually use the artifact, can we do cargo check instead?

I was wondering that too, but the cargo check documentation says:

Some diagnostics and errors are only emitted during code generation, so they inherently won’t be reported with cargo check.

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.

@simonwuelker
Copy link
Contributor

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 cargo.check.

@jschwe
Copy link
Member Author
jschwe commented Aug 11, 2025

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.

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?

@sagudev
Copy link
Member
sagudev commented Aug 11, 2025

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.

@jschwe
Copy link
Member Author
jschwe commented Aug 11, 2025

What do you think about moving libservo build into a seperate job, since the libservo build would be with the MSRV compiler, while the other build jobs would be with the default compiler, hence cache anyway can't be shareD?

@sagudev
Copy link
Member
sagudev commented Aug 11, 2025

What do you think about moving libservo build into a seperate job, since the libservo build would be with the MSRV compiler, while the other build jobs would be with the default compiler, hence cache anyway can't be shareD?

Ah, you are right. Then that's the way to go.

@jschwe
Copy link
Member < F440 span aria-label="This user is the author of this pull request." data-view-component="true" class="tooltipped tooltipped-n"> Author
jschwe commented Aug 11, 2025

@sagudev It seems to me that build-libservo is currently entirely optional, and will only be executed on try runs if explicitly specified by the user.
Should I just enable it by default, or in which cases should we test the libservo build?

@sagudev
Copy link
Member
sagudev commented Aug 11, 2025

@sagudev It seems to me that build-libservo is currently entirely optional, and will only be executed on try runs if explicitly specified by the user. Should I just enable it by default, or in which cases should we test the libservo build?

I think it would be enough to have them only on full try run (so MQ and push).

@jschwe
Copy link
Member Author
jschwe commented Aug 11, 2025

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.

@jschwe jschwe force-pushed the msrv_ci branch 2 times, most recently from 8eb680a to 1df2449 Compare August 12, 2025 06:42
@jschwe jschwe requested a review from sagudev August 12, 2025 08:37
Copy link
Member
@sagudev sagudev left a 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>
@jschwe jschwe added this pull request to the merge queue Aug 12, 2025
Merged via the queue into servo:main with commit 4b91dc6 Aug 12, 2025
22 checks passed < 76B5 /div>
@jschwe jschwe deleted the msrv_ci branch August 12, 2025 19:57
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.

4 participants
0