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

Improve the quality of the CMake build scripts #230

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

friendlyanon
Copy link

@friendlyanon friendlyanon commented Nov 8, 2021

This commit does a couple changes:

  • Actually adheres to the declared CMake 3.8 requirement
    • HOMEPAGE_URL in project() was added in 3.12
    • install(TARGETS) requires the DESTINATION argument
    • Tests' CML uses CONFIGURE_DEPENDS from CMake 3.12
  • Removes unnecessary things
    • Looking for RPM and DEB packagers is pointless
    • Misc arguments to CMake commands
  • Improves correctness
    • Includes CPack and CTest modules only if top level
    • Assigns install rules to a project specific component to not clobber
      the global, Unspecified one
    • Adds a warning guard for projects that vendor ctre
    • CMAKE_INSTALL_PREFIX should be set at configure time if the person
      building wants .pc files, so attempt to detect that error
    • Enables the install rules to be opt-out using the CMake built-in
      variable
    • Emulates ARCH_INDEPENDENT for the version config
    • Installs headers to their own directory (fixes Packaging of this lib has multiple issues #207)
    • Use CTest for testing
  • Small optimization
    • LANGUAGES can be set to NONE, because this is a header only library

@friendlyanon
Copy link
Author

friendlyanon commented Nov 11, 2021

Hmm, I thought about the pkgconfig code and it's actually not quite right as it is now either.

The problem with generating a pkgconfig file via CMake is that it does not fit the model of CMake. The prefix variable has to be absolute in the .pc file, however CMake deals with paths relative to the prefix. The final prefix can change at all times, via CMAKE_INSTALL_PREFIX, --prefix parameter during install time, at CPack time and the resulting deb/rpm files can be extracted anywhere as well. If one needs a pkgconfig file for packaging purposes, both deb and rpm CPack generators allow the user to add post-install scripts that can generate it if necessary.

As a clarification: the case where the user is installing with cmake --install or the install target both work fine, but packaging is where things break due to the prefix variable needing to be absolute in the .pc file.

@friendlyanon friendlyanon mentioned this pull request Nov 14, 2021
@alexios-angel
Copy link

It seems that when you run

cmake .
make test

Screenshot from 2021-11-14 18-18-00

@friendlyanon
Copy link
Author

You are correct. The project doesn't use the normal test mechanisms for whatever reason.

file(GLOB test-sources CONFIGURE_DEPENDS *.cpp)
add_custom_target(ctre-test)
foreach (source IN LISTS test-sources)
get_filename_component(test "${source}" NAME_WE)
add_library(ctre-test-${test} STATIC EXCLUDE_FROM_ALL ${source})
target_link_libraries(ctre-test-${test} PRIVATE ctre)
add_dependencies(ctre-test ctre-test-${test})
endforeach()

Note the lack of add_test calls. I think that could be added to this PR as well, since that fits the topic.
I will rebase the first commit and add the appropriate changes there.

@friendlyanon
Copy link
Author

I cleaned up things around the CPack config and pkgconfig.

The CPack config with the project specific defaults is configured to the CPack(Source)Config-ctre.cmake file in the root binary directory. These files include() the CMake generated ones and overwrite some values with set() calls. Using these files can be done with cpack --config ....

The .pc file now uses the configure time CMAKE_INSTALL_PREFIX variable to get its prefix value. This makes it possible to anticipate ahead of time where the generated package files will be installed to. There is a warning emitted during the very first configuration if the pkgconfig option is enabled without CMAKE_INSTALL_PREFIX also being set. If the project is installed with cmake --install ... --prefix ... however, then the .pc file will contain the wrong prefix. This sould probably be documented in #231 cc @angeletakis

@alexios-angel
Copy link

@friendlyanon Will do 💯

This commit does a couple changes:

* Actually adheres to the declared CMake 3.8 requirement
  * HOMEPAGE_URL in project() was added in 3.12
  * install(TARGETS) requires the DESTINATION argument
  * Tests' CML uses CONFIGURE_DEPENDS from CMake 3.12
* Removes unnecessary things
  * Looking for RPM and DEB packagers is pointless
  * Misc arguments to CMake commands
* Improves correctness
  * Includes CPack and CTest modules only if top level
  * Assigns install rules to a project specific component to not clobber
    the global, `Unspecified` one
  * Adds a warning guard for projects that vendor ctre
  * CMAKE_INSTALL_PREFIX should be set at configure time if the person
    building wants .pc files, so attempt to detect that error
  * Enables the install rules to be opt-out using the CMake built-in
    variable
  * Emulates ARCH_INDEPENDENT for the version config
  * Installs headers to their own directory (fixes hanickadot#207)
  * Use CTest for testing
* Small optimization
  * LANGUAGES can be set to NONE, because this is a header only library
The problem with generating a pkgconfig file via CMake is that it does
not fit the model of CMake. The `prefix` variable has to be absolute in
the .pc file, however CMake deals with paths relative to the prefix. The
final prefix can change at all times, via `CMAKE_INSTALL_PREFIX`,
`--prefix` parameter during install time, at CPack time and the
resulting deb/rpm files can be extracted anywhere as well. If one needs
a pkgconfig file for packaging purposes, both deb and rpm CPack
generators allow the user to add post-install scripts that can generate
it if necessary.
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.

Packaging of this lib has multiple issues
2 participants