Fix escaping in msvc builds

[Diggsey]

This fixes #28018 with the exception of the point about cmake, but that's really a limitation of ./configure builds.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[Diggsey]

r? @alexcrichton

[alexcrichton]

@bors: r+ 301b23e8dd66daf1669d35854866b34cb245240e

Thanks!

[Diggsey]

Actually there's one more problem with it as it stands, so don't merge just yet.

[alexcrichton]

@bors: r-

[Diggsey]

I discovered that the cmake build also requires vcvarsall.bat has been run, or it will mysteriously fail with no useful error output, so I've updated the code to do that. As you can see from the diff it's not nearly as simple as it should be...

Now the only requirements should be:

• Appropriate version of MSVC is installed
• cmd is on msys PATH
• cmake is on windows PATH

[alexcrichton]

I think there's something funny going on with cmake that we may want to detect otherwise, I've never run cmake within vcvarsall or cmd and it's always worked for me.

We already have detection of a "bad python" so I think we may want to detect a "bad cmake" instead of adding this specific logic. Which cmake package are you using?

[Diggsey]

@alexcrichton I am running the latest windows cmake - I would like to point out that you cannot have been running cmake outside of vcvarsall without this change as ./configure was never successfully able to run vcvarsall at all before this change. On this line:

include_path=$(cmd /c "\"$vcvarsall\" $msvc_part && cmd /c echo %INCLUDE%")

The /c is translated by msys into C:\ and the first half of the command fails to do anything useful (without /c it will just ignore the entire command line and enter an interactive session, which will terminate immediately because stdin is empty, but will return success, so configure is none the wiser).

Are you sure hadn't previously run vcvarsall directly in the terminal, or already had the relevent paths in your INCLUDE/LIB environment variables?

[alexcrichton]

Hm, something fishy is definitely going on... My workflow is to just open up a MSYS2 shell, cd into the rust source tree, run ./configure --build=..., and then build. The bots also have very few environment variables set up for this, they basically just configure PATH so most things are there and everything works out OK.

When you say you're running the latest windows cmake, where'd you install it from? Is it the cmake package in MSYS2? The mingw-w64-x86_64-cmake package? The CMake installer from their site?

[Diggsey]

@alexcrichton Yes, the installer from the cmake site: http://www.cmake.org/files/v3.3/cmake-3.3.1-win32-x86.exe. The msys version of cmake cannot build for visual studio (by design). That's the reason I send commands via cmd, so it will look on the windows path for cmake rather than the msys path (it's not strictly necessary if the windows version is also on the msys path, but if you have both versions of cmake installed it means that you can set up PATH differently between msys and windows, so that the two versions don't conflict, and rust will automatically use the windows version)

When I spoke to @retep998, he said that he also had manually run vcvarsall.bat before configuring.

If as you say the bots are configured so that they have most things on PATH already, couldn't that explain why it works? A normal VS installation will not add itself to PATH or modify environment variables, but whatever installs it on the bots may do things differently?

[retep998]

When I spoke to @retep998, he said that he also had to manually run vcvarsall.bat before configuring.

It actually works just fine for me without first doing the vcvars stuff. All I have to do is make sure the non-msys cmake is in my PATH and everything works fine.

[Diggsey]

Here's a log of the errors that occur without this change.

First I run ./configure - when that fails due to cmake, I cat in the contents of CMakeError.txt:

https://gist.github.com/Diggsey/63b6d64c31fd4922edb4

[alexcrichton]

Unfortunately I'm not quite sure what's going on there, I can't quite parse the output of cmake.

[Diggsey]

This is what I've discovered:

• Running ./configure --build=x86_64-pc-windows-msvc from mingw64 terminal on a fresh checkout of master will succeed (but CFG_MSVC_INCLUDE_PATH_x/CFG_MSVC_LIB_PATH_x do not include the paths set by vcvarsall.bat, although that doesn't seem to matter)
• Running the same command from bash running inside cmd will fail with ./configure: eval: line 1226: unexpected EOF while looking for matching "', due to cmd /c getting path converted to cmd C:\.
• Running cmd /c "echo test" directly from mingw64 terminal will succeed (the /c is not converted)
• Running cmd /k "echo test" in the same terminal will fail (the /k is converted to K:\)
• Running cmd /c "echo test" from bash in cmd will fail
• Running cmd.exe /c "echo test" from bash in cmd will succeed
• Running cmd.exe /k "echo test" from bash in cmd will fail
• Running which cmd and which cmd.exe show that they resolve to the same executable in all circumstances
• In all cases, the conversion of /c to C:\ is done by the parent shell, before the child process is started.
• Running cmd by its full unix-style path will always cause the /c to be converted, and fail.
• Running cmd by its full windows-style path behaves exactly the same as just running cmd or cmd.exe depending on whether the extension is included.

The only conclusion I can come to is that bash/msys does some bizarre guessing game, based on the environment, the terminal, the filename, and the arguments passed in, to infer whether the command is a windows or a unix command. If it infers the latter, it converts the path.

The only saving grace is that using //c as I have done in this PR seems to work in all circumstances, regardless of how cmd is invoked.

edit:
OK, after getting a response on the msys IRC channel, I figured out what's going on (mostly):

• Cygwin has a special exception for cmd /c: https://github.com/Alexpux/Cygwin/blob/msys2-master/winsup/cygwin/spawn.cc#L373-L374
• It will only detect cmd, if either: 1) it's called as cmd.exe or 2) it's called as cmd, and /usr/bin is on PATH before the windows path.
• Running msys make from cmd directly is not supported - it must be executed via a bash login shell, which will set up PATH correctly.

It still seems like a bad idea to rely on the special casing of cmd /c, so most of this PR still stands, but it may not be necessary to run vcvarsall.bat before executing cmake - I'll experiment with that part more thoroughly tomorrow.

[alexcrichton]

Turns out you really do learn something new about msys every day...

I'm fine with //c and business, but can you clarify the changes to cmake and rustc_trans::back::msvc? I'm a little surprised those still both need changing.

[Diggsey]

@alexcrichton There are four changes here (apart from //c):

1. Calling cmake via cmd
   This will make sure that the windows or mingw builds of cmake will be preferred over msys cmake. Without this, installing the cmake package in msys will break the build.

2. Changes to how include_path and lib_path are set (specifically the addition of //V:ON and \&).
   These never worked as intended. Under some circumstances (as documented above) vcvarsall.bat was successfully called. However, due to the way environment variables are substituted in cmd (http://ss64.com/nt/delayedexpansion.html), include_path and lib_path were expanded before that happened. This didn't seem to break the build.
   Also, cmd uses a single & rather than && (this differs between command line vs batch file execution).

3. Checking for VisualStudioVersion rather than LIB in rustc_trans::back::msvc
   The purpose of this check is to determine whether vcvarsall.bat has already been run (as a way to allow the user to override which msvc version is used by rustc). Checking VisualStudioVersion is a more reliable way to do this, as the LIB environment variable is not guaranteed to be empty, even when vcvarsall.bat has not been called. (For example, some SDK installers will add to LIB and INCLUDE environment variables)

4. Calling vcvarsall.bat directly before configuring cmake
   This was necessary for to get it to build on my machine: I'm still in the process of determining why that was the case, and whether my setup was incorrect, or whether it will happen under unusual circumstances on valid setups.

[Diggsey]

@alexcrichton OK, I've reverted 1 & 4, at least for this PR.
For 1) I've replaced it with a check, so instead of trying to be clever it just warns early on that the version of cmake is incorrect.
For 4) I've just reverted it until I can reproduce it more reliably: at the moment it seems to be happening randomly, sometimes it just works, sometimes it doesn't and nothing I do seems to fix it.

The changes to rustc_trans::back::msvc will not take full effect until new snapshots are registered.

edit:
Actually, my other changes in this PR seem to have reliably fixed 4) anyway! I think they were caused by CFG_MSVC_INCLUDE_PATH_xxx not including the changes that vcvarsall.at made.

[alexcrichton]

Ah ok, I also prefer to solve (1) with just a check, and I'm ok with not worrying about (4) for now. For (3), however, I'd prefer to keep the check for LIB around because I'd want to make sure that if someone set the variable manually that the compiler doesn't stomp over the custom value set.

[alexcrichton]

I also don't understand the claim that the current scraping of include_path and lib_path don't work. We are compiling C++ code with MSVC (rustllvm.lib) successfully, and it wouldn't work unless include_path was correctly set. If I take a look at config.mk locally as well the variables are definitely as one would expect.

I'd believe the syntax is probably not right today, but I don't believe that it's 100% always wrong (as it's working for both me, others building MSVC, and the bots)

[Diggsey]

it wouldn't work unless include_path was correctly set.

Currently the build will fail unless both LIB and INCLUDE environment variables are unset (empty is not sufficient). If INCLUDE is set, then %INCLUDE% will be expanded by cmd before it even attempts to run vcvarsall.bat.

This happens to be true on the buildbot, and on your machine because you have an essentially bare installation of windows. However, you can't rely on that being the case: frequently software will add paths to LIB or INCLUDE globally when it is installed.

I'd prefer to keep the check for LIB around because I'd want to make sure that if someone set the variable manually that the compiler doesn't stomp over the custom value set.

But that's exactly what breaks things. LIB can't be expected to be unset - the proper way to check if someone is overriding which MSVC version to use, is to check whether they've set VisualStudioVersion. I'm not exactly sure what usecase you're trying to cater for by leaving things as they are?

[alexcrichton]

Ah ok, that makes sense that a previously set LIB or INCLUDE would break things. I had no idea these were so frequently appended to in shells!

For the LIB changes, we currently mirror basically exactly what clang does (in terms of checking environment variables), but the use case I'm thinking of is that I've set up a custom LIB (without booting up the whole set of VS environment variables) to hack around a bit, but this means that my custom LIB will be tromped over by the compiler quickly.

I'd mostly prefer to err on the side of what clang is doing currently given a lack of experience otherwise I think though

[Diggsey]

I'd mostly prefer to err on the side of what clang is doing currently given a lack of experience otherwise I think though

Clang doesn't do this though:
https://github.com/llvm-mirror/clang/blob/6efa1db780530f2d2fb8605c25b5db791a4c0f44/lib/Driver/Tools.cpp#L8858

Even if LIB is set, unless you pass in /NODEFAULTLIB or whatever the clang equivalent is called, it passes -defaultlib:libcmt to link.exe.

I had no idea these were so frequently appended to in shells!

They're not: windows stores environment variables in the registry, both per-user and for the whole system. When you install an SDK, it may automatically add itself to LIB/INCLUDE by modifying the registry. However, visual studio itself doesn't do this, so that you can have multiple versions installed without conflicting: LIB/INCLUDE usually only contain additional directories to search, in addition to the defaults for whatever VS version you are using.

[alexcrichton]

It kinda depends on what you're looking at with clang, we don't add any arguments to the linker line in the compiler by default, and otherwise the logic in the compiler is mirroring what's right below the part you highlighted (if LIB isn't set add a few /LIBPATH arguments). I think the -defaultlib argument is somewhat orthogonal to adding these linker paths in the compiler itself

[Diggsey]

I think the -defaultlib argument is somewhat orthogonal to adding these linker paths in the compiler itself.

That may be, but the net result is that clang "just works", and rust doesn't. What's worse is that when it doesn't work it's very hard to track down what's really the problem, and even when you do, to fix it properly means breaking the configuration of other software you might have installed.

[alexcrichton]

Could you elaborate on how clang works and we don't? We do the exact same thing on if LIB isn't set we add a few /LIBPATH arguments, and otherwise we both do nothing.

[Diggsey]

@alexcrichton Oh, I thought -defaultlib affected the directory search path too (the similarly named option within Visual Studio itself does affect it, but I guess that's the IDE doing magic). In that case, I have no idea how clang is working, and I really CBA figuring that out, so I'll just leave it as "LIB" for now.

Also, problem 4 seems to have reappeared for me, with no apparant reason...

[alexcrichton]

Hm, definitely sounds like something weird is going on... I definitely think we want to get our builds working in as many environments as possible, but printing a nicer error message in some circumstances may just be the best way to go, so if something like the cmake detection we have doesn't cut it I think it's fine to add a few more checks!

@bors: r+ f86c853

[bors]

⌛ Testing commit f86c853 with merge 668dac4...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt