Skip to content

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