Skip to content

CMakeLists.txt Use CMakePresets instead of CMakeSettings - #3730

Merged
Stephan T. Lavavej (StephanTLavavej) merged 19 commits into
microsoft:mainfrom
duncanspumpkin:cmakePresets
Aug 3, 2023
Merged

CMakeLists.txt Use CMakePresets instead of CMakeSettings#3730
Stephan T. Lavavej (StephanTLavavej) merged 19 commits into
microsoft:mainfrom
duncanspumpkin:cmakePresets

Conversation

@duncanspumpkin

Copy link
Copy Markdown
Contributor

CMakeSettings is the older style for setting up CMake and standard CMake is encouraged to use CMakePresets instead. I've not modified the CI scripts yet but they could now be changed to use the new presets. I've verified x86 and x64 building on my system through VS (I've not got ARM build tools installed). I updated the readme to direct towards the presets but you could also continue using the old style. You don't actually need to run from the Native Tools Command Prompt just from a regular Command Prompt will now work.

@duncanspumpkin
Duncan (duncanspumpkin) requested a review from a team as a code owner May 23, 2023 21:26
Comment thread CMakePresets.json Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jun 1, 2023

Copy link
Copy Markdown
Member

Thanks, this is a really nice improvement! I pushed the following changes:

  • Consolidate the Ninja setting into the base preset.
  • Drop installDir - we never used it. (We never actually "install" the STL.)
  • Simplify binaryDir; we now prefer to omit build from the path.
  • Drop -v from building; it appeared in CMakeSettings.json but it's incredibly obnoxious and the README's usual instructions never used it.
  • Set TESTS_BUILD_ONLY for ARM/ARM64; this is needed for test runs to pass (at least for right now, when we always build on an x64 machine).
  • README.md: Add missing backtick.
  • README.md: out\build\x64 => out\x64 to reflect the simplified binaryDir.
  • README.md: Update build incantations in the benchmarks section.
  • Pre-existing: Fix benchmark dir inconsistency (taken from Michael Schellenberger Costa (@miscco)'s [vector.bool] Optimize copy / move algorithms #3353).

I've manually tested configure/build/test on the 4 architectures, and building the clang-format target (cmake --build --preset x64 --target format).

There is potential future work to make a "benchmark" target, so we can simplify those configure/build commands, but that would be entirely separate and best done in a followup PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks VSCode's CMakeTools addon.

CMakeTools has "odd" requirements when it comes to how the archtecture and toolset items are specified, and you also need to set CMAKE_CXX_COMPILER to cl.exe to get it to add visual studio to the search path before running cmake.

I'll try and submit suggestions once I get things working on both VS and VSCode.

@barcharcraz

Copy link
Copy Markdown
Contributor

In particular both the arch and toolset items must be specified in each configure preset (you can't consolidate them into a parent) and they must be literals, they can't contain any substitutions. Also VS and VSCode seem to have somewhat different conventions for how the toolset value is interpreted, I'm trying to find a way that'll work in both.

@barcharcraz

Copy link
Copy Markdown
Contributor

Yeah I can't get this to work in my copy of Visual Studio at all, either the latest 17.7 preview 1, or the internal dogfooding version. It seems that developer command prompt environment variables are around for the configure step but don't get set for the build step, so the compiler can't find any of the usual include files.

When you tested this out did you launch visual studio from a developer command prompt?

@cpplearner

Copy link
Copy Markdown
Contributor

Yeah I can't get this to work in my copy of Visual Studio at all, either the latest 17.7 preview 1, or the internal dogfooding version. It seems that developer command prompt environment variables are around for the configure step but don't get set for the build step, so the compiler can't find any of the usual include files.

I can't get this to build either. I found DevCom-10365917 which seems to have the same symptom. According to DevCom-10365917, it's a regression in 17.7.0 Preview 1.0.

@duncanspumpkin

Copy link
Copy Markdown
Contributor Author
  • Upgrade to version 6 of the schema, even though we don't need it yet.
  • Consolidate architecture into the base preset.

I had designed it for supporting being able to be run on 17.6 (although i know the readme states that you should have 17.7 preview). Consolidating the architecture into the base doesn't work on 17.6 and it doesn't support version 6 of the presets. I've also now tested this on vscode and can confirm encountering the issues with building as above.

@barcharcraz

Copy link
Copy Markdown
Contributor

Yeah I can't get this to work in my copy of Visual Studio at all, either the latest 17.7 preview 1, or the internal dogfooding version. It seems that developer command prompt environment variables are around for the configure step but don't get set for the build step, so the compiler can't find any of the usual include files.

I can't get this to build either. I found DevCom-10365917 which seems to have the same symptom. According to DevCom-10365917, it's a regression in 17.7.0 Preview 1.0.

This bug is classified as high priority internally, hopefully it'll be fixed fairly rapidly.

@StephanTLavavej

Copy link
Copy Markdown
Member

I've pushed changes to go back to version 5 and un-consolidate the architecture.

Duncan (@duncanspumpkin)

I had designed it for supporting being able to be run on 17.6 (although i know the readme states that you should have 17.7 preview).

Note that we can't ever support using anything older than the latest Preview because we continually need the latest compiler features and fixes. Right now we don't have a hard #error for 17.6 because the MSVC-internal build is still starting with a compiler that identifies itself as 17.6, but that will change soon.

Charlie Barto (@barcharcraz)

This bug is classified as high priority internally, hopefully it'll be fixed fairly rapidly.

As DevCom-10365917 / internal VSO-1822085 is being prioritized for 17.7 Preview 3 (no promises!), I'll move this PR to the blocked state until then.

@StephanTLavavej

Copy link
Copy Markdown
Member

A fix has been merged for 17.7 Preview 3.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) removed the blocked Something is preventing work on this label Jul 14, 2023
@StephanTLavavej

Copy link
Copy Markdown
Member

Charlie Barto (@barcharcraz) will take a look at this next week and validate that VSCode's CMakeTools is happy with this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't work with vscode, no matter what config you select it will always use the x64 tools, or whatever else it can find on the path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in the maintainer meeting and given that vscode cmake tools is pretty broken, and that this offers advantages for command line compilation, we're going to accept and merge this.

Improvements can be made later as well

@StephanTLavavej

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit efcf433 into microsoft:main Aug 3, 2023
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for simplifying the repo building experience, and congratulations on your first microsoft/STL commit! ⚙️ 🎉 😻

@duncanspumpkin

Copy link
Copy Markdown
Contributor Author

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Related to the build system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants