build(cmake)!: Add support for installation and usage as a library in other CMake projects; Add example project to test this use case. #65
Conversation
WalkthroughThis update restructures the ystdlib project’s CMake build system and installation infrastructure. It introduces new helper functions, unifies variable naming, removes Boost as a mandatory dependency, modularizes library and test target creation, and adds comprehensive CMake package configuration and installation logic. Example programs and documentation are updated to reflect the new workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant ystdlib-config
participant LibraryConfig
participant Target
User->>CMake: find_package(ystdlib REQUIRED)
CMake->>ystdlib-config: Load ystdlib-config.cmake
ystdlib-config->>LibraryConfig: For each LIB in ystdlib_LIBRARIES, include libs/LIB-config.cmake
LibraryConfig->>Target: Define or import ystdlib::LIB target
ystdlib-config->>CMake: check_required_components(ystdlib)
CMake-->>User: ystdlib targets available
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
CMakeLists.txt (1)
62-67:⚠️ Potential issueLink to
outcomeis missing.You
find_package(outcome REQUIRED)but do not actually link it intoystdlib. Without:target_link_libraries(ystdlib PUBLIC outcome::outcome)downstream consumers will miss this dependency. Please uncomment or add the correct link command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CMake/ystdlib-config.cmake.in(1 hunks)CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
CMakeLists.txt (1)
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
🔇 Additional comments (18)
CMake/ystdlib-config.cmake.in (3)
1-4: Proper initialization of CMake package config.Including
CMakeFindDependencyMacroand invoking@PACKAGE_INIT@is exactly what downstream consumers need to locate this package.
17-20: Correctly set and verify include directory and components.Using
set_and_checkandcheck_required_componentsaligns with best practices inCMakePackageConfigHelpers.
21-24: Guard for multiple inclusion of targets file.The
if(NOT TARGET ystdlib::ystdlib)wrap prevents duplicate imports—this is spot on.CMakeLists.txt (15)
4-7: Module path and helper includes are correct.Appending
CMake/toCMAKE_MODULE_PATHand includingystdlib-cpp-helpers,CMakePackageConfigHelpers, andGNUInstallDirsis necessary for the later package‐config steps.
16-18: Build options follow established convention.Defining
BUILD_SHARED_LIBSandYSTDLIB_CPP_BUILD_TESTINGis clear and consistent.
41-46: Export of compile commands is intentional.Enabling
CMAKE_EXPORT_COMPILE_COMMANDSwithFORCEis useful for IDE integration and static analysis.
48-56: Top‐level dependency settings are correct.Conditionally including your dependency settings and
CTestonly when this is the top‐level project is exactly right.
58-61: Test‐enable logic is clear.Mapping
BUILD_TESTINGandYSTDLIB_CPP_BUILD_TESTINGtoYSTDLIB_CPP_ENABLE_TESTSis straightforward.
69-90: Catch2 test setup looks good.All testing machinery (finding Catch2, setting up unified test target) is correctly gated by
YSTDLIB_CPP_ENABLE_TESTS.
93-95: Installation directory variables are well defined.
YSTDLIB_INSTALL_CONFIG_DIRandYSTDLIB_INSTALL_INCLUDE_DIRcorrectly map tolib/cmake/ystdliband the chosen include directory.
123-124: Linker language explicitly set.Specifying
LINKER_LANGUAGE CXXis correct for a C++ library.
132-137: Installation of the library target is correct.Exporting
ystdlibasystdlib-targetsis aligned with CMake package guidelines.
139-146: Export namespace and destination are set properly.Using
NAMESPACE ystdlib::ensures downstreamfind_package(ystdlib)works as intended.
148-157: Header installation looks good.Copying
*.h,*.hpp, and*.tppfromsrc/ystdlibis appropriate.
159-166: Package configuration generation is correct.The call to
configure_package_config_filewithPATH_VARS YSTDLIB_INSTALL_INCLUDE_DIRmatches the template.
168-172: Version file creation is correct.Using
SameMajorVersioncompatibility is sensible for a 0.x library.
174-180: Config files are installed to the right location.Installing both the config and version files under
${YSTDLIB_INSTALL_CONFIG_DIR}enables clean downstreamfind_package.
182-183: Verify no source duplication viaadd_subdirectory.You glob all sources under
src/to createystdlib, then addsrc/ystdlibagain as a subdirectory. This can lead to duplicate targets or conflicting CMakeLists. Please confirm whetheradd_subdirectory(src/ystdlib)is still required in this context.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/ystdlib/error_handling/test/test_ErrorCode.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/ystdlib/error_handling/test/test_ErrorCode.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
CMakeLists.txt (3)
11-14: 🛠️ Refactor suggestionInconsistent project name vs package name
The project is declared asYSTDLIB_CPPbut all install paths and the export namespace useystdlib. This mismatch can confuse downstream users callingfind_package(ystdlib). Consider renaming the project toystdlib(or vice versa) to align naming.Apply this diff:
- project(YSTDLIB_CPP + project(ystdlib VERSION "${YSTDLIB_CPP_VERSION}" LANGUAGES CXX )
113-115: 🛠️ Refactor suggestionPropagate C++20 requirement to consumers
PRIVATE cxx_std_20hides the C++ standard requirement from downstream targets. Change toPUBLIC cxx_std_20so any project linking againstystdlibwill also compile with C++20.-target_compile_features(ystdlib - PRIVATE cxx_std_20 -) +target_compile_features(ystdlib + PUBLIC cxx_std_20 +)
96-102:⚠️ Potential issueRegex for excluding test files is incorrect
The current regex".*test_*\..*"does not properly match filenames liketest_foo.cpp. Update to:- list(FILTER SOURCE_FILES EXCLUDE REGEX ".*test_*\..*") + list(FILTER SOURCE_FILES EXCLUDE REGEX ".*test_.*\\..*")This ensures any file starting with
test_is filtered out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
CMakeLists.txt (1)
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
🔇 Additional comments (8)
CMakeLists.txt (8)
4-7: Good addition of CMake helper modules
Appending the localCMakedirectory toCMAKE_MODULE_PATHand includingCMakePackageConfigHelpersandGNUInstallDirscorrectly equips the project for package configuration and standard install-directory variables.
104-106: Correct creation of library and alias
Theadd_library(ystdlib ${SOURCE_FILES})and the subsequent aliasadd_library(ystdlib::ystdlib ALIAS ystdlib)properly define both the target and its public namespace.
107-112: Include directories are well defined
UsingPUBLIC $<BUILD_INTERFACE:…>andPUBLIC $<INSTALL_INTERFACE:…>sets up clean include paths for both build-time and install-time usage.
126-133: Export namespace installation is correct
Theinstall(EXPORT ystdlib-targets NAMESPACE ystdlib:: DESTINATION ${YSTDLIB_INSTALL_CONFIG_DIR})ensures downstreamfind_package(ystdlib)can locate your targets.
135-144: Header installation matches expectations
Installing thesrc/ystdlibdirectory withFILES_MATCHINGpatterns for.h,.hpp, and.tppcorrectly delivers public headers.
146-153: Package config generation is correct
Theconfigure_package_config_fileinvocation properly generatesystdlib-config.cmakefor downstream consumers to locate dependencies and targets.
155-159: Version file generation is correct
Thewrite_basic_package_version_filewithSameMajorVersionprovides necessary version compatibility semantics.
161-167: Installation of generated config files is correct
Installing both the config and version files into${YSTDLIB_INSTALL_CONFIG_DIR}completes the package setup forfind_package.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
CMakeLists.txt (4)
120-120: 🧹 Nitpick (assertive)Specify explicit install destinations
It’s best practice to define explicit destinations for archives, shared libraries, and executables ininstall(TARGETS ...)to ensure consistent install layouts across platforms. For example:install(TARGETS ystdlib EXPORT ystdlib-targets -) + ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} + LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} + RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} +)
111-111:⚠️ Potential issuePropagate C++20 requirement to consumers
The current visibility isPRIVATE, which hides the C++20 requirement from downstream targets and may lead to compilation errors in client code. Change toPUBLICso that any project linking againstystdlibalso uses C++20:- target_compile_features(ystdlib PRIVATE cxx_std_20) + target_compile_features(ystdlib PUBLIC cxx_std_20)
124-135: 🛠️ Refactor suggestionUpdate include install path variable
After renaming toYSTDLIB_CPP_INSTALL_INCLUDE_DIR, update theinstall(DIRECTORY ...)call accordingly to maintain consistency:- DESTINATION "${YSTDLIB_INSTALL_INCLUDE_DIR}" + DESTINATION "${YSTDLIB_CPP_INSTALL_INCLUDE_DIR}"
99-99:⚠️ Potential issueFix test file exclusion regex
The current regex".*test_*\..*"does not exclude files liketest_foo.cpp(the*quantifier applies to_, and the\.is placed too early). Update to properly filter out any file beginning withtest_. For example:- list(FILTER SOURCE_FILES EXCLUDE REGEX ".*test_*\\..*") + list(FILTER SOURCE_FILES EXCLUDE REGEX ".*test_.*\\..*")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
CMakeLists.txt (1)
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
🔇 Additional comments (8)
CMakeLists.txt (8)
5-6: Include modules for package configuration and standard dirs
IncludingCMakePackageConfigHelpersandGNUInstallDirsis correct and necessary to supportconfigure_package_config_file, version file generation, and standard installation directory variables.
92-98: Enable reconfigure on new sources
UsingCONFIGURE_DEPENDSwithfile(GLOB_RECURSE ...)is good practice, as it triggers CMake reconfiguration when source files are added or removed.
101-102: Define library and alias target
Theadd_library(ystdlib ${SOURCE_FILES})and aliasadd_library(ystdlib::ystdlib ALIAS ystdlib)are correctly configured for downstream CMake consumption.
104-109: Expose include directories properly
Setting the include directories asPUBLICwithBUILD_INTERFACEandINSTALL_INTERFACEis the right approach to ensure consumers see the correct headers both in-tree and from an installed package.
122-122: Export namespace installation
Installing the export under theystdlib::namespace correctly aligns the build-tree and install-tree usage forfind_package(ystdlib).
137-143: Configure package config file
The use ofconfigure_package_config_filewithystdlib-config.cmake.inand specifyingPATH_VARSfor the include dir is correctly set up for downstream package discovery.
145-148: Write package version file
Generating the version file withSameMajorVersioncompatibility ensures consumers can check version constraints appropriately.
150-155: Install package config files
Installing bothystdlib-config.cmakeandystdlib-config-version.cmakeinto the config directory completes the exported CMake package layout.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
CMake/ystdlib-config.cmake.in (2)
7-9: Pin Catch2 version in find_dependency
Please propagate the Catch2 version to downstream by specifying:-if(@Catch2_FOUND@) - find_dependency(Catch2) +if(@Catch2_FOUND@) + find_dependency(Catch2 @Catch2_VERSION@ REQUIRED) endif()
11-13: Pin outcome version in find_dependency
Similarly, include the outcome version constraint:-if(@outcome_FOUND@) - find_dependency(outcome) +if(@outcome_FOUND@) + find_dependency(outcome @outcome_VERSION@ REQUIRED) endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-config.cmake.in(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
🔇 Additional comments (3)
CMake/ystdlib-config.cmake.in (3)
5-6: Initialise package version correctly
The@PACKAGE_INIT@placeholder should remain directly after the include directives to load the version file and initialize the package.
17-17: Verify required components specification
Confirm thatcheck_required_components(ystdlib)enforces the correct set of components. If your library defines named components (e.g.,Core,Extras), consider listing them explicitly:check_required_components(ystdlib Core Extras)
23-23: Re-enable gersemi linting
The# gersemi: ondirective correctly resumes linting for downstream edits.
|
Create a sample project to verify that the installed package actually works. |
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
examples/README.md (1)
1-10: 🛠️ Refactor suggestionImprove documentation structure and clarity
The README has several issues that affect usability:
- Missing proper markdown heading structure
- Grammatical errors and unclear phrasing
- Fragmented instructions without sufficient context
+# ystdlib-cpp Examples + +This directory contains examples demonstrating how to use ystdlib-cpp as an installed CMake package. + +## Prerequisites + +Build and install the main ystdlib-cpp project first: + +```bash +# From the project root +mkdir build && cd build -cmake .. - -Run above command from examples/build - -Dependencies: - -After building main project, cd to build and run - -cmake --install . --prefix=build_install to have an install location +cmake .. +make +cmake --install . --prefix=../install +``` + +## Building the Examples + +```bash +# From the examples directory +mkdir build && cd build +cmake .. -DCMAKE_PREFIX_PATH=../../install +make +```🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Two consecutive dots
Context: cmake .. Run above command from examples/build ...(DOUBLE_PUNCTUATION)
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...es/build Dependencies: After building main project, cd to build and run cmake --i...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~9-~9: The word ‘install’ is not a noun.
Context: ...all . --prefix=build_install to have an install location(A_INSTALL)
🪛 markdownlint-cli2 (0.17.2)
1-1: First line in a file should be a top-level heading
null(MD041, first-line-heading, first-line-h1)
♻️ Duplicate comments (2)
CMake/ystdlib-config.cmake.in (2)
3-3: Include CMakePackageConfigHelpers for config helpersTo ensure the macros
set_and_checkandcheck_required_componentsare available, addinclude(CMakePackageConfigHelpers)immediately after
include(CMakeFindDependencyMacro), or verify that@PACKAGE_INIT@injects it.
9-9: Use consistent variable prefix for include directoryBased on the project convention (
YSTDLIB_CPP_prefix for CMake variables), renameystdlib_INCLUDE_DIRto:-set_and_check(ystdlib_INCLUDE_DIR "@PACKAGE_ystdlib_INSTALL_INCLUDE_DIR@") +set_and_check(YSTDLIB_CPP_INCLUDE_DIR "@PACKAGE_ystdlib_INSTALL_INCLUDE_DIR@")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
CMake/ystdlib-config.cmake.in(1 hunks)CMake/ystdlib-cpp-helpers.cmake(5 hunks)CMakeLists.txt(4 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)examples/README.md(1 hunks)examples/constants.hpp(1 hunks)examples/main.cpp(1 hunks)examples/types.cpp(1 hunks)examples/types.hpp(1 hunks)lint-requirements.txt(1 hunks)src/.clang-format(1 hunks)src/ystdlib/containers/test/test_Array.cpp(2 hunks)src/ystdlib/error_handling/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/ErrorCode.hpp(1 hunks)src/ystdlib/error_handling/Result.hpp(4 hunks)src/ystdlib/error_handling/test/test_Result.cpp(1 hunks)taskfile.yaml(1 hunks)taskfiles/build.yaml(4 hunks)taskfiles/deps.yaml(1 hunks)taskfiles/lint-cpp.yaml(4 hunks)taskfiles/lint-venv.yaml(1 hunks)taskfiles/lint-yaml.yaml(1 hunks)tools/yscope-dev-utils(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
examples/constants.hppsrc/ystdlib/error_handling/ErrorCode.hppsrc/ystdlib/containers/test/test_Array.cppexamples/main.cppsrc/ystdlib/error_handling/test/test_Result.cppexamples/types.cppexamples/types.hppsrc/ystdlib/error_handling/Result.hpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
taskfile.yaml (1)
Learnt from: LinZhihao-723
PR: y-scope/ystdlib-cpp#66
File: taskfile.yaml:4-4
Timestamp: 2025-05-27T22:22:47.058Z
Learning: Task (go-task) has native support for `set` and `shopt` options in Taskfile.yaml that work with the default `mvdan.cc/sh` interpreter, without requiring `interpreter: bash`. The `shopt: ["globstar"]` option will enable recursive globbing patterns like `**/*` without additional shell configuration.
CMake/ystdlib-config.cmake.in (1)
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
src/ystdlib/error_handling/Result.hpp (3)
Learnt from: anlowee
PR: y-scope/ystdlib-cpp#62
File: src/ystdlib/error_handling/Result.hpp:63-63
Timestamp: 2025-05-31T00:43:32.193Z
Learning: In Boost.Outcome, BOOST_OUTCOME_TRYV macro is universally available and doesn't need #ifdef guards, unlike BOOST_OUTCOME_TRYX which is GCC/Clang-specific and requires guarding.
Learnt from: anlowee
PR: y-scope/ystdlib-cpp#62
File: src/ystdlib/error_handling/Result.hpp:63-63
Timestamp: 2025-05-31T00:43:32.193Z
Learning: In Boost.Outcome, BOOST_OUTCOME_TRYV macro is universally available and doesn't need #ifdef guards, unlike BOOST_OUTCOME_TRYX which is GCC/Clang-specific and requires guarding.
Learnt from: anlowee
PR: y-scope/ystdlib-cpp#62
File: src/ystdlib/error_handling/Result.hpp:63-63
Timestamp: 2025-05-31T00:43:32.193Z
Learning: In Boost.Outcome, BOOST_OUTCOME_TRYV macro is universally available and doesn't need #ifdef guards, unlike BOOST_OUTCOME_TRYX which is GCC/Clang-specific and requires guarding.
CMakeLists.txt (1)
Learnt from: Bill-hbrhbr
PR: y-scope/ystdlib-cpp#32
File: CMakeLists.txt:0-0
Timestamp: 2025-02-28T05:04:33.616Z
Learning: The y-scope/ystdlib-cpp project uses the "YSTDLIB_CPP_" prefix for project-specific CMake variables, following good naming convention practices.
🪛 markdownlint-cli2 (0.17.2)
README.md
11-11: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
examples/README.md
1-1: First line in a file should be a top-level heading
null
(MD041, first-line-heading, first-line-h1)
🪛 LanguageTool
examples/README.md
[typographical] ~1-~1: Two consecutive dots
Context: cmake .. Run above command from examples/build ...
(DOUBLE_PUNCTUATION)
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ...es/build Dependencies: After building main project, cd to build and run cmake --i...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~9-~9: The word ‘install’ is not a noun.
Context: ...all . --prefix=build_install to have an install location
(A_INSTALL)
🔇 Additional comments (44)
tools/yscope-dev-utils (1)
1-1: Update subproject commit reference
This bumps thetools/yscope-dev-utilssubproject to commit76aecbd6d0db6e2078005431946b5c2e716c4291, aligning it with the reorganized taskfile namespaces and paths.lint-requirements.txt (1)
1-2: LGTM: Reasonable version constraint updatesThe relaxation of version constraints from fixed minor versions to minimum version requirements allows for better tooling flexibility while maintaining compatibility.
src/ystdlib/containers/test/test_Array.cpp (1)
78-78: LGTM: Excellent type safety improvementConverting from
inttosize_tfor array indices and container operations is a great improvement that:
- Eliminates signed/unsigned comparison issues
- Aligns with standard library conventions where container sizes are
size_t- Improves type safety and correctness
The changes are applied consistently across all affected test cases.
Also applies to: 86-89, 95-99, 105-107, 116-116
src/ystdlib/error_handling/CMakeLists.txt (1)
10-10:✅ Verification successful
Verify consistency of dependency change across the codebase
The change from
outcome::hltoBoost::headersaligns with the broader project shift to Boost libraries. Ensure that corresponding header changes (e.g., namespace updates in Result.hpp) are consistently applied throughout the codebase.
🏁 Script executed:
#!/bin/bash # Verify that all outcome namespace references have been updated to boost::outcome rg -n "outcome::" --type cpp --type hpp rg -n "#include.*outcome" --type cpp --type hppLength of output: 145
To ensure we catch all references correctly, let’s retry without relying on built-in file-type filters:
🏁 Script executed:
#!/bin/bash # Search for outcome:: namespace usage in all .cpp/.hpp files rg -n "outcome::" -g '*.cpp' -g '*.hpp' # Search for direct includes of outcome headers in all .cpp/.hpp files rg -n "#include.*outcome" -g '*.cpp' -g '*.hpp' # Check CMakeLists for any remaining Outcome references rg -n "Outcome" -g 'CMakeLists.txt'Length of output: 437
Dependency update to Boost::headers verified
All legacy
outcome::usages and includes have been replaced. In particular,src/ystdlib/error_handling/Result.hppnow uses only:
<boost/outcome/config.hpp><boost/outcome/std_result.hpp><boost/outcome/success_failure.hpp><boost/outcome/try.hpp>No remaining
outcome::or non-Boost includes detected.src/.clang-format (1)
9-9: Boost headers grouping aligns with updated dependencies
Updating the IncludeCategories regex to group Boost and Catch2 headers reflects the shift from Outcome to Boost.README.md (2)
12-13: Correct testing option name
Switching fromYSTDLIB_CPP_BUILD_TESTINGtoystdlib_BUILD_TESTINGaligns with the CMake option rename and ensures consistency.
70-70: Consistent casing for build-testing variable
Lowercaseystdlib_BUILD_TESTINGusage matches CMake conventions and the updated option in the build scripts.src/ystdlib/error_handling/test/test_Result.cpp (1)
109-110: Formatting adjustment only
The line break for the equality assertion improves readability without affecting logic.taskfile.yaml (2)
3-4: Enable stricter shell options
Addingset: ["u", "pipefail"]andshopt: ["globstar"]increases robustness and supports recursive globbing per project conventions.
10-10: Update utils include path
The new pathtools/yscope-dev-utils/exports/taskfiles/utils/utils.yamlmatches the reorganised utilities structure.taskfiles/lint-venv.yaml (1)
18-18: Align task references with reorganised utils namespaces
Updating task references to:utils:checksum:validate,:utils:misc:create-venv, and:utils:checksum:computereflects the reorganised task namespaces and ensures the correct utility modules are invoked.Also applies to: 23-23, 29-29
CMake/ystdlib-config.cmake.in (2)
7-7: Correct dependency usage for BoostThe use of
find_dependency(Boost REQUIRED)aligns with the project's migration from theoutcomelibrary to Boost::outcome as mentioned in previous discussions.
14-16: Target inclusion logic is correctThe conditional check and quoted include path properly handle the target import while avoiding redefinition.
taskfiles/build.yaml (1)
13-13: Task reference updates are consistentThe systematic update from
:utils:cmake-buildto:utils:cmake:buildpattern is consistent across all task references and aligns with the broader task namespace reorganization.Also applies to: 24-24, 52-52, 62-62
taskfiles/lint-yaml.yaml (2)
17-18: Practical exclusion for CLion build directoriesAdding exclusion for
cmake-build-*directories is a sensible improvement for developers using CLion IDE.
26-36: Improved command formatting and path updatesThe reformatted command structure and updated yamllint config path align with the project's structural reorganization while maintaining clarity.
src/ystdlib/error_handling/ErrorCode.hpp (1)
90-90: Improved specifier orderingReordering from
constexpr statictostatic constexprfollows the conventional order where storage class specifiers precede other specifiers, improving code style consistency.examples/main.cpp (1)
15-53: Comprehensive error handling exampleThe example effectively demonstrates the ErrorCode framework with thorough testing of:
- Error code construction and conversion to std::error_code
- Message and category validation
- Same-category and cross-category comparisons
- Proper assertion of expected behavior
The implementation follows good practices with proper includes, consistent style, and logical test coverage.
CMake/ystdlib-cpp-helpers.cmake (4)
31-32: Variable naming convention changes look good.The transition from
YSTDLIB_CPP_ENABLE_TESTStoystdlib_ENABLE_TESTSaligns with the project's standardised naming conventions. The comment updates also reflect the consistent use ofystdlibthroughout the project.Also applies to: 43-43, 153-153
105-109: Install interface configuration is properly implemented.The addition of
INSTALL_INTERFACEgenerator expression correctly configures the include directories for installed packages usingCMAKE_INSTALL_INCLUDEDIR. This enables dependent projects to find headers after installation.
138-144: Header installation logic follows CMake best practices.The installation of public headers to
${CMAKE_INSTALL_INCLUDEDIR}/ystdlib/${arg_cpp_lib_NAME}provides a well-structured installation hierarchy that prevents header conflicts and follows standard conventions.
146-151: Explicit LINKER_LANGUAGE property setting is beneficial.Setting the
LINKER_LANGUAGEproperty toCXXfor both library and test targets ensures proper C++ linking behaviour, especially important for header-only libraries that might not automatically detect the language.Also applies to: 174-179
examples/constants.hpp (1)
9-15: Constant declarations follow modern C++ best practices.The use of
constexpr std::string_viewandconstexpr std::arraywith proper type deduction demonstrates excellent modern C++ practices. The naming convention with thecprefix clearly identifies these as constants.src/ystdlib/error_handling/Result.hpp (1)
6-9: Migration to Boost.Outcome is correctly implemented.The systematic transition from standalone
outcometo Boost.Outcome is well-executed:
- All includes updated from
<outcome/...>to<boost/outcome/...>- Namespace references consistently changed from
OUTCOME_V2_NAMESPACEtoBOOST_OUTCOME_V2_NAMESPACE- Macro prefixes correctly updated from
OUTCOME_*toBOOST_OUTCOME_*- Conditional compilation for
BOOST_OUTCOME_TRYXis properly maintained for GCC/Clang-specific usageAlso applies to: 23-23, 29-30, 46-46, 48-48, 63-63
examples/types.cpp (2)
24-38: Template specialization implementation is correct.The
AlwaysSuccessErrorCategoryspecializations properly implement the required interface with appropriate error handling for unrecognized error codes.
45-55: Template specialization implementation is correct.The
BinaryErrorCategory::message()specialization correctly handles both success and failure cases with appropriate default handling.taskfiles/deps.yaml (3)
12-18: Dependency installation approach is well-structured.The simplified
install-alltask using the utility task approach provides a clean interface for dependency management while maintaining the necessary CMake settings configuration.
20-24: Parallel installation correctly targets new dependencies.The
install-all-paralleltask properly reflects the migration from outcome/quickcpplib to Boost, maintaining efficient parallel installation.
38-49: Boost installation configuration is appropriate.The Boost dependency configuration correctly specifies:
- Specific version (1.87.0) with expected SHA256
- Headers-only target (appropriate for Boost.Outcome usage)
- Proper utility task usage for installation
taskfiles/lint-cpp.yaml (4)
3-4: Output directory configuration is a good addition.The new
G_LINT_CLANG_TIDY_DIRvariable provides proper organization for clang-tidy output files.
32-37: Utility namespace migration is consistent.The migration to
:utils:cpp-lint:clang-formatwith improved variable structure (FLAGS as list, explicit INCLUDE_FILENAME_PATTERNS and ROOT_PATHS) provides better maintainability and clarity.Also applies to: 46-51
66-73: Clang-tidy configuration improvements are well-structured.The migration to
:utils:cpp-lint:clang-tidy-findwith FLAGS as a quoted string list and the addition of OUTPUT_DIR provides better organization and consistency with the utility namespace restructuring.
78-86: Configuration file paths correctly updated.The updated paths to
tools/yscope-dev-utils/exports/lint-configsalign with the utility reorganization and maintain proper lint configuration management.CMakeLists.txt (11)
5-5: Project name aligned with package.
Switching the project declaration toystdlibnow matches your exported package name and avoids confusion when users callfind_package(ystdlib).
7-10: Good ordering of includes.
Addingproject(...)beforeinclude(GNUInstallDirs)and appropriately appending toCMAKE_MODULE_PATHensures no developer warnings and makes helper modules available.
13-13: Consistent testing option naming.
Using a project-scopedystdlib_BUILD_TESTINGflag is clear. Ensure it’s documented in your README.
44-44: Top-level project guard is correct.
Conditionally including dependency settings and CTest only whenystdlib_IS_TOP_LEVELisONprevents leaking those macros into downstream projects.
54-55: Test enabling logic is sound.
CombiningBUILD_TESTINGwith your project-level flag yields precise control over test builds.
63-64: Catch2 dependency is correctly versioned.
RequiringCatch2 3.8.0ensures compatibility withcatch_discover_tests.
70-70: Include Catch2 module appropriately.
Callinginclude(Catch)inside the tests guard is needed forcatch_discover_tests. Good placement.
90-90: Subdirectory inclusion is correct.
Usingadd_subdirectory(src/ystdlib)properly scopes your library targets for later installation.
104-104: Export installation is configured correctly.
Installing theystdlib-targetsexport under theystdlib::namespace aligns with CMake package conventions.
114-117: Package version file generation is correct.
Usingwrite_basic_package_version_filewithSameMajorVersioncompatibility is the recommended approach.
119-124: Install config files under proper directory.
The files are installed to${ystdlib_INSTALL_CONFIG_DIR}, which matches your namespace. Good.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
cmake/ystdlib-helpers.cmake (4)
11-11: Error message inaccuracy needs correction.The error message says "Empty value for argument" but the condition
NOT DEFINED ARG_${_NAME} OR ARG_${_NAME} STREQUAL ""checks for both undefined AND empty values.
24-24: Header file regex is too restrictive.The pattern
".*\\.(h|hpp)"ignores upper-case extensions and common variants like.hhor.hxx, potentially causing header-only libraries to be treated as compiled libraries.
158-163: RUNTIME_OUTPUT_DIRECTORY not multi-config compatible.Setting
RUNTIME_OUTPUT_DIRECTORYdirectly breaks multi-configuration generators like Visual Studio or Xcode where per-config directories are expected.
214-214: Install command may emit warnings for INTERFACE libraries.The install command attempts to install
LIBRARY ARCHIVE RUNTIMEfor all targets, which will cause warnings for INTERFACE (header-only) targets that have no artifacts.CMakeLists.txt (3)
1-1: Breaking change requires documentation in PR title.Raising the minimum CMake version to 3.23 is a breaking change for dependent projects (e.g., CLP still supports Ubuntu Jammy). This should be indicated in the PR title.
55-55: ystdlib_IS_TOP_LEVEL may be undefined.The guard assumes this variable is defined elsewhere. If missing, dependencies and CTest setup will be skipped.
98-102: Add PATH_VARS to configure_package_config_file.The call is missing
PATH_VARSto properly substitute install path variables instead of embedding absolute paths.configure_package_config_file( "${CMAKE_CURRENT_LIST_DIR}/cmake/${CONFIG_FILE_PREFIX}.cmake.in" "${CONFIG_OUTPUT_PATH}" + PATH_VARS ystdlib_INSTALL_CONFIG_DIR INSTALL_DESTINATION "${ystdlib_INSTALL_CONFIG_DIR}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(2 hunks)cmake/ystdlib-config.cmake.in(1 hunks)cmake/ystdlib-helpers.cmake(1 hunks)examples/src/linking-tests.cpp(1 hunks)src/ystdlib/error_handling/CMakeLists.txt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
examples/src/linking-tests.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
cmake/ystdlib-helpers.cmake (2)
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:05:45.088Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:02:19.821Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
CMakeLists.txt (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-ystdlib-cpp (macos-15)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04)
- GitHub Check: build-ystdlib-cpp (macos-14)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
cmake/ystdlib-config.cmake.in (1)
1-13: LGTM! Clean package configuration template.The implementation correctly uses
@ystdlib_LIBRARIES@substitution, includes component config files only if targets don't exist, and properly callscheck_required_components().src/ystdlib/error_handling/CMakeLists.txt (2)
1-3: Good practice: Early return guard.The target existence check prevents redundant processing when the target is already defined.
14-39: Excellent modular structure using helper functions.The use of
add_cpp_library,add_catch2_tests, andinstall_libraryprovides a clean, consistent approach to library definition, testing, and installation.examples/src/linking-tests.cpp (3)
65-68: Good practice: Function prototypes improve maintainability.The forward declarations allow for function reordering and better code organization.
104-104: Excellent adherence to coding guidelines.Using
false == test_containers()instead of!test_containers()follows the project's coding guidelines preference.
47-62: Proper use of [[maybe_unused]] attribute.The consistent use of
[[maybe_unused]]for unreferenced parameters follows modern C++ best practices and avoids compiler warnings.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
cmake/ystdlib-helpers.cmake (3)
24-24: Expand header file detection regex for better coverage.The current regex
.*\\.(h|hpp)is case-sensitive and misses common header file extensions like.hh,.hxx, and uppercase variants.
158-163: Fix multi-config generator compatibility.Hard-coding
RUNTIME_OUTPUT_DIRECTORYbreaks multi-config generators like Visual Studio/Xcode where per-config directories are expected.
214-214: Handle INTERFACE libraries in install command.The install command with
LIBRARY ARCHIVE RUNTIMEwill emit warnings for INTERFACE targets that have no artifacts.examples/CMakeLists.txt (1)
3-3: Consider using underscores instead of hyphens in project name.The project name
ystdlib-examplescreates CMake variables with hyphens (likeystdlib-examples_IS_TOP_LEVELon line 18), which can cause parsing issues in CMake expressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(2 hunks)cmake/ystdlib-helpers.cmake(1 hunks)examples/CMakeLists.txt(1 hunks)examples/README.md(1 hunks)examples/src/linking-tests.cpp(1 hunks)src/ystdlib/error_handling/CMakeLists.txt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
examples/src/linking-tests.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
cmake/ystdlib-helpers.cmake (2)
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:05:45.088Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:02:19.821Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
CMakeLists.txt (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
🪛 LanguageTool
examples/README.md
[uncategorized] ~11-~11: The preposition “in” seems more likely in this position.
Context: .../examples/ystdlib`. If you installed it to a different location, adjust the paths ...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04)
- GitHub Check: build-ystdlib-cpp (macos-15)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04)
- GitHub Check: build-ystdlib-cpp (macos-14)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (14)
examples/README.md (1)
1-29: Documentation looks good after addressing past feedback.The README has been significantly improved with better structure, clearer instructions, and proper formatting. All the major issues from previous reviews appear to have been resolved.
src/ystdlib/error_handling/CMakeLists.txt (1)
1-40: Clean refactoring using new helper functions.The CMake configuration has been nicely refactored to use the standardized helper functions. The early return guard, Boost dependency handling, and separation of library definition from testing are all well-structured improvements.
examples/src/linking-tests.cpp (1)
70-125: Well-structured integration test demonstrating library usage.The test implementation effectively validates all major ystdlib components with appropriate error handling and clear return codes. The code demonstrates proper usage patterns for containers, error handling, IO interfaces, and wrapped facade headers, serving as both a validation test and usage example.
CMakeLists.txt (11)
1-1: Weigh the CMake version bump as a breaking change.Requiring CMake 3.23 will break builds on older platforms (e.g., Ubuntu Jammy). Please update the PR title, release notes, and README to call out this requirement.
3-3: Project declaration looks good.Renaming the project to lowercase
ystdliband starting at version0.1.0aligns with your convention for new libraries.
5-8: Helper modules inclusion is correct.Including
CMakePackageConfigHelpers,GNUInstallDirs, and your lowercaseystdlib-helpersis consistent and in alphabetical order.
11-11: Test-build option naming is consistent.Using
ystdlib_BUILD_TESTINGwith a lowercaseystdlib_prefix follows your project’s naming convention.
41-48: Library list variable definition is clear.Defining
ystdlib_LIBRARIESas a semicolon-separated CACHE STRING provides flexibility in selecting which sub-libraries to build.
50-53: Logging of selected libraries is helpful.Emitting the list of libraries at configure time improves user feedback.
65-68: Test-enable logic is sound.Conditionally setting
ystdlib_ENABLE_TESTSbased on bothBUILD_TESTINGandystdlib_BUILD_TESTINGcorrectly gates test discovery.
69-86: Unified Catch2 test target is configured correctly.The use of
Catch2::Catch2WithMain, test discovery, and private C++20 requirement on the test executable is appropriate.
91-91: Top-level library inclusion is correct.Delegating all sub-libraries to
src/ystdlibviaadd_subdirectorymatches your modular build approach.
93-93: Package config directory is well-defined.Setting
ystdlib_INSTALL_CONFIG_DIRunder${CMAKE_INSTALL_LIBDIR}/cmake/ystdlibfollows GNUInstallDirs conventions and your lowercase prefix.
106-111: Configuration file installation is correct.Installing both the config and version files to the package config directory ensures
find_package(ystdlib)will locate them.
CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| # All libraries are required to use the same minimum version of dependencies to avoid issues. | ||
| set(MIN_BOOST_VERSION "1.81.0") |
There was a problem hiding this comment.
How about BOOST_MIN_VERSION?
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
cmake/ystdlib-helpers.cmake (2)
214-214: Installation command may generate warnings for INTERFACE libraries.The install command specifies
LIBRARY ARCHIVE RUNTIMEfor all targets, which will generate warnings for INTERFACE targets that have no artifacts to install.Consider making the install command conditional based on target type:
- install(TARGETS "${ARG_NAME}" EXPORT "${EXPORT_NAME}" LIBRARY ARCHIVE RUNTIME FILE_SET HEADERS) + get_target_property(TARGET_TYPE "${ARG_NAME}" TYPE) + if(TARGET_TYPE STREQUAL "INTERFACE_LIBRARY") + install(TARGETS "${ARG_NAME}" EXPORT "${EXPORT_NAME}" FILE_SET HEADERS) + else() + install(TARGETS "${ARG_NAME}" EXPORT "${EXPORT_NAME}" LIBRARY ARCHIVE RUNTIME FILE_SET HEADERS) + endif()
157-163: Consider multi-config generator compatibility.The
RUNTIME_OUTPUT_DIRECTORYproperty is set directly, which may not work optimally with multi-configuration generators like Visual Studio or Xcode.Consider using generator expressions for better multi-config support:
- set_property( - TARGET - ${UNIT_TEST_TARGET} - PROPERTY - RUNTIME_OUTPUT_DIRECTORY - ${CMAKE_BINARY_DIR}/testbin - ) + set_target_properties(${UNIT_TEST_TARGET} PROPERTIES + RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/testbin" + RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_BINARY_DIR}/testbin" + RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_BINARY_DIR}/testbin" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(2 hunks)README.md(4 hunks)cmake/ystdlib-helpers.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
README.md (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
cmake/ystdlib-helpers.cmake (2)
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:05:45.088Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:02:19.821Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
CMakeLists.txt (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
🪛 LanguageTool
README.md
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ... link options for your target. * lib_1, lib_2, ..., lib_N are the names of ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
5-5: Multiple top-level headings in the same document
null
(MD025, single-title, single-h1)
29-29: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
33-33: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
38-38: Multiple top-level headings in the same document
null
(MD025, single-title, single-h1)
🔇 Additional comments (18)
README.md (2)
7-8: Clear and accurate installation workflow instructions.The updated instructions correctly reflect the new CMake package-based approach, requiring users to build and install ystdlib first before using
find_package().
11-16: Consistent variable naming and improved CMake usage examples.The examples correctly demonstrate the new
find_package(ystdlib REQUIRED)approach and theystdlib_LIBRARIESvariable for selective library building, which aligns with the updated build system.Also applies to: 90-97
cmake/ystdlib-helpers.cmake (4)
7-14: Well-implemented argument validation macro.The
check_required_arguments_existmacro properly validates required arguments and provides clear error messages. The implementation correctly checks for both undefined and empty values.
21-32: Header detection function works correctly.The
check_if_header_onlyfunction properly identifies header files using a standard regex pattern and correctly sets return values viaPARENT_SCOPE.
106-114: FILE_SET HEADERS usage is correct for modern CMake.Based on retrieved learnings, using
PUBLICwithFILE_SET HEADERSon INTERFACE targets is valid in CMake 3.31+. The headers are treated as usage requirements, not compilation sources.
176-185: Clear and comprehensive function documentation.The documentation accurately describes the function parameters and provides helpful default values. The parameter descriptions are clear and follow a consistent format.
CMakeLists.txt (12)
1-1: Ensure compatibility with new CMake minimum version.Raising the minimum required CMake version to 3.23 is a breaking change for users on older platforms like Ubuntu Jammy. Please confirm this requirement is documented in the README and aligned with the PR’s breaking-change indicator in the title.
3-3: Validate initial project version.The project version is set to 0.1.0, which aligns with the introduced public API. Ensure this aligns with your release policy and update any documentation or badges that display the version.
5-8: Confirm consistent inclusion of helper modules.Including
CMakePackageConfigHelpers,GNUInstallDirs, andystdlib-helpersin alphabetical order under the updatedcmakedirectory path is clear and intentional. No issues found.
11-11: Consistent CMake option naming.Renaming the testing option to
ystdlib_BUILD_TESTINGfollows the established lowercase prefix convention and improves consistency. No changes needed.
41-48: Introduce configurable library list.Defining
ystdlib_LIBRARIESas a semicolon-separated cache variable provides flexible control over which components to build. Implementation looks correct.
50-53: Display selected libraries.The status messages clearly list the libraries to build, improving user feedback. No concerns here.
65-67: Enable unified testing control.The combined
BUILD_TESTINGandystdlib_BUILD_TESTINGconditions correctly drive test generation and follow CTest conventions.
69-72: Update Catch2 requirement.Specifying
Catch2 3.8.0explicitly ensures compatibility with the discovery API. The subsequent inclusion ofCatchis correct. No issues.
93-93: Define package configuration install directory.
ystdlib_INSTALL_CONFIG_DIRis correctly set to${CMAKE_INSTALL_LIBDIR}/cmake/ystdlib, aligning with standard GNUInstallDirs conventions.
95-97: Configure package config file paths.Setting the
CONFIG_FILE_PREFIXand calculatingCONFIG_OUTPUT_PATHandCONFIG_VERSION_OUTPUT_PATHare straightforward and clear.
104-105: Generate version file for package.Using
write_basic_package_version_filewithSameMajorVersioncompatibility is correct for downstream version checks.
106-111: Install package configuration files.Installing both the config and version files into
ystdlib_INSTALL_CONFIG_DIRensures properfind_packagesupport. This matches CMake best practices.
|
|
||
| An open-source C++ library developed and used at YScope. | ||
|
|
||
| # Usage |
There was a problem hiding this comment.
Fix multiple H1 headings to improve document structure.
The document has multiple top-level headings which violates markdown standards (MD025). Change the secondary sections to use ## instead of #.
-# Usage
+## Usage-# Contributing
+## ContributingAlso applies to: 38-38
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Multiple top-level headings in the same document
null
(MD025, single-title, single-h1)
🤖 Prompt for AI Agents
In README.md at lines 5 and 38, there are multiple top-level headings (#) which
violates markdown standards. Change these headings from single # to double ## to
make them secondary headings and improve document structure.
| > [!TIP] | ||
| > If ystdlib is not installed to a path that is searched by default, set `ystdlib_ROOT` to manually | ||
| > specify the location: | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove trailing space.
Line 33 has a trailing space that should be removed (MD009).
->
+>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > | |
| > |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
33-33: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
In README.md at line 33, there is a trailing space at the end of the line.
Remove the trailing space to comply with markdown linting rule MD009.
| > [!NOTE] | ||
| > If `BUILD_TESTING` is `ON`, set `ystdlib_BUILD_TESTING` to `OFF` to skip building ystdlib's unit | ||
| > tests. | ||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove blank line inside blockquote.
The blank line inside the blockquote violates markdown formatting standards (MD028).
> [!NOTE]
> If `BUILD_TESTING` is `ON`, set `ystdlib_BUILD_TESTING` to `OFF` to skip building ystdlib's unit
-> tests.
+> tests.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
29-29: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
In README.md at line 29, there is a blank line inside a blockquote which
violates markdown formatting standards (MD028). Remove the blank line so that
the blockquote content is continuous without any empty lines inside it.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
src/ystdlib/wrapped_facade_headers/CMakeLists.txt (1)
5-5: Address potential system header name clash.Installing
sys/types.hunder your include prefix risks shadowing the system<sys/types.h>header, which could lead to unexpected behaviour in consuming projects. Consider renaming this facade header to avoid conflicts with the POSIX standard header.cmake/ystdlib-helpers.cmake (2)
158-164: Consider multi-config generator compatibility.The hard-coded
RUNTIME_OUTPUT_DIRECTORYmay not work correctly with multi-config generators like Visual Studio or Xcode. Consider using generator expressions for better compatibility.- PROPERTY - RUNTIME_OUTPUT_DIRECTORY - ${CMAKE_BINARY_DIR}/testbin + PROPERTY + RUNTIME_OUTPUT_DIRECTORY + "${CMAKE_BINARY_DIR}/$<CONFIG>/testbin"
21-32: Header file detection could be more comprehensive.The regex pattern
.*\\.(h|hpp)may miss some common header file extensions and is case-sensitive. Consider expanding it to handle variants like.hh,.hxx, or uppercase extensions.- if(NOT "${SRC_FILE}" MATCHES ".*\\.(h|hpp)") + if(NOT "${SRC_FILE}" MATCHES ".*\\.[Hh](pp|xx|h)?$")CMakeLists.txt (5)
14-19: Redundant compiler blocks still present
The duplicatedAppleClangvsClangversion logic noted in the earlier review remains unchanged. Consolidating them into a single regex match would remove duplication and keep the minimum version in one place.
26-31: Back-slash continuation still hurts readability
Previous feedback to collapse this multi-linemessage(FATAL_ERROR …)into a single line hasn’t been addressed.
55-63:ystdlib_IS_TOP_LEVELguard still uninitialised
The earlier issue—variable may be undefined when included as a sub-project—has not been fixed. Initialise it before use to avoid silently skipping dependency setup.
88-90:BOOST_MIN_VERSIONis still dead code
This variable is set but never referenced (nofind_package(Boost …)or comparison). Either wire it intofind_packageor delete it.
105-110:configure_package_config_filestill missingPATH_VARS
WithoutPATH_VARS ystdlib_INSTALL_CONFIG_DIR, absolute build paths are baked into the generated config file—breaking relocatable installs. The prior suggestion has not been incorporated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
CMakeLists.txt(2 hunks)cmake/ystdlib-helpers.cmake(1 hunks)cmake/ystdlib/libs/containers-config.cmake.in(1 hunks)cmake/ystdlib/libs/error_handling-config.cmake.in(1 hunks)cmake/ystdlib/libs/io_interface-config.cmake.in(1 hunks)cmake/ystdlib/libs/wrapped_facade_headers-config.cmake.in(1 hunks)cmake/ystdlib/ystdlib-config.cmake.in(1 hunks)src/ystdlib/containers/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/CMakeLists.txt(2 hunks)src/ystdlib/io_interface/CMakeLists.txt(2 hunks)src/ystdlib/wrapped_facade_headers/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
cmake/ystdlib-helpers.cmake (2)
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:05:45.088Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:02:19.821Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
CMakeLists.txt (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-ystdlib-cpp (ubuntu-24.04)
- GitHub Check: build-ystdlib-cpp (macos-14)
- GitHub Check: build-ystdlib-cpp (ubuntu-22.04)
- GitHub Check: build-ystdlib-cpp (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (14)
cmake/ystdlib/libs/containers-config.cmake.in (1)
1-6: LGTM! Clean and consistent implementation.This config file follows CMake best practices with proper target guards and modular design. The implementation correctly prevents double inclusion and includes the target-specific configuration file.
cmake/ystdlib/libs/error_handling-config.cmake.in (1)
1-7: LGTM! Proper dependency handling for Boost.This config file correctly uses
find_dependencyto handle the Boost requirement, which is the standard CMake approach for package dependencies. The template variable substitution for@BOOST_FIND_PACKAGE_ARGS@allows for flexible Boost configuration.cmake/ystdlib/libs/wrapped_facade_headers-config.cmake.in (1)
1-6: LGTM! Consistent with established pattern.This config file follows the same clean pattern as the containers config file, maintaining consistency across the library configuration system.
cmake/ystdlib/libs/io_interface-config.cmake.in (1)
1-10: LGTM! Good dependency management structure.The overall structure correctly handles the dependency chain with proper target guards and includes the necessary files in the right order.
cmake/ystdlib/ystdlib-config.cmake.in (3)
1-3: LGTM! Standard CMake package initialization.The includes and package initialization follow CMake best practices for package configuration files.
5-10: LGTM! Proper dynamic library loading.The iteration over the library list and conditional inclusion of config files is implemented correctly. This allows for flexible library selection while ensuring each library's config is loaded only once.
12-12: LGTM! Proper component validation.The
check_required_componentscall ensures that all required components are properly loaded, which is essential for package configuration.src/ystdlib/containers/CMakeLists.txt (1)
1-22: Well-structured CMake modernization.The new structure effectively implements the standardized build pattern with appropriate guards, conditional testing, and proper installation setup. The early exit pattern prevents duplicate target creation, and the helper function usage ensures consistency across libraries.
src/ystdlib/io_interface/CMakeLists.txt (2)
5-10: Excellent dependency management pattern.The conditional inclusion of
wrapped_facade_headersbased on theystdlib_LIBRARIESlist is a clean approach that prevents unnecessary builds while ensuring required dependencies are available.
12-23: Library configuration is well-structured.The
add_cpp_librarycall properly specifies headers, private sources, and public link libraries. The dependency onystdlib::wrapped_facade_headersis correctly declared.src/ystdlib/error_handling/CMakeLists.txt (2)
14-24: Library configuration follows standard pattern.The
add_cpp_librarycall is properly structured with appropriate public headers and Boost dependency linkage.
5-12: Verify that BOOST_MIN_VERSION variable is defined.The code references
${BOOST_MIN_VERSION}but this variable may not be defined in the current scope. Ensure this variable is properly set in the parent CMakeLists.txt or consider using a fallback value.#!/bin/bash # Check if BOOST_MIN_VERSION is defined in the project rg -A 3 -B 3 "BOOST_MIN_VERSION" --type cmakecmake/ystdlib-helpers.cmake (1)
107-115: FILE_SET HEADERS usage is correct for modern CMake.Based on retrieved learnings, using
target_sources()withPUBLICandFILE_SET HEADERSon INTERFACE targets is valid in CMake 3.31+ as these are treated as usage requirements rather than compilation sources.CMakeLists.txt (1)
1-3: Document the CMake 3.23 requirement up-front
Jumping from the previous minimum (3.22.x) to 3.23 can break users on LTS distributions (e.g., Ubuntu 22.04 ships 3.22.1). Please highlight this bump in the changelog / release notes and in the README’s “Prerequisites” section so downstream projects aren’t caught off-guard.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
cmake/ystdlib/libs/io_interface-config.cmake.in(1 hunks)
🔇 Additional comments (2)
cmake/ystdlib/libs/io_interface-config.cmake.in (2)
5-7: Dependency include path corrected—nice catchSwitching to
wrapped_facade_headers-config.cmakeresolves the dependency-lookup inconsistency flagged earlier. No further issues here.
9-9: Target file inclusion looks correctIncluding
io_interface-target.cmakeafter dependencies guarantees the imported target is fully populated. All good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/ystdlib/io_interface/CMakeLists.txt (1)
28-28: Add safety check forUNIFIED_UNIT_TEST_TARGETvariable.The test registration relies on the
UNIFIED_UNIT_TEST_TARGETvariable being defined. Consider adding a fallback or sanity check to ensure this CMakeLists can be used independently without errors.src/ystdlib/wrapped_facade_headers/CMakeLists.txt (1)
1-1: Address potential system header name clash.Installing
sys/types.hunder your own include prefix risks shadowing the system<sys/types.h>on the consumer's include path. Consider renaming the facade header or adjusting the installation path to avoid collision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
cmake/ystdlib/libs/containers-config.cmake.in(1 hunks)cmake/ystdlib/libs/error_handling-config.cmake.in(1 hunks)cmake/ystdlib/libs/io_interface-config.cmake.in(1 hunks)cmake/ystdlib/libs/wrapped_facade_headers-config.cmake.in(1 hunks)src/ystdlib/containers/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/CMakeLists.txt(2 hunks)src/ystdlib/io_interface/CMakeLists.txt(2 hunks)src/ystdlib/wrapped_facade_headers/CMakeLists.txt(1 hunks)
🔇 Additional comments (11)
src/ystdlib/containers/CMakeLists.txt (2)
3-11: Make test source path independent of the CMake invocation directory
test/test_Array.cppis interpreted relative to the current build directory, which varies when consumers add this sub-directory viaadd_subdirectoryor when packaging. Using${CMAKE_CURRENT_SOURCE_DIR}avoids brittle include failures on some generators and external builds.- test/test_Array.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test/test_Array.cppAlso ensure
UNIFIED_UNIT_TEST_TARGETis defined in every code path whereystdlib_ENABLE_TESTSis true, otherwise configuration will fail.
13-18: Verify helper variables exist in every inclusion context
CONFIG_LIBS_DEST_DIRandCONFIG_LIBS_INPUT_DIRare referenced here but are not obviously scoped in this file. If a consumer project callsfind_package(ystdlib)and brings this directory in through an installed config, these variables must still resolve or the installation step will mis-configure.Double-check that
CMake/ystdlib-helpers.cmake(or the top-levelCMakeLists.txt) sets sensible defaults before anyinstall_library()invocation.If not, consider:
set(CONFIG_LIBS_DEST_DIR "${CMAKE_INSTALL_LIBDIR}/cmake/ystdlib" CACHE PATH "…") set(CONFIG_LIBS_INPUT_DIR "${PROJECT_SOURCE_DIR}/cmake" CACHE PATH "…")cmake/ystdlib/libs/wrapped_facade_headers-config.cmake.in (1)
1-4: Clean and consistent config file implementation.The include guard and target inclusion pattern follows CMake best practices. The implementation is correct and consistent with the modular config system design.
cmake/ystdlib/libs/io_interface-config.cmake.in (1)
1-6: Proper dependency handling and include guard implementation.The config file correctly uses
include_guard(GLOBAL)and includes the dependency config file (wrapped_facade_headers-config.cmake) rather than the target file directly. This addresses previous review concerns and follows CMake best practices.cmake/ystdlib/libs/containers-config.cmake.in (1)
1-4: Consistent and well-structured config file.The implementation follows the established pattern with proper include guard and target inclusion. It's consistent with the other library config files in the project.
cmake/ystdlib/libs/error_handling-config.cmake.in (1)
1-6: Well-structured CMake package configuration template.The implementation follows CMake best practices with proper include guard, dependency management using
find_dependency, and target inclusion. The template variable substitution approach is standard and will work correctly during installation.src/ystdlib/error_handling/CMakeLists.txt (5)
1-4: Good practice setting CMake policy for Boost configuration.Setting CMP0167 to "NEW" correctly ensures the modern BoostConfig.cmake package configuration is preferred, which aligns well with the new build system approach.
14-24: Excellent migration to the new library helper function.The transition from the old library creation approach to
add_cpp_libraryis well-executed. The namespace, headers, and dependencies are properly configured for the new modular build system.
26-37: Proper conditional test setup with new helper function.The test configuration correctly uses the new
add_catch2_testshelper function and is properly guarded by theystdlib_ENABLE_TESTSflag. Good to see the header files are no longer included in the SOURCES list, addressing the previous review concern.
39-44: Well-configured library installation setup.The
install_librarycall correctly specifies the namespace, configuration directories, and follows the new installation pattern established in the restructured build system.
5-12: Verify BOOST_MIN_VERSION variable is defined in parent scope.The Boost dependency setup looks good, but ensure that
BOOST_MIN_VERSIONis properly defined in the root CMakeLists.txt or parent scope, as this variable name differs from previous references toMIN_BOOST_VERSIONin past comments.#!/bin/bash # Description: Verify BOOST_MIN_VERSION variable is defined and used consistently # Expected: Find definition of BOOST_MIN_VERSION in root CMakeLists.txt or parent files echo "Searching for BOOST_MIN_VERSION definition:" rg -A 3 -B 3 "BOOST_MIN_VERSION.*=" echo -e "\nSearching for all BOOST_MIN_VERSION usage:" rg "BOOST_MIN_VERSION" echo -e "\nSearching for any MIN_BOOST_VERSION references:" rg "MIN_BOOST_VERSION"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmake/ystdlib-helpers.cmake (3)
24-29: Regex still too narrow & case-sensitiveThe header-detection pattern only matches
.h/.hppin lowercase. Upper-case extensions and common variants like.hh,.hxx, or mixed-case names will be treated as “source” files and mark a header-only library as non-interface.A broader, case-insensitive regex was suggested previously and remains unaddressed.
162-163: Hard-codedRUNTIME_OUTPUT_DIRECTORYbreaks multi-config generatorsSingle-config paths work, but Visual Studio/Xcode expect
RUNTIME_OUTPUT_DIRECTORY_<CONFIG>.
Wrap the path in a$<CONFIG>generator expression or set the per-config property instead.
202-203: Install rule still emits warnings for INTERFACE libraries
install(TARGETS … LIBRARY ARCHIVE RUNTIME …)is executed unconditionally.
For header-only (INTERFACE) targets this produces “no artifacts to install” warnings.
Guard the call withif(NOT TARGET_PROPERTY STREQUAL "INTERFACE_LIBRARY")or split installation logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
cmake/ystdlib-helpers.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmake/ystdlib-helpers.cmake (2)
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:05:45.088Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
Learnt from: davidlion
PR: y-scope/ystdlib-cpp#65
File: cmake/ystdlib-helpers.cmake:105-113
Timestamp: 2025-06-16T16:02:19.821Z
Learning: In CMake, using target_sources() with PUBLIC and FILE_SET HEADERS on INTERFACE targets is valid and equivalent to using INTERFACE. FILE_SET HEADERS are treated as usage requirements, not compilation sources, so they don't cause errors on INTERFACE targets in modern CMake versions like 3.31.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
As discussed offline, let's also not quote string literals that don't contain spaces. Up to you if you want to leave that to a future PR since it is a bit nitpick-y at this point.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
CMakeLists.txt (5)
55-63: Initialiseystdlib_IS_TOP_LEVELbefore evaluation
ystdlib_IS_TOP_LEVELis referenced here without a priorset().
When this file is included as a sub-project the variable is undefined, so the branch is always skipped andCTestis never enabled, potentially masking tests and dependency logic.+# Safely determine if we are the root project +if(NOT DEFINED ystdlib_IS_TOP_LEVEL) + set(ystdlib_IS_TOP_LEVEL + "${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") +endif() + if(ystdlib_IS_TOP_LEVEL)
41-48: Expose allowed library names to CMake-GUINice touch exposing
ystdlib_LIBRARIESas a cache variable.
AddSTRINGSso users get a drop-down inccmake/CMake-GUI:set_property(CACHE ystdlib_LIBRARIES PROPERTY STRINGS containers error_handling io_interface wrapped_facade_headers)
88-90:BOOST_MIN_VERSIONis dead codeThe variable is set but never referenced (Boost is not even located).
Delete it or wire it intofind_package(Boost …); otherwise it only clutters the cache.-set(BOOST_MIN_VERSION "1.81.0")
105-110: AddPATH_VARSto avoid absolute paths in generated configWithout
PATH_VARS,configure_package_config_file()bakes absolute build-time paths into the installedystdlib-config.cmake, breaking relocatability.configure_package_config_file( "${CMAKE_CURRENT_LIST_DIR}/${CONFIG_PATH_SUFFIX}/${CONFIG_FILE_PREFIX}.cmake.in" "${CONFIG_OUTPUT_PATH}" + PATH_VARS CONFIG_INSTALL_DIR INSTALL_DESTINATION "${CONFIG_INSTALL_DIR}" )Make sure the template refers to
@CONFIG_INSTALL_DIR@.
29-31: Collapse the multiline fatal-error messageThe back-slash continuation is unnecessary and hurts readability.
- "${CMAKE_CXX_COMPILER_ID} version ${CMAKE_CXX_COMPILER_VERSION} is too low. Must be at \ - least ${ystdlib_CMAKE_CXX_COMPILER_MIN_VERSION}." + "${CMAKE_CXX_COMPILER_ID} version ${CMAKE_CXX_COMPILER_VERSION} " + "is too low. Must be at least ${ystdlib_CMAKE_CXX_COMPILER_MIN_VERSION}."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
CMakeLists.txt (1)
Learnt from: PacificViking
PR: y-scope/ystdlib-cpp#65
File: CMakeLists.txt:87-88
Timestamp: 2025-06-06T15:01:53.987Z
Learning: The y-scope/ystdlib-cpp project uses lowercase `ystdlib_` prefix for project-specific CMake variables (e.g., `ystdlib_VERSION`, `ystdlib_BUILD_TESTING`, `ystdlib_INSTALL_CONFIG_DIR`) following the current naming convention standards.
| if(BUILD_TESTING AND ystdlib_BUILD_TESTING) | ||
| set(ystdlib_ENABLE_TESTS ON) | ||
| endif() | ||
|
|
||
| find_package(Boost REQUIRED) | ||
| if(Boost_FOUND) | ||
| message(STATUS "Found Boost ${Boost_VERSION}.") | ||
| endif() | ||
|
|
||
| if(YSTDLIB_CPP_ENABLE_TESTS) | ||
| if(ystdlib_ENABLE_TESTS) | ||
| find_package(Catch2 3.8.0 REQUIRED) | ||
| if(Catch2_FOUND) | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}.") | ||
| else() | ||
| message(FATAL_ERROR "Could not find libraries for Catch2.") | ||
| endif() | ||
| message(STATUS "Found Catch2 ${Catch2_VERSION}.") | ||
| include(Catch) |
There was a problem hiding this comment.
Guard use of BUILD_TESTING when the project is a sub-directory
BUILD_TESTING is only defined after a call to include(CTest).
If this project is embedded via add_subdirectory and ystdlib_IS_TOP_LEVEL is false, include(CTest) is never executed, making BUILD_TESTING unknown here and triggering a CMake error on some policies.
Add a default:
if(NOT DEFINED BUILD_TESTING)
set(BUILD_TESTING OFF)
endif()before the if(BUILD_TESTING AND ystdlib_BUILD_TESTING) check.
This keeps the logic safe in all embedding scenarios.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 65 to 72, the variable BUILD_TESTING may be
undefined if the project is included as a subdirectory without calling
include(CTest), causing CMake errors. To fix this, add a check before the
existing if statement to set BUILD_TESTING to OFF if it is not defined. This
ensures the subsequent conditional logic using BUILD_TESTING is safe in all
embedding scenarios.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build(cmake)!: Add support for installation and usage as a library in other CMake projects; Add example project to test this use case.
davidlion
changed the title
Description
Previously, users of ystdlib needed to use it through
add_subdirectory. This PR updates ystdlib to allow installing selected ystdlib libraries and using them throughfind_package.When building ystdlib the user can define the CMake variable
ystdlib_LIBRARIESwith a semicolon separated list of ystdlib library names to build (if omitted all libraries are built). Then whencmake --installis run all built libraries will be installed and configured to work withfind_package.For
ystdlib_LIBRARIES, if a user lists a ystdlib library that depends on another ystdlib not listed that dependent library will be added to the list and built.This PR also adds an
examples/directory containing an example CMake project to:Support for using
COMPONENTSwithfind_packageto selectively depend on (installed) libraries is deferred to a future PR.This PR sets the minimum version of CMake to 3.23 to add support for
FILE_SETS.Checklist
breaking change.
Validation performed
Test cases still pass. Example builds and passes by manually running the following commands: