Skip to content
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

Added check for CMAKE_SKIP_INSTALL_RULES #160

Merged
merged 1 commit into from Sep 20, 2022
Merged

Added check for CMAKE_SKIP_INSTALL_RULES #160

merged 1 commit into from Sep 20, 2022

Conversation

EvanMcBroom
Copy link
Contributor

CMake's FetchContent module allows people to include your library in their project directly without using an external library manager (ex. vcpkg or conan). Here is an example for doing that:

include(FetchContent)
FetchContent_Declare(
  cli
  GIT_REPOSITORY https://github.com/daniele77/cli.git
  GIT_TAG v2.0.2
)
FetchContent_MakeAvailable(cli)

add_executable(main-project)
target_link_libraries(main-project PRIVATE cli::cli)

The only issue with using FetchContent is that the cli library will attempt to add install steps regardless of it is the main project or not. CMake allows you to disable the install steps by temporarily setting the CMAKE_SKIP_INSTALL_RULES variable to ON, but that makes CMake generate a nasty warning message:

CMake Warning in CMakeLists.txt:
  CMAKE_SKIP_INSTALL_RULES was enabled even though installation rules have
  been specified

This is a known issue with CMake (issue #22561) which does not appear like it will be fixed anytime soon. The current fix for FetchContent users is to guard a library's install steps with a check for CMAKE_SKIP_INSTALL_RULES:

if(NOT CMAKE_SKIP_INSTALL_RULES)
  install(...)
endif()

This pull request adds that check which allows the library to be integrated using FetchContent without having additional warning messages appear.

@daniele77
Copy link
Owner

daniele77 commented Sep 20, 2022

Great contribution, thank you very much.

@daniele77 daniele77 merged commit 10c570d into daniele77:master Sep 20, 2022
@EvanMcBroom
Copy link
Contributor Author

Thanks @daniele77! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants