-
Notifications
You must be signed in to change notification settings - Fork 761
cmake: surface headers in Visual Studio solution #1222
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi I tried cloning you repository and doing an out of tree build using CMake 4.1.2 where the I read a few things on the message and they were talking about adding header using I tried changing the |
|
@davidhunter22 Hey! Yeah it is due to the later versions of CMake past 4.1.x. Try again and let me know if you still get any errors. |
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.
Pull Request Overview
This PR enhances Visual Studio integration by making GSL headers and project metadata visible in generated Visual Studio solutions. The changes improve developer experience by providing proper file organization and discoverability within the IDE.
- Explicitly lists all GSL headers in the CMake target to surface them in Visual Studio
- Organizes headers under a "Header Files" group in the solution explorer
- Adds key project documents as solution items for easy access
|
Looking at the changes I was wondering if there was a simpler way. I also wondered if the header files could be added to the |
carsonRadtke
left a comment
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 the PR! The pipeline looks good, but can you address these couple comments?
CMakeLists.txt
Outdated
| foreach(header ${GSL_HEADER_FILES}) | ||
| list(APPEND GSL_HEADER_ABSOLUTE_PATHS ${GSL_SOURCE_DIR}/include/${header}) | ||
| endforeach() | ||
| source_group(TREE ${GSL_SOURCE_DIR}/include PREFIX "Header Files" FILES ${GSL_HEADER_ABSOLUTE_PATHS}) |
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.
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 added a source_group call, the docs should land in the solution items node now.
carsonRadtke
left a comment
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.
Unfortunately, this approach requires features from CMake 4.0.0. Right now, we can't force that on our customers, so we will need a different approach. I have outlined my suggestion in one of the comments but would also like to hear of any alternatives that exist.
CMakeLists.txt
Outdated
| RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}/include" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/include/gsl/*" | ||
| ) | ||
| list(SORT GSL_HEADER_FILES) |
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: According to the docs, the result of file(GLOB, ... is already sorted.
https://cmake.org/cmake/help/latest/command/file.html#filesystem
CMakeLists.txt
Outdated
| list(SORT GSL_HEADER_FILES) | ||
|
|
||
| # Build/install interface lists ensure CMake is happy when exporting headers via | ||
| # INTERFACE sources. Absolute paths would be rejected by newer CMake versions |
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: "Newer" may be true now but won't be true in the future. Any chance you know which version of CMake starts throwing the error?
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 believe it's 4.1.2, below that didn't throw for me.
CMakeLists.txt
Outdated
| ${GSL_SOURCE_DIR}/ThirdPartyNotices.txt | ||
| ) | ||
|
|
||
| set_property(DIRECTORY PROPERTY VS_SOLUTION_ITEMS ${GSL_SOLUTION_FILES}) |
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.
issue: The VS_SOLUTION_ITEMS property is new in CMake 4.0.0. At this time, we want to maintain backwards compatibility with older CMakes. Apologies for not catching this the first time around, but we will need to figure out something else.
From what I understand, before 4.0.0 there was no way of adding arbitrary files to VS solutions. What you'll need to do is create a custom target and add the files in the GSL workspace to that target. Something like:
if (CMAKE_GENERATOR MATCHES "Visual Studio")
file(GLOB GSL_WORKSPACE_FILES "${CMAKE_CURRENT_SOURCE_DIR}/include/gsl/*")
list(APPEND GSL_WORKSPACE_FILES
${GSL_SOURCE_DIR}/.clang-format
${GSL_SOURCE_DIR}/.gitattributes
${GSL_SOURCE_DIR}/.gitignore
${GSL_SOURCE_DIR}/CMakeSettings.json
${GSL_SOURCE_DIR}/CONTRIBUTING.md
${GSL_SOURCE_DIR}/LICENSE
${GSL_SOURCE_DIR}/README.md
${GSL_SOURCE_DIR}/SECURITY.md
${GSL_SOURCE_DIR}/ThirdPartyNotices.txt
)
target_sources(GSL PRIVATE ${GSL_WORKSPACE_FILES})
endif()
And that should probably be the bulk of the change.
https://cmake.org/cmake/help/latest/prop_dir/VS_SOLUTION_ITEMS.html
| endforeach() | ||
|
|
||
| target_sources(GSL INTERFACE | ||
| ${GSL_HEADER_INTERFACE_SOURCES} |
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.
suggestion: According to https://github.com/microsoft/GSL/pull/1112/files#r1190452831 this only affects VS solutions. Let's move it into your new if (CMAKE_GENERATOR MATCHES "Visual Studio") block and combine it with the other target_sources (which doesn't yet exist; see my comment about VS_SOLUTION_ITEMS).
|
@carsonRadtke No worries! I'll do some more research to see if there's a better solution or may end up going with your suggestion. I'll update the PR. |
|
On using the |
@davidhunter22 -- I see where you're coming from, but this is effectively a feature implementation for GSL and it should work for all customers (in this case, the customers are y'all: the contributors) who meet the dependency requirements. If there was no possible way for this to work pre-4.0.0, I would be more open to a |
|
@cnaples79 -- Do you need any assistance with this? It is close, but I'm still waiting on a few comments to be resolved before merging! |

Summary
Rationale
Developers opening the generated Visual Studio solution couldn't discover headers or project metadata without manual include navigation. Adding the files directly to the solution keeps intellisense and browsing consistent with typical VS layout (matching the reporter's expectation).
Changes
Fixes #1216