microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
9.88k stars 1.45k forks source link

Block building STL in wrong command prompt #4709

Closed fsb4000 closed 1 week ago

fsb4000 commented 3 weeks ago

Fixes #4639

I didn't test ARM command prompts.

изображение изображение изображение изображение

fsb4000 commented 3 weeks ago

Our test suite doesn't use CMake Presets. So manual testing is needed... CI runs CMake like this: https://github.com/microsoft/STL/blob/e36ee6c2b9bc6f5b1f70776c18cf5d3a93a69798/azure-devops/cmake-configure-build.yml#L32-L39

cpplearner commented 3 weeks ago

Can you also test building with the Visual Studio IDE?

fsb4000 commented 3 weeks ago

Yes, it works. изображение изображение

It seems VS Studio also doesn't use CMake Presets directly. VS IDE might parse the CMake Presets but it runs cmake like this:

Command line: "C:\WINDOWS\system32\cmd.exe" /c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "c:\program files\microsoft visual studio\2022\preview\common7\ide\commonextensions\microsoft\cmake\CMake\bin\cmake.exe"  -G "Ninja"  -DCMAKE_C_COMPILER:STRING="cl.exe" -DCMAKE_CXX_COMPILER:STRING="cl.exe" -DCMAKE_BUILD_TYPE:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:/Dev/STL/out/install/x86-debug"   -DCMAKE_MAKE_PROGRAM="c:\program files\microsoft visual studio\2022\preview\common7\ide\commonextensions\microsoft\cmake\Ninja\ninja.exe" "C:\Dev\STL" 2>&1"
1> Working directory: C:/Dev/STL/out/build/x86-debug
1> [CMake] -- The CXX compiler identification is MSVC 19.41.33901.0
1> [CMake] -- Detecting CXX compiler ABI info
1> [CMake] -- Detecting CXX compiler ABI info - done
1> [CMake] -- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.41.33901/bin/Hostx64/x86/cl.exe - skipped
1> [CMake] -- Detecting CXX compile features
1> [CMake] -- Detecting CXX compile features - done
1> [CMake] -- Performing Test WINDOWS_SDK_VERSION_CHECK
1> [CMake] -- Performing Test WINDOWS_SDK_VERSION_CHECK - Success
1> [CMake] -- The ASM_MASM compiler identification is MSVC
1> [CMake] -- Found assembler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.41.33901/bin/Hostx64/x86/ml.exe
1> [CMake] -- Searching for VS clang-format
1> [CMake] -- Searching for VS clang-format - found
1> [CMake] -- Found Python: C:/Python312/python.exe (found suitable version "3.12.0", minimum required is "3.12") found components: Interpreter 
1> [CMake] -- Boost.Math: standalone mode ON
1> [CMake] -- Configuring done (4.3s)
1> [CMake] -- Generating done (1.0s)
1> [CMake] CMake Warning:
1> [CMake]   Manually-specified variables were not used by the project:
1> [CMake] 
1> [CMake]     CMAKE_C_COMPILER
cpplearner commented 3 weeks ago

This doesn't match the presets. upload Without this patch, it does. upload

I believe that with this patch, Visual Studio sees all presets as disabled, because VS doesn't set $env{Platform}, and thus it falls back to the predefined configurations.

fsb4000 commented 3 weeks ago

изображение изображение изображение

AlexGuteniev commented 3 weeks ago

After looking into this deeper I'm not sure if it is even the right direction. People were complaining about tests not building in a wrong command prompt, not the STL itself. I'm not even sure that you can't build the STL itself this way.

fsb4000 commented 3 weeks ago

@AlexGuteniev Yes, I know. But: 1) For tests, we need to modify stl-lit.py, not CMake. 2) You can build the wrong preset in the wrong command prompt; I tested it. How it works, for example: You are in an x64 command prompt and you build an x86 preset. It finishes without error. It creates the out/x86 folder, but the libs would be x64. Later, when you open an x86 command prompt and run tests from the out/x86 folder, you will get an error about a mismatch in architecture.

So I think my patch can help with these issues, partly. But we also can make a patch for stl-lit.py.

fsb4000 commented 2 weeks ago

изображение изображение

StephanTLavavej commented 2 weeks ago

Thanks, this is awesome!! :heart_eyes_cat: I pushed a commit to simplify the logic with CMake's "inList" condition, see https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html#condition .

(They also support regex "matches", but (1) their regex syntax has weirdness and (2) while it would save a few lines in the JSON, it would look more complicated than the list of ["meow", ""], so I chose the simpler option.)

I re-verified enabled and disabled cases in the terminal, and that the VS IDE works (actually the first time I've built the STL in the IDE :joy_cat:).

StephanTLavavej commented 2 weeks ago

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej commented 1 week ago

Thanks for this quality-of-life improvement for contributors! :magic_wand: :chart_with_upwards_trend: :heart_eyes_cat: