Skip to content
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

Cargo doesn't pass MSVC ToolsVersion to build.rs #1889

Closed
briansmith opened this issue Aug 11, 2015 · 7 comments
Closed

Cargo doesn't pass MSVC ToolsVersion to build.rs #1889

briansmith opened this issue Aug 11, 2015 · 7 comments

Comments

@briansmith
Copy link

My build.rs builds my native code by invoking msbuild.exe on the Visual Studio solution that normally builds it when it isn't being used with Rust. And, my Visual Studio solution is designed to build correctly with both MSVC 2013 and MSVC 2015.

I've found that Cargo will end up linking with VS 2015's link.exe (tools version 14.0), but that my %PATH% has msbuild 12.0 (VS 2013's) in the path. My build.rs needs to know which version of link.exe is going to be chosen so that it can build the code using the same version of the compiler (Code compiled with MSVC 12.0 cannot be linked with code compiled with MSVC 14.0).

To solve this problem, I propose that Cargo pass CARGO_MSVC_TOOLS_VERSION=12.0 or CARGO_MSVC_TOOLS_VERSION=14.0 in the environment when it runs build.rs based on which version of link.exe it selects.

@alexcrichton
Copy link
Member

This is kinda more of a compiler bug than a Cargo bug, Cargo doesn't do anything with MSVC toolchain selection, it's all the logic of the compiler itself. I also don't think that necessarily taking a look at msbuild is the best option as there's basically many various ways to select between VS versions. I think your best bet will be to run the full vcvarsall.bat if you want to select a specific VS version.

Regardless though if you'd like to pursue this I'd recommend opening an issue against rust-lang/rust.

@briansmith
Copy link
Author

Does the rust compiler run the build.rs script? I thought Cargo ran it.

@alexcrichton
Copy link
Member

Yes Cargo is the one running it, but Cargo doesn't do anything MSVC specific

@briansmith
Copy link
Author

Alex, can we re-open this?

New information:

  • The very first bug report I got for ring was due to this issue. This is evidence that people don't expect that they need to run vcvarsall.bat before running the build.
  • I have been working on some basic Cargo integration for the Visual Studio IDE. This problem occurs there too, and the hack of using vcvarsall.bat doesn't work (AFAICT) in that case.
  • Running command-line builds in a "VS2013/2015 native tools command prompt" is often inconvenient. In particular, the Git Powershell prompt (Git Posh) doesn't integrate well with that or with vcvarsall.bat due to differences in how batch files and Powershell work w.r.t. inheriting environment variables (AFAICT).

My suggestion is to iterate as follows:

  1. Add a -toolsversion={12.0|14.0} flag to rustc that overrides rustc's lookup heuristics.
  2. Add a -toolsversion={12.0|14.0} flag to cargo that gets forwarded onward to rustc and that controls the TOOLSVERSION environment variable in build scripts.
  3. Implement auto-discovery logic in cargo for calculating TOOLSVERSION based on what versions of msbuild are installed, always choosing the latest known-working version (currently VS2015).

#1 and #2 would be enough to enable good Visual Studio integration. #3 would make everything "just work" without developers needing to run vcvarsall or needing to run builds in a Visual Studio command prompt. In particular, this should enable builds from Git Posh to "just work."

@alexcrichton
Copy link
Member

Hm, so while I think that this is definitely an issue, I still don't think that it's really the compiler or Cargo that should be dealing with this. I agree that right now the Rust community doesn't expect to run vcvarsall before building, and so I'd like to avoid that requirement. (I definitely agree that "run from this specific command prompt" is onerous and doesn't compose well with all work flows)

Part of the design of build scripts, however, was to offload as much special-cased logic like this from Cargo and rustc to enable each crate to decide how it should work. For example the gcc crate has a function for findings VS tools by probing the windows registry. This basically mirrors the same logic in the compiler, and both of them recognize standard env vars to disable probing logic.

Along those lines, I'd at least prefer to see this prototyped outside of Cargo/rustc before considering moving it into one of those two. I'm extremely hesitant to place this in Cargo because it has 0 platform-specific logic for build systems and would prefer to keep it that way. It may be possible to modify the gcc::windows_registry::find function to also be able to find msbuild (it's probably just probing a few registry keys), and I assume that cmake at least has prior art here to draw from to find the executable. Note that this sort of auto-probing can and probably should take standard env vars into account to not override a decision that has been previously made.

Once we've got something working (which has to be implemented regardless of where this logic ends up) then if it starts taking off we can consider moving it into Cargo or the compiler, but I'd prefer to see it start externally first.

Does that make sense? I can help out in creating something that can auto-find msbuild for sure.

@briansmith
Copy link
Author

OK, sure, I think it is very reasonable to have this logic be in the gcc crate. I will modify my build scripts to use that technique, and if necessary I will write a patch to extend the gcc create.

Then I think the issue will boil down to whether cargo accepts a -toolsversion flag to override the default or whether the developer has to set an environment variable before invoking cargo, in order to override the default search logic. But, that's a much less significant thing.

Thanks for clarifying things for me.

@alexcrichton
Copy link
Member

Yeah I definitely think that any sort of auto-detection needs to be easily overridable, and in theory everyone agrees on the same override!

I'm more than welcome to extending the gcc crate (of which the name I'm really starting to regret!), I'm already using it elsewhere to find things like nmake.exe.

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

No branches or pull requests

2 participants