Failure to locate link.exe in preconfigured environment

[retep998]

Someone has their environment configured via start-shell-2015-x64.bat so they can build mozilla-central. In stable this worked fine, but in beta this fails with the following error:

 error: could not exec the linker `link.exe`: The system cannot find the file specified. (os error 2)
   |
   = note: "link.exe" "/NOLOGO" "/NXCOMPAT" "/LIBPATH:C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "d:/m-c/obj/win64fre/toolkit/library\\release\\build\\rayon-core-004e43061fffe540\\build_script_build-004e43061fffe540.0.o" "/OUT:d:/m-c/obj/win64fre/toolkit/library\\release\\build\\rayon-core-004e43061fffe540\\build_script_build-004e43061fffe540.exe" "/OPT:REF,ICF" "/DEBUG" "/LIBPATH:d:/m-c/obj/win64fre/toolkit/library\\release\\deps" "/LIBPATH:C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libstd-26aaf8685d64fee9.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\librand-ce86047000b56785.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libcollections-b8b9a576d130e244.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libstd_unicode-9fbe5d3bbc85c563.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libpanic_unwind-14e8bb7ca07ebf2c.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libunwind-a4bc20050f473f79.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\liblibc-892bd58ec617c3bd.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\liballoc-b9c9173c47c20c41.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\liballoc_system-141f235246f01712.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libcore-3a6338503b91076c.rlib" "C:\\Users\\me\\.rustup\\toolchains\\beta-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libcompiler_builtins-e9e280acad4314a4.rlib" "advapi32.lib" "ws2_32.lib" "userenv.lib" "shell32.lib" "msvcrt.lib"

Because it is a preconfigured environment, link.exe is definitely in PATH which makes the issue more bizarre.

$ where link
c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\link.exe

[froydnj]

OK, so we sat down and debugged this today. Mozilla's build environment for invoking cargo is, shall we say, unique, in that we clear out some environment variables, including PATH:

https://dxr.mozilla.org/mozilla-central/source/config/rules.mk#886

We do not, however, remove VCINSTALLDIR.

Current master (and, I assume beta) uses gcc-rs:

https://github.com/alexcrichton/gcc-rs/blob/master/src/windows_registry.rs#L68

So if VCINSTALLDIR is set, we only look in PATH for link.exe, and have no fallback otherwise. On stable, however, we have some more complicated logic for locating the linker:

https://github.com/rust-lang/rust/blob/stable/src/librustc_trans/back/msvc/mod.rs#L58

Specifically, we will look for VCINSTALLDIR and try to locate link.exe in PATH. If that doesn't succeed, then we will try to look in known Visual Studio installation directories:

https://github.com/rust-lang/rust/blob/stable/src/librustc_trans/back/msvc/mod.rs#L93

We have the same logic for looking for known Visual Studio installation directories in master and beta:

https://github.com/alexcrichton/gcc-rs/blob/master/src/windows_registry.rs#L76

But in gcc-rs, we assume that if VCINSTALLDIR is set, then we shouldn't even bother probing specific MSVC installation directories.

So we have a behavior change here between stable Rust and beta/master Rust. Whether this was intended, I can't say, nor whether the current master behavior is a good idea. It's possible that Firefox should also be removing VCINSTALLDIR and a few other variables from the environment, too.

@alexcrichton it looks like you wrote the bits in gcc-rs; was the code in gcc-rs intended to exactly mirror the old rustc behavior?

[retep998]

In this case I'd argue it is firefox's responsibility to ensure the environment variables are a good state. Attempting to auto-detect a VC++ installation when there is already a VC++ configured in the environment can potentially lead to conflicting VC++ versions and bad things happening.

[alexcrichton]

@froydnj gah sorry for the regression! Definitely not intentional. The probing logic here has always been... tenuous at best. It's just been a best-effort attempt and has mostly worked up to this point. We'd be more than happy to take a patch to gcc-rs to restore the old behavior and pull that into the compiler.

IIRC this is used for cross-compiling in gecko, right? There's an x86_64 rustc but you're building an i686 firefox, which means that rustc may build x86_64 procedural macros (e.g. serde) which would be incorrect if the compiler found the i686 linker for the x86_64 artifact (as that's what the environment is configured to do).

If that's the case, I'd imagine three possible solutions here:

• Update gcc-rs to the old behavior
• Remove more env vars in Gecko
• Update rustc itself to detect "misconfigured" cross-compilation and ignore env vars in this case, falling back to probing logic.

I'm fine with any of those solutions, @froydnj do you have a preference?

[froydnj]

Yeah, all this stuff is required to make the right thing happen on a 64-to-32 cross compilation situation. We wound up unsetting VCINSTALLDIR in the environment, and doing that makes stable and beta behave the same way. Updating gcc-rs to the old behavior sounds attractive, but it's not clear to me that the old behavior was necessarily correct, and the current behavior is likely more correct, as @retep998 points out.

I think we want a more robust solution for the long-term though; when this issue first came up, @alexcrichton outlined several possible solutions, one of which is rust-lang/cargo#3915 and one of which would be modifying rustc itself to detect this cross compile situation and try to locate the correct linker. Eventually we'll want to require MSVC 2017 which, IIUC, gcc-rs will not attempt to probe installation directories for and for which we'd need to pass in the correct PATH environment variable anyway. We'd then be dependent on whatever smarts live in Cargo or rustc.

I think it's probably easier to make rustc (via gcc-rs?) detect "misconfigured" cross-compilation and compensate accordingly.

[alexcrichton]

Oh support recently landed in gcc-rs for detecting MSVC 2017 so I think that may be solved now? I'm not sure how bullet-proof the logic may be though so it may need an update!

I agree though that the best solution here is likely detecting misconfigured cross-compile scenarios and falling back to rustc's detection at runtime as opposed to using the configured environment. I'll open a dedicated issue for that: #43069

@froydnj if that's the main action item from this issue, should we close this?

[brson]

Thanks for the thorough investigation @froydnj

[brson]

If we don't make any changes here it's still a compat note.

For the notes, I think the issue here is that if VCINSTALLDIR is set, rustc will unconditionally attempt to use it to set up the MSVC linker, where before it ignored that variable. This can cause rustc to fail to find the linker if VCINSTALLDIR is not configured correctly for the environment.

Does that sound accurate @froydnj ?

[froydnj]

@alexcrichton Yes, I think addressing this on the rustc side is all we need here, so we can close this!

@brson That release note sounds great. Thanks for writing it up!

[retep998]

If VCINSTALLDIR is set rustc will attempt to find link.exe in PATH and invoke it. The issue is what happens when link.exe turns out to not be in PATH, where in the past it would fallback to auto-detection, and at the moment it does not. The actual value of VCINSTALLDIR isn't important. Instead its presence is used as a clear indicator of whether the environment is configured, because nobody ever sets VCINSTALLDIR except by configuring their environment with vcvars.

Until now, nobody ever gave much thought about what rustc should do when the environment is in a partially configured state, because that's not something which should really happen, either you use vcvars or you don't. I don't think we should try to support partially configured environments because at that point it is a custom configuration and it is the user's responsibility to ensure it is in a sane state. However, I do think we should try to detect when the configured environment doesn't match the target architecture and purge the environment variables.

[alexcrichton]

Ok thanks for the info @froydnj! I'll close this in favor of #43069 in that case