Skip to content

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

Closed
wants to merge 1 commit into from
Closed

rustbuild fixes #36986

wants to merge 1 commit into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Oct 5, 2016

Updated version of #32599

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@alexcrichton alexcrichton left a 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 {
Copy link
Member

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?

Copy link
Contributor Author

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());
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

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.

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 15, 2016

I see --shared-mode isn't an option for llvm-config on LLVM 3.7. Is there a timeline to drop 3.7 support?

@alexcrichton
Copy link
Member

Currently there aren't any plans to drop 3.7

@bors
Copy link
Collaborator

bors commented Oct 23, 2016

☔ The latest upstream changes (presumably #37347) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

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