msvc tool issues

[digit-google]

The msvc tool has minor problems in the current code source. This issue is to track them for future fixes:

• The tool is only defined with #ifdef _MSVC, i.e. when compiling with the Microsoft compiler, this should be #ifdef _WIN32 instead.

• It is also tagged (EXPERIMENTAL) in the ninja.cc, but the Chromium build relies on it.

• The tool is not documented in doc/manual.asciidoc

• The format of the input file for the -e ENVFILE option is a binary block passed directly to the CreateProcessA() Win32 function directly, which means it cannot support long file paths. Converting the paths to UTF-16 then using CREATE_UNICODE_ENVIRONMENT in dwCreationFlags would fix this.

• Similarly, CreateProcessW() should be called instead of CreateProcessA(), even if the command-line is currently only available as an 8-bit string, this will simplify future refactors to properly support Windows Unicode file paths.

• The tool modifies the global PATH environment variable, and never resets it to its previous value (see src/msvc_helper_main-win32.cc). It seems it uses it to ensure that CreateProcessA() finds the program using the PATH definition found in the ENVFILE, but a better way to address this would be to do an explicit search With SearchPathW() instead. This is probably minor until Ninja is turned into a library.

• The tool also modifies the mode of stdout to O_BINARY without resetting it. Also minor until Ninja is turned into a library.

[digit-google]

Just to clarify a few things here, the modification of global PATH and stdout mode isn't a practical problem now since this code path is only run as part of a tool. If Ninja is refactored as a library in the future, that tool is likely never going to be part of it.

On the other hand, this tool is typically used in commands invoked by Ninja itself, e.g.:

rule cc
   command = ninja -t msvc -e ENVFILE -- cl ....
   
build foo.o: cc foo.c foo.h

Since launching a new process on Windows is a costly operation, it might be useful to have Ninja auto-detect this use case and implement the tool directly in the context of its own process (saving an intermediate process creation). This has the potential to speed up every CL invocation. In that specific case, having something that does not change the global state would be required, but that looks like an advanced feature, so let's consider this is not really part of issues with the code for now.

[jhasse]

[...] but the Chromium build relies on it.

Aren't you using clang-cl?

[digit-google]

I am not on the Chromium team, but from their build configuration files at [1], one can see that they support building with both cl and clang-cl. And they actually define both a pure MSVC toolochain, and a "win_clang_xxxx" variant here [2]. This is only used for Windows builds.

On the other hand, I use clang-cl to build Ninja and GN for Windows on Linux, but this is just a way to verify locally that compilation and unit-tests work correctly without a Windows machine or VM.

[1] https://source.chromium.org/chromium/chromium/src/+/main:build/toolchain/win/BUILD.gn;drc=364c9f71522ca2e7ce2fb1f155cd558bf480a8d5;l=138

[2] https://source.chromium.org/chromium/chromium/src/+/main:build/toolchain/win/BUILD.gn;drc=364c9f71522ca2e7ce2fb1f155cd558bf480a8d5;l=523

[jhasse]

I think that's dead code as later in the build Chromium will fail when using MSVC. Could be mistaken though. Either way: Chromium is using Ninja 1.8 anyway.

CMake doesn't use the msvc tool either IIRC. So I think it's okay to deprecate it.

[digit-google]

I'm ok with deprecating it, but please be aware that @nico had a different take on the ninja-build mailing list [1].

What makes you think that the build will fail later using MSVC? Just curious. Also I am in contact with the Chromium team, and I know that they would like to use a newer version of Ninja as well, I don't know what is blocking them exactly though, I can reach to them if you want so they can comment here.

[1] https://groups.google.com/g/ninja-build/c/os1tUBfc4Pw/m/SF9qPHyKAgAJ

[digit-google]

Also consider that Chromium is now the base for Microsoft's Edge, and I would be pretty surprised if they didn't build it with their own compiler

[jhasse]

I'm ok with deprecating it, but please be aware that @nico had a different take on the ninja-build mailing list [1].

Good point. I also didn't mean to remove the tool, just replace (EXPERIMENTAL) with (DEPRECATED). But that can also wait, maybe until a replacement is there (i.e. #1806 is fixed).

What makes you think that the build will fail later using MSVC?

One of the reasons (according to public blog posts) for the switch was to use the same compiler on all platforms and adopt new C++ language feature faster without having to worry about the lowest common denominator of Clang/GCC/MSVC. I would be surprised if they didn't actually make use of that.

[...] I know that they would like to use a newer version of Ninja as well, I don't know what is blocking them exactly though, I can reach to them if you want so they can comment here.

Last information I have is that they need to port all of their build code from Python 2 to Python 3 because of the introduction of high-resolution timestamps in Ninja 1.9. See #1554.

Also consider that Chromium is now the base for Microsoft's Edge, and I would be pretty surprised if they didn't build it with their own compiler

MSVC will die and Microsoft will switch to Clang for all of their C++ needs. Mark my words.

[digit-google]

Good point. I also didn't mean to remove the tool, just replace (EXPERIMENTAL) with (DEPRECATED). But that can also wait, maybe until a replacement is there (i.e. #1806 is fixed).

Oh, in this case this is already done in #2087 . I also clarified the documentation to show what to use as a replacement for the -o and -p options.

One of the reasons (according to public blog posts) for the switch was to use the same compiler on all platforms and adopt new C++ language feature faster without having to worry about the lowest common denominator of Clang/GCC/MSVC. I would be surprised if they didn't actually make use of that.

Yes, the public documentation as of 2022-02 states that clang-cl is used to build even if Visual Studio is required for the SDK, debugging, etc. I'll try to see if I can find more info from the Chrome team.

Last information I have is that they need to port all of their build code from Python 2 to Python 3 because of the introduction of high-resolution timestamps in Ninja 1.9. See #1554.

Thanks for the link. I am aware of the Python2 > Python3 migration, but I am not sure that this is the only thing blocking the Ninja upgrade. Again, will try to find more info.

MSVC will die and Microsoft will switch to Clang for all of their C++ needs. Mark my words.
Oohh, interesting :-)