-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Port tests/run-make-fulldeps/obtain-borrowck
to ui-fulldeps
#126073
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
//@ run-pass | ||
//@ check-run-results | ||
//@ run-flags: --sysroot {{sysroot-base}} --edition=2021 {{src-base}}/auxiliary/obtain-borrowck-input.rs | ||
//@ ignore-stage1 (requires matching sysroot built with in-tree compiler) |
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.
Incidentally, I was able to track down why these tests require stage 2 in ui-fulldeps.
It's because #110478 made “stage 1” tests in ui-fulldeps actually use the stage 0 compiler, so the test tries to use an in-tree compiler (via API) with a bootstrap sysroot, and predictably fails.
I have an idea of how to solve this (pass along two sysroots and let fulldeps tests select the correct one), but I haven't decided whether it's worth the extra complexity.
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.
Oh god.
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.
@Zalathar do you mind opening an issue for that so we don't lose track of it (since you probably have more context than me) and other people might chime in? And leaving a FIXME pointing to that?
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.
I haven't surveyed all of the ui-fulldeps
tests, but I would be very surprised if any of the tests actually relies on having the stage 2 sysroot being available?
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.
From what I can tell, ui-fulldeps tests fall into two categories:
- Those that use rustc crates in a way that doesn’t need a sysroot, so using stage 0 is fine and useful.
- Those that use rustc crates to invoke the compiler and thus need a suitable sysroot, so they’re all ignore-stage1.
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.
Ah I see, thanks for the explanation.
I have an idea of how to solve this (pass along two sysroots and let fulldeps tests select the correct one), but I haven't decided whether it's worth the extra complexity.
Seems reasonable to me, but I would like experts on T-bootstrap/T-compiler to help us diagnose if this the right fix for the interaction.
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.
i would also caution you, like, weigh the time it would take to get these working with stage1 against the time it would take to just build both sysroots
it's not super common to need to run ui-fulldeps
tests locally
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.
you don't want to know how long it took me to get #110478 working
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.
Lol. In that case, we should probably just leave it as is.
r? @jieyouxu |
Thanks for the port and explanations! I think it's reasonable to require stage2 versus trying to adjust |
…ouxu Port `tests/run-make-fulldeps/obtain-borrowck` to ui-fulldeps Thanks to `{{sysroot-base}}` from rust-lang#126008, this was also pretty straightforward to port over.
…ouxu Port `tests/run-make-fulldeps/obtain-borrowck` to ui-fulldeps Thanks to `{{sysroot-base}}` from rust-lang#126008, this was also pretty straightforward to port over.
…kingjubilee Rollup of 12 pull requests Successful merges: - rust-lang#125220 (Repair several `riscv64gc-unknown-linux-gnu` codegen tests) - rust-lang#126033 (CI: fix publishing of toolstate history) - rust-lang#126034 (Clarify our tier 1 Windows Server support) - rust-lang#126035 (Some minor query system cleanups) - rust-lang#126051 (Clarify an `x fmt` error.) - rust-lang#126059 (Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB) - rust-lang#126064 (Migrate `run-make/manual-crate-name` to `rmake.rs`) - rust-lang#126072 (compiletest: Allow multiple `//@ run-flags:` headers) - rust-lang#126073 (Port `tests/run-make-fulldeps/obtain-borrowck` to ui-fulldeps) - rust-lang#126081 (Do not use relative paths to Rust source root in run-make tests) - rust-lang#126086 (use windows compatible executable name for libcxx-version) - rust-lang#126096 ([RFC-2011] Allow `core_intrinsics` when activated) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#126073 - Zalathar:fulldeps-borrowck, r=jieyouxu Port `tests/run-make-fulldeps/obtain-borrowck` to ui-fulldeps Thanks to `{{sysroot-base}}` from rust-lang#126008, this was also pretty straightforward to port over.
Thanks to
{{sysroot-base}}
from #126008, this was also pretty straightforward to port over.