-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[docs][CMake] Add CMakePreset, improve VS documentation #84760
base: main
Are you sure you want to change the base?
Conversation
I personally don't get on with the built-in CMake support within Visual Studio. I've got no issue with documenting how to use it or adding "sensible" defaults (I have a suspicion that there is no specific "sensible" default for any given user), but I would prefer us avoiding stating that one way is preferred above another way, especially without an RFC discussion to formalise this stance: let each user decide for themselves which method they'd prefer to use. |
Another thought: since presumably no build bot will be configured to use the files you're adding, what's stopping them rotting? |
That's fine, I can change the documentation to make it clear it's a choice and without us pushing people either way.
Good point, but I don't think this will be a huge issue since I tried to use very common options. But if this is a problem I can probably do a github workflow for it down the line. |
This commit adds the following: * .vs/CMakeWorkspaceSettings.json - This file make Visual Studio find the llvm source directory out of the box with no configuration. And enables CMake for this project. * Adds llvm/CMakePresets.json - Some sane defaults to make VS be able to configure and build LLVM/Clang without any changes. At this time it only adds Windows targets, but can be expanded for other platforms. * Update the documentation for Getting started on Windows. This points out that the Visual Studio native CMake configuration is a better choice for most people.
dd05c93
to
7cea65b
Compare
@jh7370 I reworded and removed the recommendations, are you good with this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is better to me.
llvm/docs/GettingStartedVS.rst
Outdated
recommended to generate a Visual Studio solution from CMake instead of using the | ||
native CMake integration. | ||
|
||
If you want to generate a Visual Studio Solution for older versions of Visual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are "older versions" here meant to mean VS2017 and older? If so, I thought VS2019 was the oldest supported currently? If it instead means VS2019, I think there's a bit of redundancy between this and the previous paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded this.
documentation | ||
<https://learn.microsoft.com/en-us/cpp/build/configure-cmake-debugging-sessions>_. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: double blank line
llvm/docs/GettingStartedVS.rst
Outdated
Generate Visual Studio Solution with CMake | ||
========================================== | ||
|
||
1. After following the steps above you can generate a Visual Studio Solution by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I find the "above" phrasing here a little ambiguous. Obviously from reading with the full context, this is referring to the points which end with getting the LLVM source, but if you jumped straight here, you might think "above" means the immediately previous section, i.e. using CMake within Visual Studio. Perhaps referring more specifically to the section might be best for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added direct links from both sections to the getting started section.
Should we explicitly say this isn't for VS Code? I know more people are developing llvm with it now, but I don't think we currently have our docs for it (although there are redhat/linaro blogs on the subject) - I just worry beginners might get confused. |
I want to document VSCode next! |
Co-authored-by: James Henderson <46713263+jh7370@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, no more comments from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change! I was able to use it to generate and build with visual studio.
I had a question about enabling asserts for the release build, but otherwise looks good. It was nice to see the "LLVM_OPTIMIZED_TABLEGEN": "ON"
which makes a big difference when building with msvc and is nice to have that be the default for people that might not be aware of it.
"cacheVariables": { | ||
"CMAKE_BUILD_TYPE": "Release", | ||
"LLVM_ENABLE_PDB": "ON", | ||
"LLVM_ENABLE_ASSERTIONS": "ON" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are asserts enabled for the release build? It doesn't look that is the case for the clang release config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the improvements here, it's really appreciated! Note, as a long-time user of this functionality, there is actually one thing that drives me bonkers over all other usability issues: because of the way the monorepo is set up, CMake support within Visual Studio is excellent for LLVM and terrible for every other project. For example, "go to definition", the solution explorer, etc are all focused solely on the llvm
folder and have no knowledge of the clang
, et al folders in the monorepo. The IDE experience is very much hampered by this, and anything we can do (either in terms of manual interventions, of which I've never found one, or in terms of presets, layout changes, etc) to improve this would be a huge productivity boost. It's the one feature I miss on a daily basis from using CMake to generate a solution file.
@@ -72,7 +73,8 @@ These instruction were tested with Visual Studio 2019 and Python 3.9.6: | |||
1. Download and install `Visual Studio <https://visualstudio.microsoft.com/>`_. | |||
2. In the Visual Studio installer, Workloads tab, select the | |||
**Desktop development with C++** workload. Under Individual components tab, | |||
select **Git for Windows**. | |||
select **Git for Windows**. You might want to install **C++ Clang compiler for Windows** | |||
as well, since this will allow you to build LLVM with Clang. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well, since this will allow you to build LLVM with Clang. | |
as well, since this will allow you to build LLVM with Clang. |
Removes trailing whitespace.
target, you need to have the C++ Clang compiler for Windows component of Visual | ||
Studio installed. | ||
|
||
To debug Clang and other binaries within Visual Studio, see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To debug Clang and other binaries within Visual Studio, see | |
To debug Clang and other binaries within Visual Studio, see |
Removes trailing whitespace.
LLVM ships with a default CMakePresets.json that is supposed to make it easy to | ||
get started, but usually you need to modify the settings used. The best way to | ||
do this is to create a CMakeUserPresets.json in the llvm subfolder and add your | ||
configuration there. For more information about the Preset system, see | ||
`Microsoft's documentation <https://learn.microsoft.com/en-us/cpp/build/cmake-presets-json-reference>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this -- I've been using integrated CMake in Visual Studio for a few years now and I've never needed to do this. Rather, I've been going to Project->CMake Settings for LLVM
which allows me to set the state of various CMake variables. It looks like this:
However, that menu is gone entirely with CMakePresets.json present and the replacement (Project->Edit CMake Presets for LLVM) is less usable. It looks like this:
There's a few concerns here:
- This file silently overrides all of my previous settings which is going to be really surprising to folks who are already using Visual Studio's cmake integration. What I get now is:
Command line: "C:\WINDOWS\system32\cmd.exe" /c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\2022\PROFESSIONAL\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe" -G "Ninja" -DLLVM_ENABLE_PROJECTS:STRING="clang;lld;clang-tools-extra" -DCMAKE_BUILD_TYPE:STRING="Debug" -DCMAKE_CXX_COMPILER:STRING="clang-cl" -DCMAKE_C_COMPILER:STRING="clang-cl" -DLLVM_OPTIMIZED_TABLEGEN:STRING="ON" -DCMAKE_MAKE_PROGRAM="C:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\2022\PROFESSIONAL\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\Ninja\ninja.exe" "F:\source\llvm-project\llvm" 2>&1"
So it's using a different compiler, building different projects, and different settings. :-(
- Editing a JSON file is less intuitive than editing from a dialog box, so I think this is a bit of a regression in terms of usability. For example, there's no longer any discoverability of our cmake variables like there was previously:
- The default settings are missing some common, important things like number of compile and link jobs to use, enabling IDE support, and (perhaps) exporting compile commands.
Given that users are almost certainly going to have to edit the default settings whether we go with a presets file or not, I think we should drop the presets file so that users can use the more discoverable interface, and instead, we can document how to use the non-presets approach. That will have the least impact on our existing developers, but I also think it gives a better user experience to newcomers because it's a more gentle approach than "edit this JSON file". WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, this is surprising to me. Note that I haven't really used Visual Studio, but use Visual Studio Code, I was just asked by my team to improve this for the VS users since it was a little bit of a chore.
When I read up on the CMakeSettings.json vs CMakePresets.json, it read to me like CMakeSettings.json was the Microsoft only solution that is only respected in Visual Studio and that upstream CMake added CMakePresets.json to do the same thing, presets also work with the command line cmake --preset <name>
and they work in Visual Studio Code. So I thought it was a better way forward.
But if the mere presence of the file removes a whole UI bit in Visual Studio that others expect, that seems like a regression for sure (and also a bit dumb?).
I guess the goal I had here was that I got a lot of questions from devs that when they opened the default settings "clang wasn't enabled" and it was not at all obvious to them how to enable it. That's why I thought a default set of settings to build clang was an upgrade to that.
All in all, happy to remove that file if it's supposed to work the way that you described above. But I would love for some more input from someone that understand's the idea behind hiding that UI in VS when using presets think? @dmpots do you know or do you know someone that knows? (sorry I snooped and saw that you where working at Microsoft).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, this is surprising to me. Note that I haven't really used Visual Studio, but use Visual Studio Code, I was just asked by my team to improve this for the VS users since it was a little bit of a chore.
Heh, and I use Visual Studio far more than Visual Studio Code, so it's possible this may just be "different tools work differently" and causing a bit of tension.
When I read up on the CMakeSettings.json vs CMakePresets.json, it read to me like CMakeSettings.json was the Microsoft only solution that is only respected in Visual Studio and that upstream CMake added CMakePresets.json to do the same thing, presets also work with the command line cmake --preset and they work in Visual Studio Code. So I thought it was a better way forward.
It would sure seem like it would be the right way forward!
I guess the goal I had here was that I got a lot of questions from devs that when they opened the default settings "clang wasn't enabled" and it was not at all obvious to them how to enable it. That's why I thought a default set of settings to build clang was an upgrade to that.
I believe that can be done from CMakeSettings.json:
but I perhaps that's not very intuitive (you have to look at "advanced" options and select a specific compiler rather than rely on finding one on PATH).
I'm starting to suspect that whether we stick with CMakeSettings.json
or go with CMakePresets.json
, it might make sense for us to document setup in both Visual Studio and VS Code, with screenshots, to help folks out because these tools are both popular and yet manage to do things differently. WDYT?
All in all, happy to remove that file if it's supposed to work the way that you described above. But I would love for some more input from someone that understand's the idea behind hiding that UI in VS when using presets think? @dmpots do you know or do you know someone that knows? (sorry I snooped and saw that you where working at Microsoft).
+1, I don't feel like I understand enough about how Visual Studio and VS Code CMake support works to feel like I know which way is the best way forward. But I'm more than happy to play test any instructions we want to add to the docs to see how well they work in Visual Studio, help you get screenshots, etc.
I wonder why this is, do you open the root |
I open the |
Try with branch! It adds a important file in |
I don't think I understand the suggestion -- how do I try with branch? |
This commit adds the following:
This works great when installing Visual Studio 2022 community edition and just opening the llvm-project folder, but I would love to hear what other people think about the defaults in the preset and the documentation changes. If you have access to a visual studio install, please try this patch and following the docs.