Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block building STL in wrong command prompt #4709

Merged
merged 5 commits into from
Jun 18, 2024

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jun 3, 2024

Fixes #4639

I didn't test ARM command prompts.

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

@fsb4000 fsb4000 requested a review from a team as a code owner June 3, 2024 15:18
@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 3, 2024

Our test suite doesn't use CMake Presets.
So manual testing is needed...
CI runs CMake like this:

cmake -G Ninja ^
-DCMAKE_CXX_COMPILER=cl ^
-DCMAKE_BUILD_TYPE=Release ^
-DLIT_FLAGS=${{ join(';', parameters.litFlags) }} ^
-DSTL_USE_ANALYZE=${{ parameters.analyzeBuild }} ^
-DSTL_ASAN_BUILD=${{ parameters.asanBuild }} ^
-DTESTS_BUILD_ONLY=${{ parameters.testsBuildOnly }} ^
-S $(Build.SourcesDirectory) -B "$(buildOutputLocation)"

@cpplearner
Copy link
Contributor

Can you also test building with the Visual Studio IDE?

@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 3, 2024

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
Copy link
Contributor

cpplearner commented Jun 3, 2024

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 fsb4000 marked this pull request as draft June 4, 2024 01:13
@fsb4000 fsb4000 marked this pull request as ready for review June 5, 2024 14:50
@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 5, 2024

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

@StephanTLavavej StephanTLavavej self-assigned this Jun 5, 2024
@AlexGuteniev
Copy link
Contributor

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
Copy link
Contributor Author

fsb4000 commented Jun 6, 2024

@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.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jun 6, 2024
@fsb4000 fsb4000 changed the title Block building in wrong command prompt Block building STL in wrong command prompt Jun 8, 2024
@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 9, 2024

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

@StephanTLavavej
Copy link
Member

Thanks, this is awesome!! 😻 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 😹).

@StephanTLavavej StephanTLavavej removed their assignment Jun 13, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 15af446 into microsoft:main Jun 18, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this quality-of-life improvement for contributors! 🪄 📈 😻

@fsb4000 fsb4000 deleted the fix4639 branch June 18, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Warn/block building in wrong command prompt
4 participants