Adding installation support (again) #9
Conversation
Added cleaned up installation support, along with an overhauled readme.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This CI test added complexity without adding value. Testing find_package() is sufficient to ensure the package installs correctly.
To simplify the ci logic, I'm changing the ci.yml file to a template.
Adds conditional execution for environment setup and CMake configuration steps based on the presence of matrix.cc.
…brary into sean-parent/install
|
|
||
| # Generate package config file from template | ||
| configure_file( | ||
| "${CPP_LIBRARY_ROOT}/templates/Config.cmake.in" |
There was a problem hiding this comment.
This is the delicate part (the PITA).
This would work well for enum and copy, because their only dependency is cpp_library, and it is a private dependency (no need to find_dependency it).
For stlab, it has internal (enum, copy) and external (qt, libdispatch, Threads) that are public (stlab users will need them if they want to use stlab, well, those who were used during stlab build).
the classic way is that each project will have it own tmpl, and double edit each time he modify a dependency.
this is where my previous proposal of a wrapper "get_dependency" would be handy.
It would store the public dependency, and will be used to generate the corresponding find_dependency calls.
Config.cmake.in (I use Config.tmpl.cmake to keep syntax highlights) would have @Dependencies@ that will replaced here.
If you wish, I can implement a prototype of that. Ideally _cpp_library_setup_install should be called in stlab to test it.
If you wonder in term of test and ci, this issue would have become apparent if it was used on a project with external dependencies, installed, and imported in a test project. It would have complained that Qt5, enum or something was not found and stlab link against it.
There was a problem hiding this comment.
I wouldn't mind adding a way to specify additional dependencies that are not through CPM (e.g., qt, libdispatch). Are CPM dependencies handled correctly (modulo the above naming issue)?
I'm less thrilled with something that requires wrapping CPMAddPackage.
There was a problem hiding this comment.
I though about just using CPM_PACKAGES and CPM_PACKAGE_*_VERSION for theses, but there is one main issue with that, CPM does not indicate anywhere if a dependency is public or private cpp_library and enum are good 2 examples, one is for internal use only, another has to be exposed, and CPM does not have this information.
We could, of course, make workarounds, keeping a hard coded list of private deps like cpp_library. But I feel it a bit shaky.
There is also benefit that all you dependency go through the same function call, no matter the flavor.
It is up to you to decide what you prefer, I can implement it either way.
There was a problem hiding this comment.
The public/private/interface information is provided in target_link_libraries() - in my experiments the dependency on cpp_library is not currently exported but if the other dependencies are.
There was a problem hiding this comment.
Okay - I updated the auto dependency() call generation to allow a map for custom dependency formats. This way, I'm not on the hook to keep a custom table up-to-date.
There was a problem hiding this comment.
So consumer will need to provide correct name for stlab dependencies ?
Sure you do not want a wrapper for dependencies ? It would solve all of this imbroglio.
Anyway, this would work, even though currently it would still generate find_dependency(stlab-copy-on-write) as stlab target name was not changed, only the package name.
There was a problem hiding this comment.
So consumer will need to provide correct name for stlab dependencies ?
I'm not sure what that means - where would they need to provide the name? I reviewed all the naming and made changes. If you have project(enum-ops) ... cpp_library(NAMESPACE stlab...), the package name is stlab-enum-ops both for the installed package and the generated dependency for a client. There were a few inconsistencies in this area but I think I've cleaned them all up (check the read-me and the enum-ops library branch that is using this version).
The install should correctly generate all the dependency calls.
There was a problem hiding this comment.
I may have missunderstood about the "I'm not on the hook to keep a custom table up-to-date.", The current ne is wtill handling only a subset of mappings, in egenral, maps from target name and package name will never be reliable. Mainly because no one is considering they have to be in sync, this is also why on any vcpkg port, you get a helper string like:
[cmake] minhook provides CMake targets:
[cmake]
[cmake] # this is heuristically generated, and may not be correct
[cmake] find_package(minhook CONFIG REQUIRED)
[cmake] target_link_libraries(main PRIVATE minhook::minhook)
This is because the package name and target name cannot be correlated in a reliable way, generally speaking.
The _cpp_library_setup_install function now requires a PACKAGE_NAME argument, ensuring correct package naming in generated CMake config files and install locations. Version detection in _cpp_library_get_git_version now prefers PROJECT_VERSION if set, improving compatibility with package managers and source archives.
Added logic to introspect INTERFACE_LINK_LIBRARIES and generate appropriate find_dependency() calls in installed CMake package config files. Updated documentation to explain dependency handling, added a helper function in cpp-library-install.cmake, and modified the config template to include generated dependencies. This ensures downstream users automatically find and link required dependencies.
Replaces usage of a 'clean' name with PACKAGE_NAME for library target aliases and CI workflow templates. Ensures consistent naming between CMake targets and package discovery, improving clarity and reducing potential mismatches.
Introduces the cpp_library_map_dependency() function to allow custom find_dependency() calls for specific targets, such as Qt components requiring COMPONENTS syntax. Updates documentation and dependency generation logic to prioritize custom mappings, improving flexibility for complex dependencies in installed CMake package config files.
Updated internal CMake functions to require PACKAGE_NAME as a parameter instead of relying on PROJECT_NAME. This clarifies the preconditions for template and CI workflow generation, ensuring correct substitution and improving maintainability.
Ikar-Rahl
left a comment
Ikar-Rahl
left a comment
There was a problem hiding this comment.
Was able to test it with stlab and confirmed that it generate a config file, with some fixes as noted above.
I would suggest also reconsidering not using a dependency wrapper, but I may not have the full picture on the rational regarding that.
The install-test job was removed from the GitHub Actions workflow because it duplicated functionality already provided by the main test job. This simplifies the workflow and reduces unnecessary runs across multiple operating systems. Also fixed a quoting issue with the install prefix on Windows.
Updated dependency mapping logic to distinguish between internal cpp-library and external dependencies, ensuring package names are collision-safe and consistent. Added support for overriding the version using the CPP_LIBRARY_VERSION variable, which is useful for package managers and CI systems without git history. Improved documentation in README.md to clarify dependency handling, target naming patterns, and version management. Refactored CMake scripts to align with these changes and updated comments for clarity.
Adds conditional steps in the CI workflow to set CC and CXX environment variables only when matrix.cc is defined. Ensures builds use the correct compiler configuration based on the matrix setup.
Clarifies that GitHub repository names must match package names, including namespace prefixes, for CPM compatibility. Updates usage examples, instructions, and project links to reflect this requirement and prevent issues with CPM's local package finding and source fetching. Repository naming requirements may be backed out if [this CPM PR](cpm-cmake/CPM.cmake#682) lands.
| add_library(${ARG_NAMESPACE}::${ARG_CLEAN_NAME} ALIAS ${ARG_NAME}) | ||
| target_include_directories(${ARG_NAME} INTERFACE | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | ||
| $<INSTALL_INTERFACE:include> |
There was a problem hiding this comment.
Bug: Exported target name incorrect for alternative naming pattern
When using project(namespace-component) with NAMESPACE namespace, the exported target becomes namespace::namespace-component instead of namespace::component. The target is created with name ARG_NAME (which could be namespace-component), but no EXPORT_NAME property is set. When install(EXPORT) adds the NAMESPACE prefix at line 162 of cmake/cpp-library-install.cmake, it creates a double prefix. Consumers expecting namespace::component (matching the build-tree alias) will fail to link. The target needs EXPORT_NAME set to CLEAN_NAME to ensure consistent exported names regardless of project naming pattern.
Introduces setup.cmake, an interactive script for initializing new C++ library projects using the cpp-library template. The README is updated with detailed quick start instructions for using the script in both interactive and non-interactive modes, as well as guidance for manual setup.
| # Skip generator expressions (typically BUILD_INTERFACE dependencies) | ||
| if(LIB MATCHES "^\\$<") | ||
| continue() | ||
| endif() |
There was a problem hiding this comment.
Bug: Generator expressions incorrectly skipped in dependency generation
The dependency generation skips all generator expressions, including $<INSTALL_INTERFACE:...> dependencies that should be processed. This causes dependencies specified with $<INSTALL_INTERFACE:package::target> to be omitted from the generated find_dependency() calls in the package config file, breaking downstream builds that rely on those dependencies. Only $<BUILD_INTERFACE:...> should be skipped, while $<INSTALL_INTERFACE:...> content needs extraction and processing.
Updated README and setup.cmake to clarify template file generation steps and improve user prompts for input. Project directory is now created in the current working directory, and setup messages better reflect the workflow for generating and regenerating template files.
Ikar-Rahl
left a comment
Ikar-Rahl
left a comment
There was a problem hiding this comment.
So, with the following changes:
- ${PKG_NAME}-${COMPONENT} => ${COMPONENT}
- project(enum-ops) to project(stlab-enum-ops)
- CPMAddPackage("gh:stlab/copy-on-write@1.0.6") => CPMAddPackage("gh:stlab/stlab-copy-on-write@1.0.6"), same for enum.
- Used _cpp_library_setup_install in the stlab, with minor renaming of FILE_SET to headers.
I was able to make it work full circle.
stlab produced a correclty formed Config package:
Find dependencies required by this package
find_dependency(stlab-copy-on-write)
find_dependency(stlab-enum-ops)
find_dependency(Threads)
find_dependency(Qt6)
Final decision about naming still to be made, but this is viable.
cmake/cpp-library-install.cmake
Outdated
| list(APPEND DEPENDENCY_LIST "find_dependency(${PKG_NAME})") | ||
| else() | ||
| # Different names: namespace::component → find_dependency(namespace-component) | ||
| list(APPEND DEPENDENCY_LIST "find_dependency(${PKG_NAME}-${COMPONENT})") |
There was a problem hiding this comment.
If the target is stlab::stlab-enum-ops, this will generate find_dependency(stlab-stlab-enum-ops).
Changin it to
list(APPEND DEPENDENCY_LIST "find_dependency(${COMPONENT})")
Behave correctly with a project(stlab-enum-ops)
Introduces a GitHub Actions CI workflow for unit, integration, and documentation tests. Expands the README with detailed documentation on dependency mapping, version detection, and custom mappings. Refactors and enhances `cpp-library-install.cmake` to support automatic version detection, component merging, and improved error messages for dependency handling. Adds a full unit test suite in `tests/install` with 18 cases covering system packages, external dependencies, component merging, custom mappings, and edge cases. Minor cleanup in `.vscode/extensions.json` and removal of unused symlink logic in `cpp-library.cmake`.
Expanded the README with a troubleshooting section and clarified documentation deployment instructions. Updated comments and logic in cpp-library-install.cmake to centralize system package handling for dependency mapping. Generalized cpp-library.cmake to support both header-only and compiled C++ libraries.
.github/workflows/ci.yml
Outdated
| - name: Verify installation | ||
| run: | | ||
| # Check that package config was installed | ||
| if [ ! -f "${{ runner.temp }}/install/lib/cmake/testlib-test/testlib-testConfig.cmake" ]; then |
There was a problem hiding this comment.
Bug: Integration test expects wrong package and target names
The integration test creates a project with project(test-library) and NAMESPACE testlib, but the expected paths and names are incorrect. According to the package naming logic, CLEAN_NAME would be test-library (not test) since the project name doesn't start with testlib-. Therefore the actual package name would be testlib-test-library and the target alias would be testlib::test-library, but the test expects testlib-test and testlib::test. This will cause the CI verification step and consumer test to fail.
Additional Locations (1)
| "// SPDX-License-Identifier: BSL-1.0 | ||
| #ifndef ${ARG_NAMESPACE}_${ARG_NAME}_HPP | ||
| #define ${ARG_NAMESPACE}_${ARG_NAME}_HPP |
There was a problem hiding this comment.
Bug: Generated header include guard contains invalid identifier with hyphens
When generating header files for libraries with hyphens in their names (e.g., my-library), the include guards are created as ${ARG_NAMESPACE}_${ARG_NAME}_HPP, which would produce something like myns_my-library_HPP. C/C++ preprocessor identifiers cannot contain hyphens, so this results in invalid code that will fail to compile. The library name needs to have hyphens replaced with underscores before being used in the include guard.
| get_property(ALL_PKG_KEYS GLOBAL PROPERTY _CPP_LIBRARY_PKG_KEYS) | ||
| foreach(prop IN LISTS ALL_PKG_KEYS) | ||
| set_property(GLOBAL PROPERTY "_CPP_LIBRARY_PKG_COMPONENTS_${prop}") | ||
| endforeach() |
There was a problem hiding this comment.
Bug: Test cleanup clears keys before reading them
The run_test macro clears _CPP_LIBRARY_PKG_KEYS to empty on line 37, then immediately tries to read it on line 38 to iterate over component properties for cleanup. Since the keys list was just cleared, the loop on lines 39-41 never executes, meaning component properties from previous tests are never cleaned up. The order should be: read the keys first, clear the component properties, then clear the keys list. This bug is masked by the production code's own cleanup in _cpp_library_get_merged_dependencies, but could cause test pollution if the production cleanup behavior changes.
| "// SPDX-License-Identifier: BSL-1.0 | ||
| #ifndef ${ARG_NAMESPACE}_${ARG_NAME}_HPP | ||
| #define ${ARG_NAMESPACE}_${ARG_NAME}_HPP |
There was a problem hiding this comment.
Bug: Header guards with hyphens cause invalid C identifiers
When the library name contains hyphens (e.g., my-library, which the documentation explicitly recommends), the generated header guard becomes invalid. For example, #ifndef mycompany_my-library_HPP produces a C preprocessor identifier containing -, which is not allowed and causes a compilation error. The name needs to have hyphens converted to underscores before being used in the guard macro.
Additional Locations (1)
| #ifndef ${ARG_NAMESPACE}_${ARG_NAME}_HPP | ||
| #define ${ARG_NAMESPACE}_${ARG_NAME}_HPP | ||
| namespace ${ARG_NAMESPACE} { |
There was a problem hiding this comment.
Bug: Namespace declarations invalid when namespace contains hyphens
If a user enters a hyphenated namespace (e.g., my-company), the generated C++ code would contain namespace my-company { which is invalid C++ syntax. C++ namespace identifiers cannot contain hyphens. This occurs in both the header file (line 245) and source file (line 264) generation. Unlike the header guard issue involving ARG_NAME, this affects ARG_NAMESPACE and C++ namespace declarations rather than preprocessor macros.
Additional Locations (1)
| get_property(ALL_PKG_KEYS GLOBAL PROPERTY _CPP_LIBRARY_PKG_KEYS) | ||
| foreach(prop IN LISTS ALL_PKG_KEYS) | ||
| set_property(GLOBAL PROPERTY "_CPP_LIBRARY_PKG_COMPONENTS_${prop}") | ||
| endforeach() |
There was a problem hiding this comment.
Bug: Test cleanup clears keys before reading them
The run_test macro sets _CPP_LIBRARY_PKG_KEYS to empty on line 37, then immediately reads it on line 38. Since the property was just cleared, ALL_PKG_KEYS is always empty and the subsequent loop on lines 39-41 never executes. The get_property call needs to happen before the set_property call to retrieve the previous test's keys for cleanup.
The CI workflow now runs unit tests only on ubuntu-latest instead of a matrix of OSes. Test project and header names were updated from 'test' to 'mylib' for consistency, and related CMake and file references were adjusted accordingly.
| } // namespace ${ARG_NAMESPACE} | ||
| #endif // ${ARG_NAMESPACE}_${ARG_NAME}_HPP |
There was a problem hiding this comment.
Bug: Invalid C preprocessor macro in header guards with hyphens
When a library name contains hyphens (e.g., my-library as shown in the README examples), the generated header guard contains invalid characters. The code produces #ifndef mycompany_my-library_HPP which fails to compile because hyphens are not valid in C/C++ preprocessor macro identifiers. The library name needs to have hyphens converted to underscores before being used in the header guard.
| get_property(ALL_PKG_KEYS GLOBAL PROPERTY _CPP_LIBRARY_PKG_KEYS) | ||
| foreach(prop IN LISTS ALL_PKG_KEYS) | ||
| set_property(GLOBAL PROPERTY "_CPP_LIBRARY_PKG_COMPONENTS_${prop}") | ||
| endforeach() |
There was a problem hiding this comment.
Bug: Test cleanup retrieves property after clearing it
In the run_test macro, _CPP_LIBRARY_PKG_KEYS is cleared on line 37, then immediately retrieved on line 38. This means ALL_PKG_KEYS is always empty, so the loop to clean up component properties (_CPP_LIBRARY_PKG_COMPONENTS_*) from previous tests never executes. The get_property call needs to occur before the set_property that clears the list.
Added cleaned up installation support, along with an overhauled readme.
Note
Adds installation and CMake package-config generation (with dependency mapping/merging), new CI to verify install/find_package, comprehensive README and setup script, and unit tests for dependency handling.
cmake/cpp-library-install.cmakewithcpp_library_map_dependency()and automaticfind_dependency()generation (version detection, system-package handling, component merging) for installedConfig.cmake.templates/Config.cmake.inupdated to include generated@PACKAGE_DEPENDENCIES@and correctTargets.cmakeinclude.cpp-library.cmake/cpp-library-setup.cmake: integrate install when top-level; derivePACKAGE_NAME; supportCPP_LIBRARY_VERSIONoverride; shared/static viaBUILD_SHARED_LIBS.templates/.github/workflows/ci.yml.invia newcmake/cpp-library-ci.cmake..github/workflows/ci.yml: unit tests (CMake script), integration test that builds/installs a sample lib and consumes it viafind_package, and docs checks.templates/.github/workflows/ci.yml.in) and presets addinstallconfiguration.setup.cmakeinteractive bootstrap script.templates/.vscode/extensions.json).tests/install/with 18 unit tests covering dependency mapping, component merging, system packages, and edge cases, plus summary docs..gitignoreadds.DS_Store.Written by Cursor Bugbot for commit 256d0e7. This will update automatically on new commits. Configure here.