-
Notifications
You must be signed in to change notification settings - Fork 13.3k
rustbuild fixes #36986
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
rustbuild fixes #36986
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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.
Looks great, thanks!
} | ||
|
||
#[cfg(windows)] | ||
pub fn build_path(path: &str) -> PathBuf { |
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.
This is somewhat innocuously named, but perhaps it could be contained to just config.rs
which should be the only source of these paths (reading them from config.mk
which is generated from a shell script, configure
).
Perhaps this could be called msys_path_to_system(&path)
as well?
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 could use a nicer name
let llvm_mode = output(Command::new(&llvm_config).arg("--shared-mode")); | ||
if llvm_mode.trim() == "shared" { | ||
if cfg!(target_os = "macos") { | ||
let dypath = env::var_os("DYLD_LIBRARY_PATH").unwrap_or(OsString::new()); |
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.
Could this check LD_LIBRARY_PATH
on Unix as well?
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'm not sure that should be required (even if it currently is)
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.
Eventually we could perhaps update this so you didn't have to set that env var, but for now if we're already checking OSX we may as well check Linux, and it's definitely required today at least.
I see |
Currently there aren't any plans to drop 3.7 |
☔ The latest upstream changes (presumably #37347) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing due to inactivity, but feel free to resubmit with a rebase! |
Updated version of #32599