Add Test for Basic CMake Config-File Package#122
Add Test for Basic CMake Config-File Package#122chayden83 wants to merge 20 commits intobemanproject:mainfrom
Conversation
…o recognize constexpr?
There was a problem hiding this comment.
Generated file:
set(BEMAN_EXEMPLAR_VERSION 0.1.0)
####### Expanded from @PACKAGE_INIT@ by configure_package_config_file() #######
####### Any changes to this file will be overwritten by the next CMake run ####
####### The input file was beman.exemplar-config.cmake.in ########
get_filename_component(PACKAGE_PREFIX_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE)
macro(set_and_check _var _file)
set(${_var} "${_file}")
if(NOT EXISTS "${_file}")
message(FATAL_ERROR "File or directory ${_file} referenced by variable ${_var} does not exist !")
endif()
endmacro()
macro(check_required_components _NAME)
foreach(comp ${${_NAME}_FIND_COMPONENTS})
if(NOT ${_NAME}_${comp}_FOUND)
if(${_NAME}_FIND_REQUIRED_${comp})
set(${_NAME}_FOUND FALSE)
endif()
endif()
endforeach()
endmacro()
####################################################################################
include(${CMAKE_CURRENT_LIST_DIR}/beman.exemplar-targets.cmake)
check_required_components(beman.exemplar)
bretbrownjr
left a comment
There was a problem hiding this comment.
I like the scope of this PR, @chayden83. It's a big missing feature that I think we can turn this around reasonably efficiently. I like having tests in-repo as well so that mistakes are caught quickly.
As I said in other threads, I'd eventually like all of the install and export logic abstracted out to a reusable helper across Beman libraries, but it makes sense to come back to that in another PR. I do expect that C++ library engineers will consider CMake export code to be magic, understand none of it. and tolerate it at best.
Specifically on the test workflow, I'm happy to see it land now, but I will note that the whole in-repo test approach could be replaced by what Conan calls a "test package" whenever we get far enough to support packaging via Conan. I expect that approach to be a lot more intuitive and maintainable. Eventually.
As to status of the PR, I have a lot of fairly modest style points I'd like you to consider. Let me know if you're OK with the suggestions and let's talk otherwise.
| - name: "Disable example building" | ||
| arg: "-DBEMAN_EXEMPLAR_BUILD_EXAMPLES=OFF" | ||
| - name: "Disable config-file package creation" | ||
| arg: "-DBEMAN_EXEMPLAR_INSTALL_CONFIG_FILE_PACKAGE=OFF" |
There was a problem hiding this comment.
What's the use case for adding this option?
| # targets (e.g., library, executable, etc.). | ||
| DESCRIPTION "A Beman library exemplar" | ||
| LANGUAGES CXX | ||
| VERSION 0.1.0 |
There was a problem hiding this comment.
I suppose this works for now, but ideally the version here would match the release version and the git tag. We need at least a release runbook for making sure that happens. Or we could provide GitHub actions that do the job, maybe leveraging things like the PYPI bumpversion package.
See: #112
| @@ -0,0 +1,8 @@ | |||
| #!/bin/bash -x | |||
There was a problem hiding this comment.
What are your thoughts about just shipping one portable, executable test.cmake script and keeping things simple? As in: cmake -P test.cmake
Alternatively, we could ship this behavior as a CMake preset and just have: cmake --workflow user-test
| set_target_properties(beman.exemplar PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON) | ||
| set_target_properties( | ||
| beman.exemplar | ||
| PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON EXPORT_NAME exemplar |
There was a problem hiding this comment.
| PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON EXPORT_NAME exemplar | |
| PROPERTIES | |
| EXPORT_NAME exemplar | |
| VERIFY_INTERFACE_HEADER_SETS ON |
Is there a way to get the formatter to accept something like this?
It's nice to have key/value pairs on separate lines for clarity if possible.
| add_test( | ||
| NAME beman.exemplar.test.config-file-package | ||
| COMMAND | ||
| "${PROJECT_SOURCE_DIR}/cmake/test/test.sh" |
There was a problem hiding this comment.
I'd prefer using paths relative to CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_BINARY_DIR whenever possible. PROJECT_SOURCE_DIR and such are at a minimum confusing and often wrong when projects get combined into superbuild projects and such.
This goes for the whole PR, not just this file.
If we find all length or indirection annoying, fair, but we can make a local variable or even a special project-wide variable to clarify.
| write_basic_package_version_file( | ||
| "${PROJECT_BINARY_DIR}/cmake/${PROJECT_NAME}/${PROJECT_NAME}-version.cmake" | ||
| VERSION ${PROJECT_VERSION} | ||
| COMPATIBILITY SameMajorVersion |
There was a problem hiding this comment.
I guess I'll defer to advice in Professional CMake if you can provide references, but I expect Beman projects will be rather unstable, so I'd actually start Beman projects off with ExactVersion... or even skipping the version compatibility entirely (leaving it up to the package manager, git operations, etc.).
| include(CMakePackageConfigHelpers) | ||
|
|
||
| configure_package_config_file( | ||
| "${PROJECT_SOURCE_DIR}/cmake/${PROJECT_NAME}-config.cmake.in" |
There was a problem hiding this comment.
I'd rather not use PROJECT_NAME like this. It just obfuscates what's going on. The package name and the project name do match for this project, but they're different settings. I'd be fine just hardcoding beman.exemplar literally throughout this file, but if we want a self-documenting local variable like export_package_name, or something, that's fine too.
|
Oops. I didn't realize when I opened up this PR that #122 was intended to be reviewed first. If it's too much of a pain to figure out which comments go on that PR versus this one, let me know. If you don't mind, I'll put this PR in Draft status since it isn't ready to review until #122 is complete. Feel free to take it out of Draft after that point, assuming you're ready for it to be reviewed fully. |
That makes sense. I don't have any experience w/Conan. I have done a lot of packaging, but only using Linux distro package managers. |
|
Following up to see if we're ready to merge this PR. Thanks! |
|
I merged your other PR and have updated this branch to the latest main. Looking for @bretbrownjr and others to finish any comments. |
|
@chayden83 are you still interested in moving this PR forward? |
| add_test( | ||
| NAME beman.exemplar.test.config-file-package | ||
| COMMAND | ||
| "${PROJECT_SOURCE_DIR}/cmake/test/test.bat" | ||
| "${CMAKE_COMMAND}" | ||
| "${PROJECT_SOURCE_DIR}/cmake/test" # source dir | ||
| "${CMAKE_CURRENT_BINARY_DIR}/cmake/test" # build dir | ||
| "${PROJECT_BINARY_DIR}/cmake/${PROJECT_NAME}" # prefix path | ||
| "$<CONFIG>" | ||
| ) |
There was a problem hiding this comment.
I don't like to call bat as sh scripts, because this is not portable.
And you see, this if else block should to the same, but it does't!
Too the CMAKE_CXX_STANDARD is a user defined value!
For CXX_MODULES, the build and test environment mast be absolute the same.
|
please have a look at #173 It does all with In my company, only |
|
Closing out this PR since there hasn't been activity since June. Feel free to reopen if there's interest in continuing it. |
This PR extends #121 to include a test to verify that the CMake config-file package is valid. That is:
find_package(beman.exemplar REQUIRED).beman::exemplartarget.beman::exemplartarget can be linked.The directory layout I chose is somewhat arbitrary, so please let me know if we should move some of the files to different directories.
Alternatively, if this is not how we want to integrate config-file package tests, please let me know.