-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Test that -Zbuild-std=core works on a variety of profiles #150524
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c57b138 to
ff1b625
Compare
|
The run-make-support library was changed cc @jieyouxu These commits modify the If this was unintentional then you should revert the changes before this PR is merged. These commits modify compiler targets. |
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.
It looks great! When a test failed, it was not clear which specific configuration had an issue, so I implemented a bit of context propagation for run-make commands to make that clearer. It's here, in case you'd like to use it directly.
One thing I am slightly concerned about is the duration of the test and the RAM usage. It's not a problem per-se, but rather because we execute everything in a single test, and thus it won't participate fully in the normal parallel load-balancing of cargo/rustc/bootstrap/compiletest.
If we had revisions for run-make tests, we could just build the matrix array in the declarative header of the test, and then let compiletest generate the full matrix of the configurations. But I don't think that's working today, and I don't think it's worth blocking this PR on that.
I'd say let's try to land it and see if people start running into issues with the test.
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.
Damn, it took ~11 minutes (https://triage.rust-lang.org/gha-logs/rust-lang/rust/59234759361#L2025-12-31T20:02:49.9025046Z) to run this test on aarch64-gnu-llvm-20-1, which gets run on every PR CI :( I wonder if there is anything that we could do to make the test faster? I guess that doing just check (if that's even possible with -Zbuild-std?) is not an option?
| let all_targets: HashMap<String, Value> = serde_json::from_str(&all_targets).unwrap(); | ||
| for (target, spec) in all_targets { | ||
| let metadata = spec.as_object().unwrap()["metadata"].as_object().unwrap(); | ||
| let tier = |
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.
Not strictly relevant to this PR, but I wonder if we should just remove Option from the tier field, since now we essentially have a test that will fail if some target does not have a tier. @jieyouxu WDYT?
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.
For these metadata, IMO we should evenutally move to enforce them... For custom targets, they should just specify tier 3, or we have sth like
enum TargetTier {
One,
Two,
Three,
Custom,
}
I think the right way to make this faster is to select 1, 2, or 3 targets to run in PR CI and run the full suite in auto builds only. Even if we just tested Is there a way for a test to detect which CI job is running it? If not, I suppose we could have the CI script execute the test on its own with some special argument or env var to say "test all targets" or something like that. |
|
Using an environment variable would be possible, but also it would be a bit opaque if in order to run something that might have failed on CI you would need to start passing some custom env. var. to bootstrap/compiletest. We generally try to avoid stuff like that. Instead, I was thinking of creating a separate There is still the question of how to ensure that the full matrix test won't run by default on CI in all jobs that just execute run-make tests though (without having to manually skip it). We could either just make it ignored/skipped by default, and somehow enable it selectively (is there a way to run a test only if it is manually selected?), or even create a completely new test run-make suite that would be used for build-std tests (similar to run-make-cargo). I'd be curious what @jieyouxu thinks. |
Yeah, this is why we try to thread the info actually through bootstrap -> compiletest -> run-make-support -> rmake.rs where possible. For instance, whether std was built with debug assertions come from bootstrap's config, which gets fed to compiletest via a new compiletest cli flag, which in turn for tests/run-make, compiletest sets an internal env var based on that (e.g.
This seems to me like potentially a better option.
A separate suite would be okay, it'd still be |
|
This feels like a new test suite, because it's a large number of independent builds that all can run at the same time and pass or fail completely independently of each other. But the set of tests is not well-described by files on the filesystem, it's best implemented with code as I've written here. If this were its own test suite, it would be a test suite with a single very parallel test. Which is goofy. Or it would be entirely built into bootstrap, I suppose that's not too weird because |
TBH I personally think it isn't too goofy. I wonder if we could also use this test suite for other "heavier" I kinda rather have a test-suite-with-single-test over "this particular |
|
Yeah, I wouldn't mind creating a new test suite for this. I expect we'll want to have other build-std tests, it's currently heavily undertested, and before stabilization we need to change that anyway. |
0a72aa7 to
6d7359d
Compare
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
What do we think about testing a few targets in PR CI and more in auto builds? If that sounds like a good idea, how would I tell the test logic which set of targets to test? |
This comment has been minimized.
This comment has been minimized.
Thread `--jobs` from `bootstrap` -> `compiletest` -> `run-make-support` Context is rust-lang#150524 (comment), where we would like to thread the `--jobs` config from bootstrap explicitly through to run-make tests without relying on an "external env var" that bypasses the build/test infra. Note that this PR currently intentionally couples the jobs configured for *builds*, versus for `TestMode::RunMake` tests. We can further specialize some kind of `run-make-jobs` bootstrap config *if actually needed*; I will keep this configuration naive for now. r? @Kobzol
Thread `--jobs` from `bootstrap` -> `compiletest` -> `run-make-support` Context is rust-lang#150524 (comment), where we would like to thread the `--jobs` config from bootstrap explicitly through to run-make tests without relying on an "external env var" that bypasses the build/test infra. Note that this PR currently intentionally couples the jobs configured for *builds*, versus for `TestMode::RunMake` tests. We can further specialize some kind of `run-make-jobs` bootstrap config *if actually needed*; I will keep this configuration naive for now. r? @Kobzol
Thread `--jobs` from `bootstrap` -> `compiletest` -> `run-make-support` Context is rust-lang#150524 (comment), where we would like to thread the `--jobs` config from bootstrap explicitly through to run-make tests without relying on an "external env var" that bypasses the build/test infra. Note that this PR currently intentionally couples the jobs configured for *builds*, versus for `TestMode::RunMake` tests. We can further specialize some kind of `run-make-jobs` bootstrap config *if actually needed*; I will keep this configuration naive for now. r? @Kobzol
|
I think that the easiest would be to simply create one test that will only test x64, and then another test that will run the whole matrix, and then we'll hardcode the x64 test and just run it, instead of running the whole test suite. We can move the shared |
Rollup merge of #150717 - jobs, r=Kobzol Thread `--jobs` from `bootstrap` -> `compiletest` -> `run-make-support` Context is #150524 (comment), where we would like to thread the `--jobs` config from bootstrap explicitly through to run-make tests without relying on an "external env var" that bypasses the build/test infra. Note that this PR currently intentionally couples the jobs configured for *builds*, versus for `TestMode::RunMake` tests. We can further specialize some kind of `run-make-jobs` bootstrap config *if actually needed*; I will keep this configuration naive for now. r? @Kobzol
|
(Let me know if you want/need help with a new test suite, which I think is the direction we probably want to take) |
See #t-infra > Non-blocking testing for -Zbuild-std? for some background.
This is an incredibly CPU-hungry run-make-cargo test, but at least on my desktop the entire suite only takes a minute.