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

Fall back to MSVC link.exe detection if env looks "misconfigured" #43069

Closed
alexcrichton opened this issue Jul 5, 2017 · 9 comments
Closed
Labels
A-linkage Area: linking into static, shared libraries and binaries C-enhancement Category: An issue proposing an enhancement or a PR with one. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Build systems like Firefox tend to want to configure MSVC ahead of time to ensure that compiles use the right compiler/linker. This means that the various env vars corresponding to the link.exe invocation will be set before rustc is invoked. This in turn currently means that rustc will blindly use this configuration for all invocations of the linker, even if it's actually wrong.

One situation where this becomes a problem is when you're cross compiling. For example you may have an x86_64-pc-windows-msvc host compiler and then you're compiling to i686-pc-windows-msvc. This means that the environment specifies a linker that knows how to create 32-bit executables, but you may need to create 64-bit shared libraries as part of the build process (procedural macros).

The compiler should detect this situation and help out. If the environment looks clearly invalid for the given target, we should assume that this situation is happening and basically scrub the environment. The compiler would fall back to its own internal detection which should correctly locate a linker.

@alexcrichton alexcrichton added the O-windows-msvc Toolchain: MSVC, Operating system: Windows label Jul 5, 2017
@DoumanAsh
Copy link

I would think that erroring on a bad configuration instead of attempting to fix it would be a better approach.
After all user had reason to pre-configure environment.

Instead of auto-reconfiguring, it would be better to have flag in cargo/rustc to auto-fix if user is lazy.
I don't like idea of cargo/rustc fixing my environment.
I'd prefer to find out my error before proceeding, otherwise user will not detect his error at all.

P.s. I'm talking about general case, not very complex configuration that are attempting to build both x64 and x32 binaries/libs

@alexcrichton
Copy link
Member Author

@DoumanAsh in general I'd agree, yes, but in practice it seems like it's very common to have a misconfigured environment and it'd be relatively easy for us to detect it as well. Because this is so common it implies to me that we want a better user experience than erroring-out, and because it's easy to detect that implies to me that it's feasible to do this.

To be clear I don't think Gecko is the only one running into this. If you start up an MSVC shell you're in the same situation where you otherwise wouldn't be able cross-compile. Put another way, if you have MSVC env vars, it's impossible to cross-compile and this is just opening up the possibility to do so.

@DoumanAsh
Copy link

DoumanAsh commented Jul 5, 2017

Because this is so common it implies to me that we want a better user experience than erroring-out, and because it's easy to detect that implies to me that it's feasible to do this.

Wouldn't it be better to actually recommend users to use clean environment so that Rust would auto-detect everything?
Besides that improving error would be beneficial.

Put another way, if you have MSVC env vars, it's impossible to cross-compile

Yes, but if user wants auto-configuration, wouldn't it make more sense to not initiate MSVC env vars?
It makes sense to auto-detect environment on clean environment.
But when user, for example I have script to configure x32/x64 build environment for my console, uses MSVC environment, it would be better to not implicitly to do it.
The reason is that user DID started MSVC environment for a reason.
By handling all errors on its own, Rust hides user's problems from it.

I believe user configuration should take precedence over rust/cargo own, after all it is what user wanted.
Otherwise user should use clean environment to leave everything to Rust/Cargo.
Or at least i would prefer this to be a configurable option.

@retep998
Copy link
Member

retep998 commented Jul 5, 2017

At the very least, if we do implement this fall back, there should be a big loud warning that Rust has detected the environment is configured for the wrong architecture and will attempt to detect stuff itself. I'm opposed to rustc silently ignoring the configured environment.

@alexcrichton
Copy link
Member Author

@DoumanAsh

Wouldn't it be better to actually recommend users to use clean environment so that Rust would auto-detect everything?

Not always, no. If there are multiple installations of visual studio then env vars are the only way to force rustc to use a particular one.

I believe user configuration should take precedence over rust/cargo own, after all it is what user wanted.

Of course. I'm only advocating when we clearly detect a failure mode that we do something different. We could even run what's configured first and only if that fails see if it's a known reason why it failed, only then falling back to other detection. There's a lot of possibilities here.

@DoumanAsh
Copy link

Not always, no. If there are multiple installations of visual studio then env vars are the only way to force rustc to use a particular one.

In this case wouldn't be nice to have option to configure VC Studio/build tools installation for use?
Would it be possible somehow? Maybe through Rust/Cargo env variable?

Well as long as it is not enforced and user will be well-informed it is good.

@alexcrichton
Copy link
Member Author

Yes it's possible. I would rather we not spiral this issue into new env vars and/or configuration option bikesheds.

@codyps
Copy link
Contributor

codyps commented Jul 5, 2017

This sounds quite a bit like the reason we have CC_x86-64_pc-linux-gnu, HOST_CC, TARGET_CC, CC environment variables in gcc-rs.

Please correct me if I'm not understanding the issue, but it seems like we could use the model introduced there to allow specific configuration rather than doing hard-coded fallbacks.

In this case, using the same "most specific to least specific" variable fallback for the VCINSTALLDIR seems like a very reasonable solution that mirrors the solution in gcc-rs for unix systems.'

This would require that folks needing specific configuration export different VCINSTALLDIR flavors, of course.

That said: using environment vars here always feels a bit awkward. Ideally we'd somehow tie them into the target (as some sort of variables), but that potentially needs some work improving how target specification works (to allow extending with these variables to work).

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 28, 2017
@Enselic Enselic added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 22, 2023
@ChrisDenton
Copy link
Member

This was long ago addressed in cc-rs (which Rust uses for vs detection) by looking for VSCMD_ARG_TGT_ARCH environment variable and, if that differs from the current target, then it uses VSINSTALLDIR to ask for the right target libs/bins. In that way cross-compiling works seamlessly whilst still respecting the environment. So if the target is i686 and the host is x86_64 then both build scripts and crates themselves can compile rather than one or the other failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-enhancement Category: An issue proposing an enhancement or a PR with one. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants