Reintroduce CMake changes that were reverted in #966#967
Conversation
* [cmake] Adding GSL_INSTALL option Not all consumers of GSL automatically want to have this install logic. It's good practice to gate install logic behind an option. For an example look at magic_enum: https://github.com/Neargye/magic_enum/blob/master/CMakeLists.txt If the client wants to install GSL they still can. But they should ask for it by overriding GSL_INSTALL. * Update cmake/guidelineSupportLibrary.cmake added nl@eof * Update CMakeLists.txt * Update CMakeLists.txt Co-authored-by: Juan Ramos <juanr0911@gmail.com> Co-authored-by: Jordan Maples [MSFT] <49793787+JordanMaples@users.noreply.github.com>
|
Awesome work @JordanMaples! Thanks for fixing the issue 👍😊 |
|
@pr0g Thanks again for letting me know that the previous change broke your CI. |
|
As the thing that caused the issue was a bit odd (the location of the I don't see anything mentioned about where it needs to be defined here https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html |
|
@pr0g + @JordanMaples thanks for fixing this. Apologies I didn't do enough local testing. Glad the new changes still got in. |
|
@hdf89shfdfs I'm a little curious as to what the rationale is to make installing the GSL library a bit more difficult (by moving it behind the I do think a corresponding note in the README/docs would be appropriate too as it might trip up someone trying to install the library. |
|
@pr0g this change is to allow users control of the contents of their install directory. Without this change the user would have to install gsl files into their install folder even if they didn't want to. Which isn't desirable in every circumstance. This "PROJECT"_INTALL is a convention I've seen in many projects for this exact rational. The top level project should be 100% in control of the install directory contents. "I do think a corresponding note in the README/docs would be appropriate too as it might trip up someone trying to install the library." I agree there should be more documentation. |
|
@hdf89shfdfs I'm afraid I don't quite understand "Without this change the user would have to install gsl files into their install folder even if they didn't want to". If a user doesn't run the install command, nothing gets installed right? Is this if you've cloned gsl or are using it via |
|
"Is this if you've cloned gsl or are using it via add_subdirectory, and then run the install command from the top level the sub library will also get installed? I don't usually do this so might not have run into this particular issue before." Yes the use case is adding it via add_subdirectory. Using FetchContent for example. |
|
Ah okay got you, I can see now how this could be a problem with |
Reintroducing the changes from #964 by @hdf89shfdfs that were reverted in #966. @pr0g found that the change unintentionally removed a line from the Microsoft.GSLConfig.cmake file, which broke
FindPackage.I was able to restore the missing line in the config file by moving
include(GNUInstallDirs)back to the main CMakeLists.txt file.