Install GSL.natvis #1112
Merged
Install GSL.natvis #1112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpr42
commented
May 10, 2023
| @@ -19,8 +17,7 @@ target_compile_features(GSL INTERFACE "cxx_std_14") | |||
| # Setup include directory | |||
| add_subdirectory(include) | |||
|
|
|||
| # Add natvis file | |||
| gsl_add_native_visualizer_support() | |||
| target_sources(GSL INTERFACE $<BUILD_INTERFACE:${GSL_SOURCE_DIR}/GSL.natvis>) | |||
There was a problem hiding this comment.
This does nothing on all platforms except for when generating a visual studio project.
In which case you get the .natvis experience that doesn't affect the PDB.
jpr42
commented
May 10, 2023
| @@ -46,4 +43,6 @@ if (GSL_INSTALL) | |||
| write_basic_package_version_file(${gls_config_version} COMPATIBILITY SameMajorVersion ARCH_INDEPENDENT) | |||
|
|
|||
| install(FILES ${gls_config_version} DESTINATION ${cmake_files_install_dir}) | |||
|
|
|||
| install(FILES GSL.natvis DESTINATION ${cmake_files_install_dir}) | |||
There was a problem hiding this comment.
Installing the file at least gives find_package users a chance to decide what they want to do. Versus nothing which is what they currently get.
jpr42
commented
May 10, 2023
| endif() | ||
|
|
||
| # add natvis file to the library so it will automatically be loaded into Visual Studio | ||
| if(GSL_VS_ADD_NATIVE_VISUALIZERS) |
There was a problem hiding this comment.
This if statement isn't really needed. Just complicates the code IMO. I don't see a reason to keep it.
|
@jpr42 Thanks a lot for looking into this! Unfortunate that natvis support is so limited, but the cleanup you've done to work around it looks good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Turns out supporting GSL.natvis perfectly is quite difficult.
There is no universal way to consume .natvis files that would satisfy everyone.
I thought the solution was to use the /NATVIS linker option. But it turns out that actually embeds information into the PDB. So I'm not sure how to properly support the Ninja generator...
That's not even accounting for the fact target_link_options doesn't play nicely with /NATVIS when installing.
When you just add the file via target_sources the visual studio solution will just pick it up and recognize it without adding it to the PDB file. Which won't affect the binary and is what most devs want.
This all comes down to the fact that /NATVIS files have native integration with visual studio that makes it difficult to use with non-visual studio solutions. /NATVIS almost works but embeds itself into the PDB which not everyone wants, and not everyone generates PDBs either.
Docs for natvis files and /NATVIS
So my current solution is to just simplify the existing CMake code, and install the natvis so the user can decide.
closes #1084