Uh oh!
There was an error while loading. Please reload this page.
Overhaul MSVC linker and Windows SDK detection code - #30233
Conversation
retep998
commented
Dec 6, 2015
rust-highfive
commented
Dec 6, 2015
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
retep998
commented
Dec 6, 2015
cc @vadimcn |
00b9542 to
bf1cb1bCompareretep998
commented
Dec 6, 2015
Testing by @DanielKeep confirms that this also works with the standalone Microsoft Visual C++ Build Tools 2015 Technical Preview, providing an option for users that want to use msvc but without installing the entire VS IDE. |
There was a problem hiding this comment.
Instead of reverse here you should also be able to do dirs.into_iter().rev().filter(...) below
alexcrichton
commented
Dec 11, 2015
I'm a little worried about this logic for a few reasons:
I think that if this passes manual testing that we should land this, however, as fixing compatibility with the most recent VS is likely more important for now. It'd be great, however, to get a definitive answer on what this logic should be (or perhaps providing a better guarantee that it'll work), but I unfortunately don't know how to do that :(. cc @vadimcn |
There was a problem hiding this comment.
Is this because of env vars, or just to avoid having to detect bitness of the host OS?
There was a problem hiding this comment.
Mainly just to avoid having to detect bitness of the host OS. I'll add a FIXME comment saying this should probably detect the host OS bitness.
vadimcn
commented
Dec 11, 2015
I don't know which approach is better off the top of my head.
The VC's registry structure will likely stay the same for the next few releases, so we could try to extrapolate versions into the future and hope that it works. |
retep998
commented
Dec 12, 2015
Currently you can already override the entire system by just executing the proper vcvars bat file causing this code to back off and just trust everything is setup correctly and it'll work. Since we never know what sort of changes will occur in new versions of VS (such as VS 2015 adding the UCRT and the Windows 10 SDK both of which required new logic), I think it's better to not try to guess and just add support for it once someone can actually test it out. As for testing this, I have VS 2015 2013 and 2012, and by commenting out some of the versions in that VS version array I could test how it handles all three versions and it works perfectly. It falls back to the 8.1 SDK for 2015 when there's no 10 SDK, it uses the 8.1 SDK for 2013 and it uses the 8 SDK for 2012. It still would be nice to get people testing it on other setups though as I've done all my testing on 64-bit Windows 10 only. This code should work fine for new releases of the Win10 SDK as I've borrowed the exact logic from vcvars to find the newest Win10 SDK. Considering Microsoft said that Windows 10 is their final version of Windows, this means we may be in the clear with regards to that. New versions of VS however are still a possibility although the user can always just execute vcvars to setup the environment themselves. The reason I rewrote this was because I felt like it wasn't robust enough. This implementation should very closely mirror what vcvars does and I'm more willing to trust what Microsoft does than what clang does, at least with regards to Microsoft's own software. |
ghost
commented
Dec 12, 2015
We can now also download the standalone / command-line Build Tools 2015, without even installing full VS2015, which installs compilers+SDKs+other utilities to compile C/C++ code on Windows. Although the package itself is a Technical Preview, but I have successfully compiled with it on Windows 7 (fresh install) to Windows 10 and build Windows XP+ compatible libs. The CL compiler version is same as the one shipped with VS2015 Update 1. It seems like this CLI-base is in preparation for Server 2016 and Nano Server feature called "containers". Also Windows team is working to provide "docker containers" (to bridge with their "native containers") GUI/CLI, where installing VS won't be a viable option: https://github.com/docker/docker/pulls/jhowardmsft. |
ghost
commented
Dec 12, 2015
I know at least detecting Windows 10 SDK (which is now at a new location the first time in a long time compared to its predecessors) hasn't done correctly by gyp, the build system favored by Chrome and node.js native modules projects. Initially gyp was developed because cmake was considered not meeting the requirement. Chrome guys are now moving away (or already) from their own gyp to another build system by Google called gn which seems to have fixed this SDK detection issue (but it is definitely in amateur hour to be fully trusted). |
retep998
commented
Dec 13, 2015
@vadimcn |
alexcrichton
commented
Dec 14, 2015
I agree with @vadimcn that we should definitely have an escape hatch for future proofing, but I also agree with @retep998 that we should have the necessary detection in place that if you're in a VC shell then rustc won't be doing anything fancy. I also agree that mirroring what Microsoft does is likely better than Clang, and for now we claim we need VS 2013+ anyway so as long as we're compatible with those I'm happy. Thanks again @retep998! r=me with the debugging prints and such removed |
alexcrichton
commented
Dec 14, 2015
Or better yet actually, feel free to just convert them all to logging |
Fixesrust-lang#30229 Signed-off-by: Peter Atashian <retep998@gmail.com>
612e2d6 to
915cb37Compareretep998
commented
Dec 15, 2015
Switched it to use |
alexcrichton
commented
Dec 15, 2015
bors
commented
Dec 15, 2015
⌛ Testing commit 915cb37 with merge ecae5f2... |
bors
commented
Dec 15, 2015
💔 Test failed - auto-mac-64-nopt-t |
alexcrichton
commented
Dec 15, 2015
@bors: retry On Mon, Dec 14, 2015 at 7:53 PM, bors notifications@github.com wrote:
|
bors
commented
Dec 15, 2015
⌛ Testing commit 915cb37 with merge 98ef4cc... |
bors
commented
Dec 15, 2015
💔 Test failed - auto-mac-64-nopt-t |
retep998
commented
Dec 15, 2015
I'm getting somewhat annoyed at that mac buildbot and its spurious failures. |
Manishearth
commented
Dec 15, 2015
@bors: retry |
bors
commented
Dec 15, 2015
…alexcrichton What I've done here is try to make the code match what vcvars does much more closely. It now chooses which SDK to find based on the version of MSVC that it found. It also bases the decision of whether to find all the things on whether `VCINSTALLDIR` has been set, which is more likely to have only been set by an invocation of vcvars, unlike previously where it would do some things only if `LIB` wasn't set even though there was a valid use case for libraries to add themselves to `LIB` without having invoked vcvars. There are still some debug `println!`s so people can test the PR and make sure it works correctly on various setups. It supports VS 2015, 2013, and 2012. People who want to use versions of VS older (or newer) than that will have to manually invoke the appropriate vcvars bat file to set the proper environment variables themselves. Do not merge yet. Fixes#30229
bors
commented
Dec 15, 2015
brson
commented
Jan 7, 2016
Requested for backport in #30229 |
alexcrichton
commented
Jan 12, 2016
Accepted for a backport |
What I've done here is try to make the code match what vcvars does much more closely. It now chooses which SDK to find based on the version of MSVC that it found. It also bases the decision of whether to find all the things on whether
VCINSTALLDIRhas been set, which is more likely to have only been set by an invocation of vcvars, unlike previously where it would do some things only ifLIBwasn't set even though there was a valid use case for libraries to add themselves toLIBwithout having invoked vcvars.There are still some debug
println!s so people can test the PR and make sure it works correctly on various setups.It supports VS 2015, 2013, and 2012. People who want to use versions of VS older (or newer) than that will have to manually invoke the appropriate vcvars bat file to set the proper environment variables themselves.
Do not merge yet.
Fixes#30229