-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
fix testing Miri with --stage 0 #85959
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors rollup=iffy |
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.
@@ -45,6 +45,7 @@ pub fn libdir(target: TargetSelection) -> &'static str { | |||
} | |||
|
|||
/// Adds a list of lookup paths to `cmd`'s dynamic library lookup path. | |||
/// If The dylib_path_par is already set for this cmd, the old value will be overwritten! |
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.
Can we fix it to not do this? Overwriting the old value seems very confusing.
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.
(a) this is pre-existing, I'd rather not touch this since I have no idea what changing this will break,
(b) fixing it requires an unstable feature (#44434), which AFAIK bootstrap tries to avoid.
Okay, so it seems reasonable then to ignore error_index_generator for this PR, I would say. The main question that remains is if |
IMO this is probably fine -- it should be ok to have more libraries in the search path, generally speaking, especially for tools which aren't generally linked dynamically (other than to rustc/llvm, and then we just need these libraries in the search path). So I think this PR is good if it works -- r=me if we've tested that. |
I can't tell if "this" is what the PR does, or the proposal of calling
What kind of tests are you thinking of? I did verify that |
Oh, I see -- yeah, this PR as is, or the more invasive patch both seem OK. That verification seems sufficient for now. I'm not entirely sure this is correct but it does seem like the appropriate start. @bors r+ |
📌 Commit 9e674af has been approved by |
☀️ Test successful - checks-actions |
Fixes #78778 for Miri.
The issue remains open for error_index_generator, which has its own dylib logic:
rust/src/bootstrap/tool.rs
Lines 396 to 400 in 903e369
@jyn514 I could just copy the logic from
add_rustc_lib_path
, but that does not seem great. Any other suggestions?Also I wonder if maybe
prepare_tool_cargo
should already calladd_rustc_lib_path
.