Skip to content

Add Test for Basic CMake Config-File Package#122

Closed
chayden83 wants to merge 20 commits intobemanproject:mainfrom
chayden83:configure-file-pakage-test
Closed

Add Test for Basic CMake Config-File Package#122
chayden83 wants to merge 20 commits intobemanproject:mainfrom
chayden83:configure-file-pakage-test

Conversation

@chayden83
Copy link
Copy Markdown
Contributor

This PR extends #121 to include a test to verify that the CMake config-file package is valid. That is:

  • The config-file package can be used with the command find_package(beman.exemplar REQUIRED).
  • The config-file package correctly exports the beman::exemplar target.
  • The exported beman::exemplar target 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.

@chayden83 chayden83 marked this pull request as ready for review February 12, 2025 18:57
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for adding this option?

Comment thread CMakeLists.txt
# targets (e.g., library, executable, etc.).
DESCRIPTION "A Beman library exemplar"
LANGUAGES CXX
VERSION 0.1.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread cmake/test/test.sh
@@ -0,0 +1,8 @@
#!/bin/bash -x
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bretbrownjr bretbrownjr marked this pull request as draft February 16, 2025 18:13
@bretbrownjr
Copy link
Copy Markdown
Member

bretbrownjr commented Feb 16, 2025

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.

@chayden83
Copy link
Copy Markdown
Contributor Author

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.

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.

@chayden83
Copy link
Copy Markdown
Contributor Author

Following up to see if we're ready to merge this PR. Thanks!

@JeffGarland
Copy link
Copy Markdown
Member

I merged your other PR and have updated this branch to the latest main. Looking for @bretbrownjr and others to finish any comments.

@wusatosi
Copy link
Copy Markdown
Member

@chayden83 are you still interested in moving this PR forward?

@wusatosi wusatosi mentioned this pull request Jun 1, 2025
1 task
Comment on lines +28 to +37
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>"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ClausKlein
Copy link
Copy Markdown
Contributor

please have a look at #173

It does all with cmake and ctest only.

In my company, only py or cmake script must be used on as scripts, because it is portable!

@camio
Copy link
Copy Markdown
Member

camio commented Oct 10, 2025

Closing out this PR since there hasn't been activity since June. Feel free to reopen if there's interest in continuing it.

@camio camio closed this Oct 10, 2025
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.

6 participants