build: use CPM for dependencies#373
Open
Daan Timmer (daantimmer) wants to merge 18 commits into
Open
Conversation
❌MegaLinter analysis: Error
Detailed Issues❌ ACTION / actionlint - 1 error❌ ACTION / zizmor - 20 errors
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s third-party dependency management from ad-hoc FetchContent in external/** to a centralized CPM-based flow with a committed lockfile, and updates the devcontainer to reuse a CPM source cache.
Changes:
- Removed per-dependency
FetchContent_Declare/MakeAvailableCMake snippets underexternal/**. - Added
cmake/dependencies.cpm.cmake+cmake/package-lock.cmakeand wired them into the top-levelCMakeLists.txt. - Added
CPM_SOURCE_CACHEto the devcontainer environment.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Switches dependency setup to CPM script and simplifies module include usage. |
cmake/dependencies.cpm.cmake |
New centralized CPM dependency configuration and conditional find/fetch logic. |
cmake/package-lock.cmake |
New CPM lockfile pinning dependency versions/commits/options. |
.devcontainer/devcontainer.json |
Sets CPM cache location for faster devcontainer rebuilds. |
external/CMakeLists.txt |
Stops adding most third-party subdirectories; only keeps tobiaslocker. |
external/cliutils/CMakeLists.txt |
Removes subdir wiring for CLI11. |
external/cliutils/cli11/CMakeLists.txt |
Removes CLI11 FetchContent logic. |
external/cucumber/CMakeLists.txt |
Removes subdir wiring for messages/gherkin. |
external/cucumber/messages/CMakeLists.txt |
Removes cucumber/messages FetchContent and build wiring. |
external/cucumber/gherkin/CMakeLists.txt |
Removes cucumber/gherkin FetchContent and target property adjustments. |
external/fmtlib/CMakeLists.txt |
Removes subdir wiring for fmt. |
external/fmtlib/fmt/CMakeLists.txt |
Removes fmt FetchContent logic. |
external/google/CMakeLists.txt |
Removes wiring for abseil/re2/googletest. |
external/google/abseil/CMakeLists.txt |
Removes abseil FetchContent logic. |
external/google/re2/CMakeLists.txt |
Removes re2 FetchContent logic. |
external/google/googletest/CMakeLists.txt |
Removes googletest FetchContent logic and warning configuration. |
external/jbeder/CMakeLists.txt |
Removes subdir wiring for yaml-cpp. |
external/jbeder/yaml-cpp/CMakeLists.txt |
Removes yaml-cpp FetchContent logic. |
external/nlohmann/CMakeLists.txt |
Removes subdir wiring for nlohmann/json. |
external/nlohmann/json/CMakeLists.txt |
Removes nlohmann/json FetchContent logic. |
external/zeux/CMakeLists.txt |
Removes subdir wiring for pugixml. |
external/zeux/pugixml/CMakeLists.txt |
Removes pugixml FetchContent logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+9
to
+24
| # renovate: datasource=github-tags packageName=cucumber/gherkin versioning=semver | ||
| ccr_dependency(CUCUMBER_GHERKIN v42.0.0 f2f7b51ae6a886aefc1f4a2059e4989b83aab8c8) | ||
| # renovate: datasource=github-tags packageName=google/googletest versioning=semver | ||
| ccr_dependency(GOOGLE_TEST v1.17.0 52eb8108c5bdec04579160ae17225d66034bd723) | ||
| # renovate: datasource=github-tags packageName=CLIUtils/CLI11 versioning=semver | ||
| ccr_dependency(CLI11 v2.6.2 37bb6edc5317e99af72ef48405e65d9ca5218861) | ||
| # renovate: datasource=github-tags packageName=fmtlib/fmt versioning=semver | ||
| ccr_dependency(LIBFMT v12.1.0 407c905e45ad75fc29bf0f9bb7c5c2fd3475976f) | ||
| # renovate: datasource=github-tags packageName=zeux/pugixml versioning=semver | ||
| ccr_dependency(PUGIXML v1.15 ee86beb30e4973f5feffe3ce63bfa4fbadf72f38) | ||
| # renovate: datasource=github-tags packageName=abseil/abseil-cpp | ||
| ccr_dependency(ABSEIL_CPP 20250814.2 0cf0a5c9d12cc3783363ab20f11613e69fd04c9a) | ||
| # renovate: datasource=github-tags packageName=google/re2 | ||
| ccr_dependency(RE2 2025-08-12 0f6c07eae69151e606acb3d9232750c3442dff23) | ||
| # renovate: datasource=github-tags packageName=jbeder/yaml-cpp versioning=semver | ||
| ccr_dependency(YAML_CPP v0.9.0 56e3bb550c91fd7005566f19c079cb7a503223cf) |
Comment on lines
+53
to
+63
| # --------------------------------------------------------------------------- | ||
| # cucumber_gherkin | ||
| # --------------------------------------------------------------------------- | ||
| CPMAddPackage( | ||
| URI "gh:cucumber/gherkin@${CUCUMBER_GHERKIN_VERSION}#${CUCUMBER_GHERKIN_DIGEST}" | ||
| NAME cucumber_gherkin | ||
| SOURCE_SUBDIR cpp | ||
| OPTIONS | ||
| "CUCUMBER_GHERKIN_FETCH_DEPS ON" | ||
| ) | ||
|
|
Comment on lines
+138
to
+142
| find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED) | ||
| find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED) | ||
| find_package(pugixml ${PUGIXML_VERSION} REQUIRED) | ||
| find_package(fmt ${LIBFMT_VERSION} REQUIRED) | ||
|
|
| } | ||
| }, | ||
| "remoteEnv": { | ||
| "CPM_SOURCE_CACHE": "~/cache/.cpm" |
| option(CCR_ENABLE_COVERAGE "Enable compiler flags for code coverage measurements" Off) | ||
| option(CCR_ENABLE_TIME_PROFILE "Enable compiler flags for time profiling measurements" Off) | ||
|
|
||
| set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS}) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
cmake/dependencies.cpm.cmake:14
- cmake/dependencies.cpm.cmake uses (and should version-pin) nlohmann_json, but there is no NLOHMANN_JSON dependency definition alongside the other pinned deps. This makes it awkward to add a matching CPMAddPackage/find_package later and risks drifting from the package-lock pin.
ccr_dependency(CLI11 v2.6.2 37bb6edc5317e99af72ef48405e65d9ca5218861)
cmake/dependencies.cpm.cmake:85
- The build links against nlohmann_json::nlohmann_json (e.g. cucumber_cpp/library/formatter/CMakeLists.txt), but when CCR_FETCH_DEPS=ON this file never calls CPMAddPackage for nlohmann_json. This will fail at configure/generate time unless some other dependency happens to bring in the target.
CPMAddPackage(
URI "gh:CLIUtils/CLI11@${CLI11_VERSION}#${CLI11_DIGEST}"
NAME cli11
)
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS=OFF, this script doesn’t locate CLI11 or nlohmann_json, but the project links against CLI11::CLI11 and nlohmann_json::nlohmann_json. This will break non-fetch builds that rely on system-installed packages.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
.devcontainer/devcontainer.json:18
- "
" typically won’t be expanded in environment variables; CPM_SOURCE_CACHE may end up pointing to a literal "/cache/.cpm" path. Use $HOME via devcontainer variable substitution or an absolute path so caching works reliably.
"CPM_SOURCE_CACHE": "~/cache/.cpm"
Comment on lines
+132
to
+135
| NAME yaml-cpp | ||
| OPTION | ||
| "YAML_ENABLE_PIC OFF" | ||
| ) |
Comment on lines
+102
to
+110
| CPMDeclarePackage(yaml-cpp | ||
| NAME yaml-cpp | ||
| VERSION 0.9.0 | ||
| GIT_TAG 56e3bb550c91fd7005566f19c079cb7a503223cf | ||
| GITHUB_REPOSITORY jbeder/yaml-cpp | ||
| SYSTEM YES | ||
| EXCLUDE_FROM_ALL YES | ||
| OPTION YAML_ENABLE_PIC OFF | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (5)
cmake/dependencies.cpm.cmake:135
- In the yaml-cpp CPMAddPackage call, the keyword is
OPTIONbut CPM expectsOPTIONS(plural). As written, the YAML_ENABLE_PIC setting is likely ignored, changing the build compared to the previous FetchContent setup.
CPMAddPackage(
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
)
cmake/dependencies.cpm.cmake:141
- When
CCR_FETCH_DEPSis OFF, this file no longer finds CLI11 or nlohmann_json, but project targets link toCLI11::CLI11(cucumber_cpp/library/CMakeLists.txt:28) andnlohmann_json::nlohmann_json(cucumber_cpp/library/formatter/CMakeLists.txt:33). CMake configuration will fail unless those targets are provided implicitly by another package.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:85
- The build links against
nlohmann_json::nlohmann_json, butCCR_FETCH_DEPS=ONdoes not explicitly add nlohmann_json via CPM here. Relying on transitive dependency setup from another package is fragile and can break if upstream changes.
CPMAddPackage(
URI "gh:CLIUtils/CLI11@${CLI11_VERSION}#${CLI11_DIGEST}"
NAME cli11
)
cmake/package-lock.cmake:110
- In the yaml-cpp lock entry,
OPTION YAML_ENABLE_PIC OFFis not valid CPMDeclarePackage syntax (CPM uses anOPTIONSlist). This likely means YAML_ENABLE_PIC is not applied when using the lock file.
SYSTEM YES
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
CMakeLists.txt:26
CUCUMBER_CPP_FETCH_DEPSis set here but is not referenced anywhere else in the repository, so it currently has no effect and can confuse readers about the dependency flow.
set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS})
| key: ${{ github.job }}-${{ matrix.target }} | ||
| max-size: 2G | ||
| path: ${{ env.CPM_SOURCE_CACHE }} | ||
| key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }} |
| uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 | ||
| with: | ||
| path: ${{ env.CPM_SOURCE_CACHE }} | ||
| key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }} |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
cmake/dependencies.cpm.cmake:57
- When CCR_FETCH_DEPS is enabled, this script never adds nlohmann_json or cucumber_messages, but the project links against nlohmann_json::nlohmann_json and includes cucumber/messages headers. This will break fresh configures where those targets aren’t already provided transitively.
# ---------------------------------------------------------------------------
# cucumber_gherkin
# ---------------------------------------------------------------------------
CPMAddPackage(
URI "gh:cucumber/gherkin@${CUCUMBER_GHERKIN_VERSION}#${CUCUMBER_GHERKIN_DIGEST}"
cmake/dependencies.cpm.cmake:142
- When CCR_FETCH_DEPS is OFF, required dependencies aren’t located: CLI11 (used by cucumber_cpp.library), nlohmann_json (used by formatter), and cucumber_messages (headers used throughout) are never found, so consumers relying on system packages will fail to configure/build.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:23
- This file references NLOHMANN_JSON_* and CUCUMBER_MESSAGES_* variables (needed to add those deps), but they aren’t declared alongside the other pinned dependency versions/digests.
# renovate: datasource=github-tags packageName=abseil/abseil-cpp
ccr_dependency(ABSEIL_CPP 20250814.2 0cf0a5c9d12cc3783363ab20f11613e69fd04c9a)
# renovate: datasource=github-tags packageName=google/re2
ccr_dependency(RE2 2025-08-12 0f6c07eae69151e606acb3d9232750c3442dff23)
# renovate: datasource=github-tags packageName=jbeder/yaml-cpp versioning=semver
cmake/dependencies.cpm.cmake:135
- CPMAddPackage uses the keyword OPTIONS (plural). Using OPTION here won’t apply YAML_ENABLE_PIC and may even be treated as an unknown argument depending on CPM version.
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
)
cmake/package-lock.cmake:110
- CPMDeclarePackage expects dependency options under OPTIONS (plural), typically as quoted "VAR value" strings. As written, YAML_ENABLE_PIC won’t be applied from the lock file.
SYSTEM YES
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
CMakeLists.txt:26
- CUCUMBER_CPP_FETCH_DEPS is set but never read anywhere in the repo. Keeping it around suggests there’s a toggle that doesn’t actually do anything, which makes the build logic harder to reason about.
set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS})
.github/workflows/ci.yml:60
- The CPM cache key doesn’t include cmake/package-lock.cmake, so updating the lock file won’t invalidate the cache. This can lead to stale sources being restored across dependency updates.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:135
- Same as above: this CPM cache key ignores cmake/package-lock.cmake, so dependency lock updates won’t invalidate the restored CPM_SOURCE_CACHE.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
Comment on lines
+35
to
+40
| message(STATUS "Downloading CPM.cmake ${CPM_DOWNLOAD_VERSION}…") | ||
| file(DOWNLOAD | ||
| "https://github.com/cpm-cmake/CPM.cmake/releases/download/${CPM_DOWNLOAD_TAG}/CPM.cmake" | ||
| "${CPM_DOWNLOAD_LOCATION}" | ||
| TLS_VERIFY ON | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
cmake/dependencies.cpm.cmake:56
- When CCR_FETCH_DEPS is ON, this file never adds/locks
nlohmann_jsonorcucumber_messages, but the build linksnlohmann_json::nlohmann_json(cucumber_cpp/library/formatter/CMakeLists.txt:33) and includescucumber/messages/*headers (e.g. compatibility/BaseCompatibility.cpp:2). This will fail at configure/build time because the targets/headers are unavailable.
# ---------------------------------------------------------------------------
# cucumber_gherkin
# ---------------------------------------------------------------------------
CPMAddPackage(
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS is OFF, the configure path no longer calls
find_package()forCLI11,nlohmann_json, orcucumber_messages, but the project linksCLI11::CLI11(cucumber_cpp/library/CMakeLists.txt:28) andnlohmann_json::nlohmann_json(cucumber_cpp/library/formatter/CMakeLists.txt:33) and includes cucumber messages headers. Add the missing packages to the non-fetch path so the build works without CPM.
else()
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
.github/workflows/ci.yml:60
- The CPM cache key only hashes
cmake/dependencies.cpm.cmake, but the actual pinned versions live incmake/package-lock.cmake. If the lock file changes, this cache will be incorrectly reused, potentially hiding dependency updates or causing hard-to-reproduce CI behavior.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:135
- Same issue as the devcontainer job: the CPM cache key doesn’t include
cmake/package-lock.cmake, so dependency version bumps in the lock file won’t invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
Comment on lines
+75
to
+76
| fmt::println("current path: {}", std::filesystem::current_path().string()); | ||
| fmt::println(" uri path: {}", uri); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
cmake/dependencies.cpm.cmake:93
- When CCR_FETCH_DEPS is enabled, this file fetches fmt and pugixml via CPM but never fetches nlohmann_json, even though the build links against nlohmann_json::nlohmann_json (see cucumber_cpp/library/formatter/CMakeLists.txt:33). This will fail at configure/link time unless another dependency happens to create that target.
CPMAddPackage(
URI "gh:fmtlib/fmt@${LIBFMT_VERSION}#${LIBFMT_DIGEST}"
NAME libfmt
)
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS is OFF, this branch no longer locates CLI11 and nlohmann_json, but the project links against CLI11::CLI11 (cucumber_cpp/library/CMakeLists.txt:28) and nlohmann_json::nlohmann_json (cucumber_cpp/library/formatter/CMakeLists.txt:33). This will break consumers relying on system-installed dependencies.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:135
- CPMAddPackage uses the keyword "OPTIONS" elsewhere in this file, but yaml-cpp is configured with "OPTION". If CPM doesn’t recognize "OPTION" here, YAML_ENABLE_PIC will be ignored (changing binary compatibility/relocation behavior vs the previous FetchContent-based build).
CPMAddPackage(
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
)
cmake/package-lock.cmake:110
- In the CPM package lock, yaml-cpp uses "OPTION YAML_ENABLE_PIC OFF" while all other packages use an "OPTIONS" block. If CPM expects "OPTIONS" here, the lock will not apply YAML_ENABLE_PIC and builds may differ from local/CI expectations.
CPMDeclarePackage(yaml-cpp
NAME yaml-cpp
VERSION 0.9.0
GIT_TAG 56e3bb550c91fd7005566f19c079cb7a503223cf
GITHUB_REPOSITORY jbeder/yaml-cpp
SYSTEM YES
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
compatibility/BaseCompatibility.cpp:77
- These fmt::println calls add unconditional debug output during compatibility runs, which can clutter CI logs and interfere with tools that parse test output. Consider removing them or using a test-scoped tracing mechanism (e.g., SCOPED_TRACE) instead.
fmt::println("current path: {}", std::filesystem::current_path().string());
fmt::println(" uri path: {}", uri);
.github/workflows/ci.yml:61
- The CPM cache key here only hashes cmake/dependencies.cpm.cmake, but other jobs also hash cmake/package-lock.cmake. If the lock changes without changing dependencies.cpm.cmake, this job can reuse a stale CPM cache and produce inconsistent builds.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
CMakeLists.txt:27
- CUCUMBER_CPP_FETCH_DEPS is set but not referenced anywhere in this repository. Leaving unused cache/config variables around is confusing for consumers because it suggests a supported knob that has no effect.
set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS})
Comment on lines
15
to
17
| #include <cstddef> | ||
| #include <fmt/base.h> | ||
| #include <iterator> |
Comment on lines
+104
to
+105
| fmt::println("Registering step definition@{}", sourceLocation.file_name()); | ||
|
|
Comment on lines
15
to
19
| #include <chrono> | ||
| #include <filesystem> | ||
| #include <fmt/base.h> | ||
| #include <fmt/ostream.h> | ||
| #include <fstream> |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (6)
cmake/dependencies.cpm.cmake:85
- nlohmann_json is still linked by cucumber_cpp.library.formatter (nlohmann_json::nlohmann_json) but it is never provided here when CCR_FETCH_DEPS is enabled (external/nlohmann is no longer added either). This will make a fresh configure/build fail with an unknown target.
# ---------------------------------------------------------------------------
# cli11
# ---------------------------------------------------------------------------
CPMAddPackage(
URI "gh:CLIUtils/CLI11@${CLI11_VERSION}#${CLI11_DIGEST}"
NAME cli11
)
cmake/dependencies.cpm.cmake:142
- When CCR_FETCH_DEPS is Off, this file no longer calls find_package(CLI11) or find_package(nlohmann_json), but the build still links against CLI11::CLI11 and nlohmann_json::nlohmann_json (see cucumber_cpp/library/CMakeLists.txt and cucumber_cpp/library/formatter/CMakeLists.txt). This will break system-dependency builds.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:135
- CPMAddPackage uses the argument key "OPTIONS" (plural) elsewhere in this file; "OPTION" here is inconsistent and is not recognized by CPM.cmake, so the yaml-cpp option will be ignored or configuration may fail depending on CPM version.
CPMAddPackage(
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
)
cmake/package-lock.cmake:110
- CPMDeclarePackage expects options under the "OPTIONS" keyword (plural), consistent with the rest of this lock file. Using "OPTION" here can prevent YAML_ENABLE_PIC from being applied.
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
cucumber_cpp/library/support/StepRegistry.cpp:104
- Unconditional fmt::println during step registration will add unexpected output to normal runs and test output, and can break tools consuming stdout. Consider removing it or guarding behind an explicit debug/logging flag.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
compatibility/BaseCompatibility.cpp:76
- These debug prints add extra stdout output during compatibility processing, which can make CI logs noisy and can interfere with tests/tools that expect only NDJSON/structured output. Prefer an opt-in logger/trace flag or remove.
fmt::println("current path: {}", std::filesystem::current_path().string());
fmt::println(" uri path: {}", uri);
Comment on lines
+140
to
+142
| { | ||
| fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name()); | ||
| } |
| template<class T> | ||
| std::size_t DefinitionRegistration::Register(std::string_view matcher, StepType stepType, std::source_location sourceLocation) | ||
| { | ||
| fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name()); |
|
|
||
| std::size_t DefinitionRegistration::Register(std::string_view matcher, StepType stepType, util::StepFactory factory, std::source_location sourceLocation) | ||
| { | ||
| fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name()); |
… improve compatibility kit source inclusion
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (12)
cucumber_cpp/library/support/StepRegistry.hpp:142
- The Entry constructor now prints to stdout on every registration. This introduces a global side effect in a public header and can pollute test/output for consumers.
{
fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name());
}
cucumber_cpp/library/support/StepRegistry.cpp:104
- Unconditional fmt::println during step registration will spam stdout in normal runs/tests and makes behavior non-deterministic for tools that parse output.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
cucumber_cpp/library/support/StepRegistry.cpp:16
- If the debug println is removed, this fmt include is no longer needed.
#include <fmt/base.h>
cucumber_cpp/library/support/DefinitionRegistration.hpp:100
- This template Register overload prints to stdout during static registration, which will affect all consumers and makes output noisy/non-deterministic.
fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.cpp:79
- Unconditional fmt::println during definition registration will spam stdout in normal runs/tests.
fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name());
compatibility/BaseCompatibility.cpp:76
- These debug prints will add extra lines to the compatibility test output and can interfere with tooling that expects only test/framework output.
fmt::println("current path: {}", std::filesystem::current_path().string());
fmt::println(" uri path: {}", uri);
compatibility/BaseCompatibility.cpp:18
- If the debug printlns are removed, these fmt includes become unused.
#include <fmt/base.h>
#include <fmt/ostream.h>
cmake/dependencies.cpm.cmake:134
- CPMAddPackage uses the keyword OPTIONS (plural). Using OPTION here is likely ignored, so YAML_ENABLE_PIC may not be applied.
OPTION
"YAML_ENABLE_PIC OFF"
cmake/package-lock.cmake:109
- CPMDeclarePackage expects dependency configuration under OPTIONS (plural) as a list of strings.
OPTION YAML_ENABLE_PIC OFFis inconsistent with the rest of the lock file and is likely not applied.
OPTION YAML_ENABLE_PIC OFF
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS is Off, this file does not find CLI11 or nlohmann_json, but the project links to CLI11::CLI11 (cucumber_cpp/library/CMakeLists.txt:28) and nlohmann_json::nlohmann_json (cucumber_cpp/library/formatter/CMakeLists.txt:33). This will break non-fetch builds.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
.github/workflows/ci.yml:60
- The CPM cache key in this job does not include cmake/package-lock.cmake, so dependency updates in the lock file won't invalidate the cache and can cause confusing/stale builds.
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
CMakeLists.txt:26
- CUCUMBER_CPP_FETCH_DEPS is set here but does not appear to be used anywhere else in the repository, which can be confusing when trying to understand dependency behavior.
set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS})
| #include <cstdint> | ||
| #include <exception> | ||
| #include <filesystem> | ||
| #include <fmt/base.h> |
| #include "cucumber_cpp/library/util/HookFactory.hpp" | ||
| #include "cucumber_cpp/library/util/StepFactory.hpp" | ||
| #include <cstddef> | ||
| #include <fmt/base.h> |
| #include "cucumber_cpp/library/util/HookFactory.hpp" | ||
| #include "cucumber_cpp/library/util/StepFactory.hpp" | ||
| #include <cstddef> | ||
| #include <fmt/base.h> |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (15)
cmake/dependencies.cpm.cmake:141
- In the non-fetch (find_package) path, CLI11 is never located, but cucumber_cpp.library links against CLI11::CLI11 (cucumber_cpp/library/CMakeLists.txt:18-29). This makes CCR_FETCH_DEPS=Off configurations fail at generate time due to a missing imported target.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:134
- CPMAddPackage() uses the plural "OPTIONS" keyword elsewhere in this file; using "OPTION" here is inconsistent and likely ignored by CPM, meaning YAML_ENABLE_PIC may not be applied.
OPTION
"YAML_ENABLE_PIC OFF"
cmake/package-lock.cmake:109
- CPMDeclarePackage() blocks in this lock file use "OPTIONS" (plural). Using "OPTION" here is inconsistent with the rest of the lock and may prevent YAML_ENABLE_PIC from being applied when the lock is used.
OPTION YAML_ENABLE_PIC OFF
cucumber_cpp/library/support/StepRegistry.hpp:142
- This constructor now prints to stdout during step registration. That introduces side effects during normal library initialization/registration and can corrupt program output (especially for formatters/tests that expect deterministic stdout).
{
fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name());
}
cucumber_cpp/library/support/StepRegistry.hpp:15
- <fmt/base.h> was added solely to support the debug println in Entry's constructor. If that debug output is removed, this include should be removed as well to avoid pulling fmt into this header and increasing compile-time dependencies.
#include <fmt/base.h>
cucumber_cpp/library/support/StepRegistry.cpp:105
- Unconditional fmt::println() during step registration will pollute stdout for all consumers, including when running the actual cucumber output formatters. This looks like leftover debug logging and should not run in normal builds.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
cucumber_cpp/library/support/StepRegistry.cpp:16
- <fmt/base.h> was introduced only for the debug println in this file. If the println is removed, this include can be removed to keep dependencies minimal.
#include <fmt/base.h>
cucumber_cpp/library/support/DefinitionRegistration.hpp:13
- <fmt/base.h> was added here only to support the debug println in Register(). If the println is removed, this include should be removed to avoid forcing fmt into this public header.
#include <fmt/base.h>
cucumber_cpp/library/support/DefinitionRegistration.cpp:79
- Unconditional fmt::println() during registration adds stdout noise for all consumers and test runs. This should be removed or gated behind an explicit debug/logging mechanism.
fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.cpp:11
- <fmt/base.h> appears to be added solely for the debug println below. If the println is removed, drop this include to keep compile dependencies minimal.
#include <fmt/base.h>
compatibility/BaseCompatibility.cpp:76
- These unconditional fmt::println() calls add extra stdout output to the compatibility test runner, which can make test output noisy and potentially break tests that parse output. Prefer assertions/SCOPED_TRACE if you need diagnostics, or gate behind a debug flag.
fmt::println("current path: {}", std::filesystem::current_path().string());
fmt::println(" uri path: {}", uri);
compatibility/BaseCompatibility.cpp:18
- fmt headers were added only for the debug println statements later in this file. If those prints are removed, these includes should be removed to avoid unnecessary dependencies in the compatibility harness.
#include <fmt/base.h>
#include <fmt/ostream.h>
.github/workflows/ci.yml:60
- The CPM cache key here is based only on cmake/dependencies.cpm.cmake, but other jobs also include cmake/package-lock.cmake. If package-lock.cmake changes without dependencies.cpm.cmake changing, this job can restore a stale CPM cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:135
- This CPM cache key also ignores cmake/package-lock.cmake, which can cause stale dependency sources to be restored when the lock changes. Align the key with the other jobs by hashing both files.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
cucumber_cpp/library/support/DefinitionRegistration.hpp:100
- Printing from DefinitionRegistration::Register() adds unexpected stdout noise every time a step is registered. This is likely unintended debug output and can break consumers/tests that rely on clean output.
fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name());
…akeLists.txt and log paths in BaseCompatibility
…s in CMakeLists.txt and update BaseCompatibility to use relative paths
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (17)
CMakeLists.txt:26
- CUCUMBER_CPP_FETCH_DEPS is set but not used anywhere in the repo (only this assignment). If it’s not consumed by a dependency, it should be removed to avoid confusion.
set(CUCUMBER_CPP_FETCH_DEPS ${CCR_FETCH_DEPS})
cmake/dependencies.cpm.cmake:93
- nlohmann_json::nlohmann_json is linked by cucumber_cpp.library.formatter, but this dependency isn’t explicitly added in the CPM (CCR_FETCH_DEPS=On) path here. Relying on a transitive dependency to provide this target is brittle and can break builds when upstream changes.
# ---------------------------------------------------------------------------
# libfmt
# ---------------------------------------------------------------------------
CPMAddPackage(
URI "gh:fmtlib/fmt@${LIBFMT_VERSION}#${LIBFMT_DIGEST}"
NAME libfmt
)
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS is Off (subproject/system-deps mode), this file doesn’t find CLI11 or nlohmann_json, but the build links against CLI11::CLI11 (cucumber_cpp/library/CMakeLists.txt:28) and nlohmann_json::nlohmann_json (cucumber_cpp/library/formatter/CMakeLists.txt:33). This configuration will fail unless those targets are created elsewhere.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/dependencies.cpm.cmake:134
- CPMAddPackage uses the parameter name OPTIONS (plural). With "OPTION" here, YAML_ENABLE_PIC won’t be passed through as intended.
CPMAddPackage(
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
cmake/package-lock.cmake:110
- CPMDeclarePackage uses OPTIONS (plural) for cache options. "OPTION YAML_ENABLE_PIC OFF" doesn’t match the pattern used elsewhere in this lockfile and is unlikely to be applied.
SYSTEM YES
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
cucumber_cpp/library/support/StepRegistry.hpp:142
- This fmt::println() in a registration data structure constructor introduces unconditional stdout noise during static initialization/registration. That will pollute normal runs and test output; consider removing it or gating it behind an explicit debug/trace option.
{
fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name());
}
cucumber_cpp/library/support/StepRegistry.hpp:15
- <fmt/base.h> is only needed for the new debug print in this header. If the print is removed, this include should be dropped to keep the header lightweight.
#include <fmt/base.h>
cucumber_cpp/library/support/StepRegistry.cpp:104
- Unconditional fmt::println() during step registration will add noisy stdout output and can interfere with consumers/tests. Consider removing or guarding behind a debug flag.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
cucumber_cpp/library/support/StepRegistry.cpp:16
- <fmt/base.h> is only introduced for the new debug print. If the print is removed, drop this include to reduce compile overhead.
#include <fmt/base.h>
cucumber_cpp/library/support/DefinitionRegistration.hpp:100
- This template registration helper now prints unconditionally to stdout on every registration. This will add noise for library consumers and makes tests harder to keep clean; consider removing or gating behind a debug option.
fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.hpp:13
- <fmt/base.h> was added only for the new debug print in this header. If the print is removed, drop the include to keep this widely-included header minimal.
#include <fmt/base.h>
cucumber_cpp/library/support/DefinitionRegistration.cpp:79
- Unconditional fmt::println() in DefinitionRegistration::Register() will produce noisy stdout output during normal operation and tests. Consider removing or guarding behind a debug flag.
fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.cpp:11
- <fmt/base.h> is only introduced for the new debug print. If the print is removed, this include should be dropped.
#include <fmt/base.h>
compatibility/BaseCompatibility.cpp:78
- These fmt::println() calls look like temporary debug output and will pollute test logs/CI output. If you need this information, consider routing it through a controllable logger or guarding behind a verbosity flag.
std::filesystem::path path{ uri };
if (path.is_relative())
path = std::filesystem::path{ CCR_BINARY_DIR } / path;
compatibility/BaseCompatibility.cpp:18
- <fmt/base.h> and <fmt/ostream.h> are only needed for the new debug prints below. If those prints are removed, these includes should be dropped to keep dependencies minimal.
#include <fmt/base.h>
#include <fmt/ostream.h>
.github/workflows/ci.yml:60
- The CPM cache key here only hashes dependencies.cpm.cmake. If package-lock.cmake changes (new pins), this cache can become stale and serve the wrong sources.
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:135
- Same issue as above: this CPM cache key ignores package-lock.cmake, so lockfile updates won’t invalidate the cache.
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
Comment on lines
+89
to
+90
| add_compile_definitions(-DCCR_SOURCE_DIR="${CMAKE_SOURCE_DIR}") | ||
| add_compile_definitions(-DCCR_BINARY_DIR="${CMAKE_BINARY_DIR}") |
Comment on lines
+34
to
+40
| if(NOT EXISTS "${CPM_DOWNLOAD_LOCATION}") | ||
| message(STATUS "Downloading CPM.cmake ${CPM_DOWNLOAD_VERSION}…") | ||
| file(DOWNLOAD | ||
| "https://github.com/cpm-cmake/CPM.cmake/releases/download/${CPM_DOWNLOAD_TAG}/CPM.cmake" | ||
| "${CPM_DOWNLOAD_LOCATION}" | ||
| TLS_VERIFY ON | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (13)
cmake/dependencies.cpm.cmake:93
- This block is missing nlohmann_json, but cucumber_cpp/library/formatter links against nlohmann_json::nlohmann_json (cucumber_cpp/library/formatter/CMakeLists.txt:33). Without adding/finding it here, configuration will fail when CCR_FETCH_DEPS is ON. Also, use *_TAG (not *_VERSION) in the URIs to avoid dropping the 'v' prefix.
CPMAddPackage(
URI "gh:CLIUtils/CLI11@${CLI11_VERSION}#${CLI11_DIGEST}"
NAME cli11
)
# ---------------------------------------------------------------------------
# libfmt
# ---------------------------------------------------------------------------
CPMAddPackage(
URI "gh:fmtlib/fmt@${LIBFMT_VERSION}#${LIBFMT_DIGEST}"
NAME libfmt
)
cmake/dependencies.cpm.cmake:134
- Two issues in this yaml-cpp CPMAddPackage call: (1) it uses ${YAML_CPP_VERSION} (strips the leading 'v') rather than ${YAML_CPP_TAG}, and (2) the argument is spelled OPTION but CPM expects OPTIONS (as used elsewhere in this file).
CPMAddPackage(
URI "gh:jbeder/yaml-cpp@${YAML_CPP_VERSION}#${YAML_CPP_DIGEST}"
NAME yaml-cpp
OPTION
"YAML_ENABLE_PIC OFF"
cmake/dependencies.cpm.cmake:141
- When CCR_FETCH_DEPS is OFF, this file currently doesn't find CLI11 or nlohmann_json, but those imported targets are used by cucumber_cpp/library (CLI11::CLI11) and cucumber_cpp/library/formatter (nlohmann_json::nlohmann_json). Add the missing find_package calls so non-fetch builds configure successfully.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cmake/package-lock.cmake:110
- CPMDeclarePackage uses the keyword OPTIONS elsewhere in this lock file; 'OPTION YAML_ENABLE_PIC OFF' looks like a typo and may be ignored, causing yaml-cpp to be configured differently than intended.
CPMDeclarePackage(yaml-cpp
NAME yaml-cpp
VERSION 0.9.0
GIT_TAG 56e3bb550c91fd7005566f19c079cb7a503223cf
GITHUB_REPOSITORY jbeder/yaml-cpp
SYSTEM YES
EXCLUDE_FROM_ALL YES
OPTION YAML_ENABLE_PIC OFF
)
cucumber_cpp/library/support/StepRegistry.hpp:142
- This adds unconditional console output every time a step entry is constructed (often during static initialization). That will spam test output and is not appropriate for a library by default. Remove the println (or gate it behind an explicit debug macro).
{
fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name());
}
cucumber_cpp/library/support/StepRegistry.cpp:104
- Unconditional fmt::println during step registration will add noisy output and can interfere with consumers/tests that expect clean stdout. Remove or gate behind a debug/logging option.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.hpp:100
- This template registration helper now prints on every registration call, which is noisy and can break consumers expecting no stdout. Remove or gate behind a debug/logging option.
fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.cpp:80
- Unconditional fmt::println in the core registration path will spam stdout for every step definition. Remove or gate behind a debug/logging option.
fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name());
registry.try_emplace(sourceLocation, StepStringRegistration::Entry{ stepType, std::string{ matcher }, factory, sourceLocation });
.github/workflows/ci.yml:61
- This cache key expression is missing the closing '}}', which will make the workflow YAML invalid. Also include package-lock.cmake in the hash so dependency version changes invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:94
- This cache key expression is missing the closing '}}', which will make the workflow YAML invalid. Also include package-lock.cmake in the hash so dependency version changes invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake', 'cmake/package-lock.cmake') }}
.github/workflows/ci.yml:136
- This cache key expression is missing the closing '}}', which will make the workflow YAML invalid. Also include package-lock.cmake in the hash so dependency version changes invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/static-analysis.yml:40
- This cache key expression is missing the closing '}}', which will make the workflow YAML invalid. Fix the expression so the workflow parses, and keep package-lock.cmake in the hash so dependency updates invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake', 'cmake/package-lock.cmake') }}
.github/workflows/static-analysis.yml:95
- This cache key expression is missing the closing '}}', which will make the workflow YAML invalid. Fix the expression so the workflow parses, and keep package-lock.cmake in the hash so dependency updates invalidate the cache.
- name: Cache CPM packages
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: ${{ env.CPM_SOURCE_CACHE }}
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake', 'cmake/package-lock.cmake') }}
Comment on lines
+56
to
+58
| CPMAddPackage( | ||
| URI "gh:cucumber/gherkin@${CUCUMBER_GHERKIN_VERSION}#${CUCUMBER_GHERKIN_DIGEST}" | ||
| NAME cucumber_gherkin |
Comment on lines
+67
to
+69
| CPMAddPackage( | ||
| URI "gh:google/googletest@${GOOGLE_TEST_VERSION}#${GOOGLE_TEST_DIGEST}" | ||
| NAME googletest |
Comment on lines
+98
to
+101
| CPMAddPackage( | ||
| URI "gh:zeux/pugixml@${PUGIXML_VERSION}#${PUGIXML_DIGEST}" | ||
| NAME pugixml | ||
| ) |
Comment on lines
48
to
52
| target_compile_definitions(${libname} PRIVATE | ||
| KIT_NAME=${KITNAME} | ||
| KIT_STRING="${name}" | ||
| KIT_FOLDER="${CMAKE_CURRENT_SOURCE_DIR}/${name}" | ||
| KIT_NDJSON_FILE="${CMAKE_CURRENT_SOURCE_DIR}/${name}/${name}.ndjson" |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (11)
cmake/dependencies.cpm.cmake:133
CPMAddPackageusesOPTIONS(plural).OPTIONis not a valid keyword here and will cause configuration errors for yaml-cpp.
OPTION
cmake/package-lock.cmake:109
CPMDeclarePackageuses anOPTIONSlist (see other entries in this lockfile).OPTION YAML_ENABLE_PIC OFFis inconsistent and likely not parsed as intended.
OPTION YAML_ENABLE_PIC OFF
cmake/dependencies.cpm.cmake:141
- When
CCR_FETCH_DEPSis OFF, the project still links againstCLI11::CLI11andnlohmann_json::nlohmann_json(seecucumber_cpp/library/CMakeLists.txtandcucumber_cpp/library/formatter/CMakeLists.txt), but those packages are not found here. This will break configuration for system-provided deps.
find_package(cucumber_gherkin ${CUCUMBER_GHERKIN_VERSION} REQUIRED)
find_package(GTest ${GOOGLE_TEST_VERSION} REQUIRED)
find_package(pugixml ${PUGIXML_VERSION} REQUIRED)
find_package(fmt ${LIBFMT_VERSION} REQUIRED)
cucumber_cpp/library/support/StepRegistry.hpp:142
- This constructor is executed during step registration and currently prints unconditionally to stdout. That creates noisy/non-deterministic output (including during static initialization) and can break consumers that expect clean stdout.
{
fmt::println("StepStringRegistration::Entry: {}", sourceLocation.file_name());
}
cucumber_cpp/library/support/StepRegistry.cpp:104
- Unconditional registration logging to stdout will pollute normal program output and test logs. This should be removed or gated behind an explicit debug/logging option.
fmt::println("Registering step definition@{}", sourceLocation.file_name());
cucumber_cpp/library/support/DefinitionRegistration.hpp:101
- This template is used by user code for step registration; unconditionally printing to stdout here will add noisy output and can break tooling that parses stdout.
fmt::println("DefinitionRegistration::Register<T>: {}", sourceLocation.file_name());
return Instance().Register(matcher, stepType, util::StepBodyFactory<T>, sourceLocation);
cucumber_cpp/library/support/DefinitionRegistration.cpp:80
- Unconditional stdout logging during registration will pollute normal output and can break tests/formatters that expect clean stdout.
fmt::println("DefinitionRegistration::Register: {}", sourceLocation.file_name());
registry.try_emplace(sourceLocation, StepStringRegistration::Entry{ stepType, std::string{ matcher }, factory, sourceLocation });
compatibility/BaseCompatibility.cpp:18
- These newly-added fmt headers are unused in this file and add unnecessary compile-time dependencies.
#include <fmt/base.h>
#include <fmt/ostream.h>
.github/workflows/ci.yml:60
- The CPM cache key only hashes
cmake/dependencies.cpm.cmake. Changes tocmake/package-lock.cmakewon't invalidate the cache, which can lead to stale/incorrect dependency sources being reused.
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
.github/workflows/ci.yml:135
- The CPM cache key only hashes
cmake/dependencies.cpm.cmake. Changes tocmake/package-lock.cmakewon't invalidate the cache, which can lead to stale/incorrect dependency sources being reused.
key: cpm-${{ hashFiles('cmake/dependencies.cpm.cmake') }}
cmake/dependencies.cpm.cmake:85
nlohmann_json::nlohmann_jsonis a direct link dependency ofcucumber_cpp.library.formatter, but it isn't added inCCR_FETCH_DEPSmode. Adding it explicitly here avoids relying on transitive fetches from other packages.
CPMAddPackage(
URI "gh:CLIUtils/CLI11@${CLI11_VERSION}#${CLI11_DIGEST}"
NAME cli11
)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.