Migrate CI from GitHub Actions to Drone#177
Merged
mvandeberg merged 1 commit intocppalliance:developfrom Feb 20, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR restructures CI configuration to introduce three CMake-based job variants (superproject, mainproject, subdirectory), adds macOS and Valgrind testing jobs, removes path-based filtering from code coverage workflows, and converts three static member functions from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
An automated preview of the documentation is available at https://177.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-20 22:10:45 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.drone/drone.sh (2)
88-191: Consider extracting shared setup into a helper function.The initialization block (lines 96–101, 136–141, 170–175) is duplicated verbatim across all three cmake handlers:
export CXXFLAGS="-Wall -Wextra -Werror" export CMAKE_SHARED_LIBS=${CMAKE_SHARED_LIBS:-1} export CMAKE_NO_TESTS=${CMAKE_NO_TESTS:-error} if [ $CMAKE_NO_TESTS = "error" ]; then CMAKE_BUILD_TESTING="-DBUILD_TESTING=ON" fiA small helper (e.g.,
cmake_init) would reduce the triplication and make future flag changes less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.sh around lines 88 - 191, Extract the repeated CMake initialization block into a helper function (e.g., cmake_init) and call it from each cmake handler branch (the blocks handling DRONE_JOB_BUILDTYPE values "cmake-superproject", "cmake-mainproject", and "cmake-subdirectory"); move the lines that export CXXFLAGS, set CMAKE_SHARED_LIBS and CMAKE_NO_TESTS and the conditional that sets CMAKE_BUILD_TESTING into cmake_init, ensure cmake_init is defined before the first use, and replace the three duplicated blocks with a single call to cmake_init to centralize flag setup.
88-126: cmake-superproject handler looks correct.The logic for defaulting
CMAKE_NO_TESTS, conditionally enablingBUILD_TESTING, and gating the shared build is sound. The static/shared split with explicit--target testsbeforectestensures test binaries are built.One defensive-coding nit: line 99 (and the equivalent lines 139 and 173) use an unquoted variable in the test:
if [ $CMAKE_NO_TESTS = "error" ]; thenWhile safe here because
:-errorguarantees a non-empty value, quoting is idiomatic shell best practice:if [ "$CMAKE_NO_TESTS" = "error" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.sh around lines 88 - 126, The if-tests use an unquoted variable which is fragile; update every occurrence of the test that checks CMAKE_NO_TESTS (the existing lines using if [ $CMAKE_NO_TESTS = "error" ]; and the equivalent occurrences later) to quote the variable (use if [ "$CMAKE_NO_TESTS" = "error" ]; ) so shell word-splitting and empty-value edge cases are avoided while keeping the existing logic that sets CMAKE_NO_TESTS via ${CMAKE_NO_TESTS:-error}..drone/drone.bat (1)
129-134: Same--no-tests=errorhardcoding incmake-mainprojectandcmake-subdirectory.Lines 142, 145 (
cmake-mainproject) hardcode--no-tests=errordespite settingCMAKE_NO_TESTSon lines 129–131. Thecmake-subdirectoryblock (lines 165–170) sets the variable but doesn't even invokectest, relying solely on thechecktarget — which is fine, but the deadCMAKE_NO_TESTSsetup is confusing. Consider either using the variable or removing the dead setup.Also applies to: 142-145, 165-170, 175-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.bat around lines 129 - 134, The script contains dead/duplicated handling of CMAKE_NO_TESTS and inconsistent usage in the cmake-mainproject and cmake-subdirectory blocks: remove the hardcoded "--no-tests=error" usage or wire the blocks to consistently use the CMAKE_NO_TESTS variable; specifically, update the cmake-mainproject logic that currently hardcodes --no-tests=error (lines referenced in the review) to respect and pass CMAKE_NO_TESTS/CMAKE_BUILD_TESTING when invoking ctest, and either remove the redundant CMAKE_NO_TESTS setup in the cmake-subdirectory block (which never calls ctest and relies on the check target) or make cmake-subdirectory actually use CMAKE_NO_TESTS if you intend to control test behavior there; ensure references like CMAKE_NO_TESTS, CMAKE_BUILD_TESTING, cmake-mainproject, cmake-subdirectory, ctest, and the check target are the points you change for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.drone/drone.bat:
- Around line 42-109: The ctest invocations hardcode "--no-tests=error" instead
of using the CMAKE_NO_TESTS variable; update every ctest call in this block
(both __build_static and __build_shared sections where ctest is invoked) to use
"--no-tests=!CMAKE_NO_TESTS!" so the CMAKE_NO_TESTS setting (managed alongside
CMAKE_BUILD_TESTING and the earlier SET logic) is respected across Debug and
Release runs.
- Around line 111-146: The Windows CI script (.drone/drone.bat) is missing the
shared-library build pathways for the cmake-mainproject and cmake-subdirectory
job branches (unlike drone.sh which runs builds when CMAKE_SHARED_LIBS=1);
update the cmake-mainproject block (the branch starting with ) else if
"%DRONE_JOB_BUILDTYPE%" == "cmake-mainproject" ( and containing cmake
-DBoost_VERBOSE=1 !CMAKE_BUILD_TESTING! ...) to add a separate
build/install/test sequence that sets -DBUILD_SHARED_LIBS=ON (or honors
CMAKE_SHARED_LIBS), runs cmake, cmake --build for Debug and Release, and runs
ctest for the shared build (mirroring the static build steps), and apply the
same pattern to the cmake-subdirectory block (the ) else if
"%DRONE_JOB_BUILDTYPE%" == "cmake-subdirectory" ( section) so both jobs perform
both static and shared builds when CMAKE_SHARED_LIBS=1.
In @.drone/drone.sh:
- Around line 128-160: The cmake-mainproject path sets CMAKE_BUILD_TESTING
(CMAKE_BUILD_TESTING variable) but only builds the install target, so test
executables never get built and ctest can fail; update both build blocks (the
__build_static and __build_shared sections where you run "cmake --build .
--target install") to also run a build of the test target (e.g., run "cmake
--build . --target tests" or the project's test target) before install when
CMAKE_BUILD_TESTING is enabled (i.e., when CMAKE_NO_TESTS != "0"/when
CMAKE_BUILD_TESTING is set), ensuring tests are built prior to running ctest.
---
Nitpick comments:
In @.drone/drone.bat:
- Around line 129-134: The script contains dead/duplicated handling of
CMAKE_NO_TESTS and inconsistent usage in the cmake-mainproject and
cmake-subdirectory blocks: remove the hardcoded "--no-tests=error" usage or wire
the blocks to consistently use the CMAKE_NO_TESTS variable; specifically, update
the cmake-mainproject logic that currently hardcodes --no-tests=error (lines
referenced in the review) to respect and pass CMAKE_NO_TESTS/CMAKE_BUILD_TESTING
when invoking ctest, and either remove the redundant CMAKE_NO_TESTS setup in the
cmake-subdirectory block (which never calls ctest and relies on the check
target) or make cmake-subdirectory actually use CMAKE_NO_TESTS if you intend to
control test behavior there; ensure references like CMAKE_NO_TESTS,
CMAKE_BUILD_TESTING, cmake-mainproject, cmake-subdirectory, ctest, and the check
target are the points you change for consistency.
In @.drone/drone.sh:
- Around line 88-191: Extract the repeated CMake initialization block into a
helper function (e.g., cmake_init) and call it from each cmake handler branch
(the blocks handling DRONE_JOB_BUILDTYPE values "cmake-superproject",
"cmake-mainproject", and "cmake-subdirectory"); move the lines that export
CXXFLAGS, set CMAKE_SHARED_LIBS and CMAKE_NO_TESTS and the conditional that sets
CMAKE_BUILD_TESTING into cmake_init, ensure cmake_init is defined before the
first use, and replace the three duplicated blocks with a single call to
cmake_init to centralize flag setup.
- Around line 88-126: The if-tests use an unquoted variable which is fragile;
update every occurrence of the test that checks CMAKE_NO_TESTS (the existing
lines using if [ $CMAKE_NO_TESTS = "error" ]; and the equivalent occurrences
later) to quote the variable (use if [ "$CMAKE_NO_TESTS" = "error" ]; ) so shell
word-splitting and empty-value edge cases are avoided while keeping the existing
logic that sets CMAKE_NO_TESTS via ${CMAKE_NO_TESTS:-error}.
Comment on lines
42
to
109
| ) else if "%DRONE_JOB_BUILDTYPE%" == "cmake-superproject" ( | ||
|
|
||
| echo "Running cmake superproject job" | ||
| echo '==================================> INSTALL' | ||
| SET "CXX=" | ||
|
|
||
| git clone https://github.com/boostorg/boost-ci.git boost-ci-cloned --depth 1 | ||
| cp -prf boost-ci-cloned/ci . | ||
| rm -rf boost-ci-cloned | ||
|
|
||
| for /F %%i in ("%DRONE_REPO%") do @set SELF=%%~nxi | ||
| SET BOOST_CI_TARGET_BRANCH=!TRAVIS_BRANCH! | ||
| SET BOOST_CI_SRC_FOLDER=%cd% | ||
|
|
||
| call ci\common_install.bat | ||
|
|
||
| echo '==================================> COMPILE' | ||
|
|
||
| if "!CMAKE_NO_TESTS!" == "" ( | ||
| SET CMAKE_NO_TESTS=error | ||
| ) | ||
| if "!CMAKE_NO_TESTS!" == "error" ( | ||
| SET CMAKE_BUILD_TESTING=-DBUILD_TESTING=ON | ||
| ) | ||
|
|
||
| cd ../../ | ||
|
|
||
| mkdir __build_static && cd __build_static | ||
| cmake -DBoost_VERBOSE=1 !CMAKE_BUILD_TESTING! -DCMAKE_INSTALL_PREFIX=iprefix ^ | ||
| -DBOOST_INCLUDE_LIBRARIES=!SELF! !CMAKE_OPTIONS! .. | ||
|
|
||
| cmake --build . --target install --config Debug | ||
| if NOT "!CMAKE_BUILD_TESTING!" == "" ( | ||
| cmake --build . --target tests --config Debug | ||
| ) | ||
| ctest --output-on-failure --no-tests=error -R boost_!SELF! -C Debug | ||
|
|
||
| cmake --build . --target install --config Release | ||
| if NOT "!CMAKE_BUILD_TESTING!" == "" ( | ||
| cmake --build . --target tests --config Release | ||
| ) | ||
| ctest --output-on-failure --no-tests=error -R boost_!SELF! -C Release | ||
| cd .. | ||
|
|
||
| if "!CMAKE_SHARED_LIBS!" == "" ( | ||
| SET CMAKE_SHARED_LIBS=1 | ||
| ) | ||
| if "!CMAKE_SHARED_LIBS!" == "1" ( | ||
|
|
||
| mkdir __build_shared && cd __build_shared | ||
| cmake -DBoost_VERBOSE=1 !CMAKE_BUILD_TESTING! -DCMAKE_INSTALL_PREFIX=iprefix ^ | ||
| -DBOOST_INCLUDE_LIBRARIES=!SELF! -DBUILD_SHARED_LIBS=ON !CMAKE_OPTIONS! .. | ||
|
|
||
| cmake --build . --config Debug | ||
| cmake --build . --target install --config Debug | ||
| if NOT "!CMAKE_BUILD_TESTING!" == "" ( | ||
| cmake --build . --target tests --config Debug | ||
| ) | ||
| ctest --output-on-failure --no-tests=error -R boost_!SELF! -C Debug | ||
|
|
||
| cmake --build . --config Release | ||
| cmake --build . --target install --config Release | ||
| if NOT "!CMAKE_BUILD_TESTING!" == "" ( | ||
| cmake --build . --target tests --config Release | ||
| ) | ||
| ctest --output-on-failure --no-tests=error -R boost_!SELF! -C Release | ||
|
|
||
| ) |
There was a problem hiding this comment.
--no-tests=error is hardcoded instead of using !CMAKE_NO_TESTS!.
Lines 60–62 set CMAKE_NO_TESTS but the ctest calls on lines 77, 83, 100, and 107 hardcode --no-tests=error rather than --no-tests=!CMAKE_NO_TESTS!. This defeats the purpose of the variable and diverges from drone.sh, which uses --no-tests=$CMAKE_NO_TESTS.
Proposed fix (showing one instance; apply to all four ctest lines)
-ctest --output-on-failure --no-tests=error -R boost_!SELF! -C Debug
+ctest --output-on-failure --no-tests=!CMAKE_NO_TESTS! -R boost_!SELF! -C Debug🧰 Tools
🪛 Blinter (1.0.112)
[error] 67-67: Unvalidated path traversal. Explanation: Path operations may allow directory traversal attacks with .. sequences. Recommendation: Validate paths and remove .. sequences before file operations. Context: Path contains .. which may allow directory traversal
(SEC011)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.drone/drone.bat around lines 42 - 109, The ctest invocations hardcode
"--no-tests=error" instead of using the CMAKE_NO_TESTS variable; update every
ctest call in this block (both __build_static and __build_shared sections where
ctest is invoked) to use "--no-tests=!CMAKE_NO_TESTS!" so the CMAKE_NO_TESTS
setting (managed alongside CMAKE_BUILD_TESTING and the earlier SET logic) is
respected across Debug and Release runs.
|
GCOVR code coverage report https://177.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-20 22:26:09 UTC |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.drone.star (1)
113-120:B2_TOOLSETandB2_CXXSTDare unused in a cmake-superproject build.The Windows
cmake-superprojectjob setsB2_TOOLSETandB2_CXXSTDin its environment, butdrone.bat'scmake-superprojectpath never reads these variables — it usesCMAKE_OPTIONSandCMAKE_BUILD_TESTINGinstead. These entries are harmless but misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone.star around lines 113 - 120, Remove the misleading unused env vars by deleting B2_TOOLSET and B2_CXXSTD from the environment block of the windows_cxx("msvc-14.3 cmake-superproject", ...) job; the cmake-superproject path in drone.bat consumes CMAKE_OPTIONS and CMAKE_BUILD_TESTING (not B2_*), so keep CMAKE_* entries only and ensure no other part of the job references B2_TOOLSET or B2_CXXSTD..drone/drone.sh (1)
99-101: Quote$CMAKE_NO_TESTSin the[test to avoid word-splitting errors.
[ $CMAKE_NO_TESTS = "error" ]will produce a syntax error if the variable is ever empty or contains spaces. The default on the preceding line makes this unlikely in practice, but quoting is standard shell hygiene and costs nothing.-if [ $CMAKE_NO_TESTS = "error" ]; then +if [ "$CMAKE_NO_TESTS" = "error" ]; thenAlso applies to: 139-141, 173-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.sh around lines 99 - 101, The test expressions that compare the variable CMAKE_NO_TESTS use unquoted expansion (e.g., [ $CMAKE_NO_TESTS = "error" ]) which can break if the variable is empty or contains spaces; update every such test (the instance that sets CMAKE_BUILD_TESTING and the other identical test blocks) to quote the variable (e.g., [ "$CMAKE_NO_TESTS" = "error" ]) so the shell does not perform word-splitting or produce syntax errors..drone/drone.bat (1)
42-56: Consider extracting common install boilerplate into a subroutine.All three cmake branches (
cmake-superproject,cmake-mainproject,cmake-subdirectory) duplicate the same ~14-line clone-boost-ci / set-SELF / common_install sequence. Theboostbranch at lines 13–40 has the same block too. Extracting it into aCALL :install_commonsubroutine would reduce duplication and maintenance burden.Also applies to: 111-125, 147-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.bat around lines 42 - 56, Extract the repeated clone-and-setup block into a batch subroutine named :install_common and replace each duplicated block (in branches handling cmake-superproject, cmake-mainproject, cmake-subdirectory and the boost branch) with a single CALL :install_common; the new :install_common should perform the git clone of boost-ci, copy and remove boost-ci-cloned, set SELF from %DRONE_REPO%, set BOOST_CI_TARGET_BRANCH=!TRAVIS_BRANCH! and BOOST_CI_SRC_FOLDER=%cd%, clear CXX if needed, then call ci\common_install.bat and exit the subroutine with GOTO :EOF so behavior is identical to the original blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.drone/drone.bat:
- Around line 138-145: The cmake-mainproject block skips building test binaries
before running ctest; after the install builds (the commands that include "cmake
--build . --target install --config Debug" and "cmake --build . --target install
--config Release") insert an explicit build step to build the test target (run
"cmake --build . --target tests" for each configuration) so that test
executables exist before calling "ctest --output-on-failure --no-tests=error -R
boost_!SELF! -C Debug" and the Release ctest invocation; ensure the test-build
step mirrors the cmake-superproject pattern used elsewhere.
---
Duplicate comments:
In @.drone/drone.bat:
- Around line 77-83: Replace the hardcoded ctest flag "--no-tests=error" with
the CMake variable placeholder so the script respects the configured
CMAKE_NO_TESTS value: update all ctest invocations (the lines calling "ctest
--output-on-failure --no-tests=error -R boost_!SELF! -C Debug" and the Release
variant, and the other four ctest lines in the same
cmake-superproject/cmake-mainproject blocks) to use
"--no-tests=!CMAKE_NO_TESTS!" instead of the literal "error", ensuring
consistency with the earlier set variable and with drone.sh's
"--no-tests=$CMAKE_NO_TESTS".
- Around line 111-146: The Windows CI branch for "cmake-mainproject" (and
similarly the "cmake-subdirectory" block) only runs static builds; add a second,
gated shared-library build path that mirrors the static one when
CMAKE_SHARED_LIBS is enabled. Concretely: after the existing static
__build_static block for cmake-mainproject, add a gated block that checks the
CMAKE_SHARED_LIBS variable (e.g. if "%CMAKE_SHARED_LIBS%" == "ON" or not empty)
and creates a separate build directory (e.g. __build_shared), invokes cmake with
-DBUILD_SHARED_LIBS=ON (or equivalent), then runs cmake --build . --target
install and ctest for Debug and Release just like the static flow. Repeat the
same change for the cmake-subdirectory section so both job types cover
shared-library builds.
In @.drone/drone.sh:
- Around line 143-148: The static build block creates __build_static and
installs but doesn't build the test target before running ctest; modify the
sequence around the commands that call "cmake --build . --target install" (in
the mkdir __build_static / cd __build_static section) to add an explicit build
of the tests target (e.g., run cmake --build . --target tests or cmake --build .
--target all && cmake --build . --target tests) prior to invoking "ctest
--output-on-failure --no-tests=$CMAKE_NO_TESTS"; apply the same change in the
shared build block (the analogous commands later) so test executables are built
before ctest runs.
---
Nitpick comments:
In @.drone.star:
- Around line 113-120: Remove the misleading unused env vars by deleting
B2_TOOLSET and B2_CXXSTD from the environment block of the
windows_cxx("msvc-14.3 cmake-superproject", ...) job; the cmake-superproject
path in drone.bat consumes CMAKE_OPTIONS and CMAKE_BUILD_TESTING (not B2_*), so
keep CMAKE_* entries only and ensure no other part of the job references
B2_TOOLSET or B2_CXXSTD.
In @.drone/drone.bat:
- Around line 42-56: Extract the repeated clone-and-setup block into a batch
subroutine named :install_common and replace each duplicated block (in branches
handling cmake-superproject, cmake-mainproject, cmake-subdirectory and the boost
branch) with a single CALL :install_common; the new :install_common should
perform the git clone of boost-ci, copy and remove boost-ci-cloned, set SELF
from %DRONE_REPO%, set BOOST_CI_TARGET_BRANCH=!TRAVIS_BRANCH! and
BOOST_CI_SRC_FOLDER=%cd%, clear CXX if needed, then call ci\common_install.bat
and exit the subroutine with GOTO :EOF so behavior is identical to the original
blocks.
In @.drone/drone.sh:
- Around line 99-101: The test expressions that compare the variable
CMAKE_NO_TESTS use unquoted expansion (e.g., [ $CMAKE_NO_TESTS = "error" ])
which can break if the variable is empty or contains spaces; update every such
test (the instance that sets CMAKE_BUILD_TESTING and the other identical test
blocks) to quote the variable (e.g., [ "$CMAKE_NO_TESTS" = "error" ]) so the
shell does not perform word-splitting or produce syntax errors.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/boost/capy/io/any_buffer_sink.hpp (1)
658-714:consteval→constexprchange is correct — consider consolidating the repeated comments.The fix is valid for the same reason as
any_buffer_source.hpp. However, the identical Clang-14 rationale comment is emitted three times (lines 658–660, 685–687, 712–714) whileany_buffer_source.hppachieves the same with a single comment block beforecompute_max_size. Consider consolidating to reduce noise, as per the guideline to use inline comments sparingly.♻️ Suggested consolidation
- // constexpr instead of consteval: Clang 14 rejects consteval - // member functions used before the closing brace of the class. - - static constexpr std::size_t + // constexpr instead of consteval: Clang 14 rejects consteval + // member functions used before the closing brace of the class. + + static constexpr std::size_t compute_max_size() noexcept { ... } - // constexpr instead of consteval: Clang 14 rejects consteval - // member functions used before the closing brace of the class. - - static constexpr std::size_t + static constexpr std::size_t compute_max_align() noexcept { ... } - // constexpr instead of consteval: Clang 14 rejects consteval - // member functions used before the closing brace of the class. - - static constexpr vtable + static constexpr vtable make_vtable() noexcept { ... }As per coding guidelines: "Single-line
//comments within code should be used sparingly and judiciously … Warn about comments that merely restate what the code does or excessive commenting that adds noise rather than clarity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/io/any_buffer_sink.hpp` around lines 658 - 714, Remove the repeated three-line comment blocks and replace them with a single consolidated comment placed once before compute_max_size (or at the top of the pair of helper functions) explaining why constexpr is used instead of consteval for Clang 14; update the comment text to be generic and reusable for both compute_max_size and compute_max_align and remove the duplicate comments immediately preceding compute_max_size and compute_max_align so only the single consolidated comment remains; ensure compute_max_size and compute_max_align signatures and behavior are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.drone/drone.bat:
- Around line 77-83: The ctest invocations currently hardcode "--no-tests=error"
which ignores the CMAKE_NO_TESTS variable; update every ctest call (all
occurrences of the ctest --output-on-failure ... commands) to use the batch
variable form "!CMAKE_NO_TESTS!" instead of the literal "--no-tests=error" so
the previously set CMAKE_NO_TESTS value is honored; ensure you replace the flag
in all four ctest invocations including the later block around the Release
build/tests section.
- Around line 138-146: The static build block hardcodes --no-tests=error, omits
building the tests target, and lacks a shared-library branch; update the two
ctest invocations to use --no-tests=!CMAKE_NO_TESTS!, add cmake --build .
--target tests --config Debug before the Debug ctest and cmake --build .
--target tests --config Release before the Release ctest (matching the pattern
used in cmake-superproject), and add an if block conditioned on
!CMAKE_SHARED_LIBS! == "1" that mirrors the shared-library build steps from
cmake-superproject/drone.sh to perform configure, build (including tests
target), install and run ctest for the shared build.
In @.drone/drone.sh:
- Around line 143-158: The static (__build_static) and shared (__build_shared)
build blocks are missing the step to actually build the "tests" target before
running ctest, so when CMAKE_BUILD_TESTING is enabled test binaries are never
compiled; add a conditional build step invoking "cmake --build . --target tests"
(guarded by the existing CMAKE_BUILD_TESTING/BUILD_TESTING condition)
immediately before each "ctest --output-on-failure --no-tests=$CMAKE_NO_TESTS"
invocation so that tests are compiled when BUILD_TESTING is ON (apply to both
cmake --build sequences).
---
Nitpick comments:
In `@include/boost/capy/io/any_buffer_sink.hpp`:
- Around line 658-714: Remove the repeated three-line comment blocks and replace
them with a single consolidated comment placed once before compute_max_size (or
at the top of the pair of helper functions) explaining why constexpr is used
instead of consteval for Clang 14; update the comment text to be generic and
reusable for both compute_max_size and compute_max_align and remove the
duplicate comments immediately preceding compute_max_size and compute_max_align
so only the single consolidated comment remains; ensure compute_max_size and
compute_max_align signatures and behavior are unchanged.
Contributor
|
The hosted FreeBSD is runner is on operating system version 13.1 https://github.com/cppalliance/drone-ci/blob/master/docs/drone-ci.md So, remove that OS from the matrix, or return to 13.1. If it's important, let me know, and I may be able to work on upgrading/replacing the existing machines. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.drone.star:
- Around line 56-76: The FreeBSD jobs target versions 14.3 and 15.0 which likely
don't exist on the hosted runner; update the jobs block that uses freebsd_cxx to
use the actual runner version (change freebsd_version values from "14.3" and
"15.0" to "13.1") or wrap the freebsd_cxx job additions in a runtime/feature
check that verifies runner availability before appending to jobs; also add a
brief TODO comment referencing sdarwin to confirm upgraded runners if you prefer
to keep newer versions.
- Around line 22-40: The generated jobs created by generate() use an internal
globalenv that doesn't include the file-level environment
(B2_VARIANT='debug,release' and B2_FLAGS='warnings=extra
warnings-as-errors=on'), causing inconsistency with explicit job helpers
(osx_cxx, freebsd_cxx, linux_cxx, windows_cxx); update generate() to accept a
globalenv parameter and pass the repository/file-level globalenv into generate()
(or, if changing the API is undesired, change generate()'s internal defaults to
include B2_VARIANT='debug,release' and B2_FLAGS='warnings=extra
warnings-as-errors=on') so generated jobs get the same environment as explicit
job creators.
---
Duplicate comments:
In @.drone/drone.bat:
- Around line 138-145: The cmake-mainproject block is missing a build step for
test targets and still uses a hardcoded flag; add a conditional "cmake --build .
--target tests --config Debug" before the Debug ctest invocation and a
conditional "cmake --build . --target tests --config Release" before the Release
ctest invocation guarded by the existing variable check (if NOT
"!CMAKE_BUILD_TESTING!" == "") so test executables are built when testing is
enabled, and replace the hardcoded "--no-tests=error" in both ctest calls with
the variable "--no-tests=!CMAKE_NO_TESTS!" so the behavior follows the provided
configuration.
- Around line 111-146: The cmake-mainproject block currently only builds static
libs; mirror the cmake-superproject pattern to add a shared-library build when
CMAKE_SHARED_LIBS (or CMAKE_SHARED_LIBS=1) is set: detect
CMAKE_SHARED_LIBS/!CMAKE_SHARED_LIBS! and, when enabled, create a separate build
folder (e.g., __build_shared), pass -DBUILD_SHARED_LIBS=ON (or set
BUILD_SHARED_LIBS=ON) to cmake alongside existing flags, then run cmake
--build/install and ctest for Debug and Release just like the static path; apply
the identical change to the cmake-subdirectory block so both targets exercise
shared-library builds.
- Line 77: The ctest invocations currently hardcode --no-tests=error which
overrides the configurable CMAKE_NO_TESTS; update each ctest command (the four
occurrences invoking ctest --output-on-failure --no-tests=error) to use the
CMAKE_NO_TESTS variable instead — replace the literal "error" with the batch
variable %CMAKE_NO_TESTS% so the commands read like ctest --output-on-failure
--no-tests=%CMAKE_NO_TESTS% (apply this change to all four ctest calls in the
block).
There was a problem hiding this comment.
🧹 Nitpick comments (4)
include/boost/capy/io/any_buffer_source.hpp (1)
489-548:consteval→constexprchange is correct and well-motivated.
static constexpr vtable value = make_vtable()(line 548) ensuresmake_vtable()— and transitivelycompute_max_size()/compute_max_align()— are still evaluated at compile time.vtableis a literal type (function pointers +std::size_t), soconstexprinitialization is valid. The Clang 14 rejection ofconstevalmember functions used before the class closing brace is a known Clang bug where staticconstevalmember functions fail to initialize astatic constexprmember within the same class body.One optional refinement: the rationale comment appears only before
compute_max_size()(lines 489–491) but not beforecompute_max_align()(line 509) ormake_vtable()(line 526), while the parallelany_buffer_sink.hpprepeats the comment before all three. A reader reviewingcompute_max_align()ormake_vtable()in isolation won't see the rationale inline. Consider either repeating the comment (matching the sink file) or elevating it to the//------section divider at line 487.✨ Proposed alignment with sink.hpp style
//------------------------------------------------------ - // constexpr instead of consteval: Clang 14 rejects consteval - // member functions used before the closing brace of the class. - static constexpr std::size_t + // constexpr instead of consteval: Clang 14 rejects consteval + // member functions used before the closing brace of the class. compute_max_size() noexcept ... + // constexpr instead of consteval: Clang 14 rejects consteval + // member functions used before the closing brace of the class. static constexpr std::size_t compute_max_align() noexcept ... + // constexpr instead of consteval: Clang 14 rejects consteval + // member functions used before the closing brace of the class. static constexpr vtable make_vtable() noexcept🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/io/any_buffer_source.hpp` around lines 489 - 548, The review says the rationale for switching consteval→constexpr is only placed before compute_max_size(), making compute_max_align() and make_vtable() unclear in isolation; update the comments to match any_buffer_sink.hpp by either repeating the Clang/consteval rationale comment immediately before compute_max_align() and make_vtable(), or move the rationale up to the section divider (the //------ block) so it applies to all three members (compute_max_size, compute_max_align, make_vtable) and users scanning any one of those functions see the reason; modify the comments around compute_max_align() and make_vtable() (and optionally the section divider) to reflect this change.include/boost/capy/io/any_buffer_sink.hpp (1)
658-715:consteval→constexprchange is correct for the same reason as inany_buffer_source.hpp.
static constexpr vtable value = make_vtable()(line 744) guarantees compile-time evaluation. No functional difference in behavior.The same comment block is repeated verbatim three times (lines 658–660, 685–687, 712–714). A single comment at the
//------section divider (line 656) would convey the rationale once without duplication, matching the single-comment style used inany_buffer_source.hpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/io/any_buffer_sink.hpp` around lines 658 - 715, The three identical comment blocks explaining the consteval→constexpr rationale are duplicated around compute_max_size(), compute_max_align(), and the static vtable value; remove the two redundant copies and keep a single explanatory comment placed at the section divider (the "//------" area) or immediately before compute_max_size(), so the rationale covers compute_max_size(), compute_max_align(), and the static constexpr vtable value = make_vtable() (symbol: vtable/value) without repetition..drone/drone.sh (1)
128-160:cmake-mainproject: note on build-order difference vsdrone.batand missing--target tests.In
drone.sh, thecmake-mainprojectpath goes straight from configure →--target install→ctestwithout an explicit--target testsstep (lines 147–148, 157–158). This is functionally correct becauseinstalldepends onall, which includes test executables whenBUILD_TESTING=ON. However, this differs fromcmake-superproject(which has an explicit--target testsstep) and creates a subtle inconsistency.Additionally, in
drone.batthecmake-superprojectblock builds tests after install (line 73 → 75), while here indrone.shit's the reverse (line 108 → 110). This ordering difference doesn't cause functional issues but is worth aligning for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone/drone.sh around lines 128 - 160, The cmake-mainproject branch in the script (the elif for "cmake-mainproject" that creates __build_static and __build_shared and uses BUILD_TESTING/ CMAKE_NO_TESTS) omits an explicit "cmake --build . --target tests" step and orders build/install vs test differently than cmake-superproject and drone.bat; to align behavior and improve consistency, add an explicit build target for tests (run "cmake --build . --target tests" after building/installing each build dir or adjust ordering to match cmake-superproject: build -> install -> build tests -> ctest) when BUILD_TESTING is enabled (i.e., when CMAKE_BUILD_TESTING is set), and ensure the test build runs before/consistently with ctest --output-on-failure --no-tests=$CMAKE_NO_TESTS in both __build_static and __build_shared flows..drone.star (1)
56-99: Remove unused B2_TOOLSET and B2_CXXSTD from cmake-superproject job definition.Lines 95–96 set
B2_TOOLSETandB2_CXXSTD, which are B2-specific variables. However, the cmake-superproject build path in bothdrone.shanddrone.batuses only CMAKE-related variables (CMAKE_OPTIONS,CMAKE_BUILD_TESTING, etc.) and never references these B2 variables. They are dead configuration and can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.drone.star around lines 56 - 99, The cmake-superproject job config sets B2_TOOLSET and B2_CXXSTD which are B2-specific and unused by the cmake-superproject path; remove these keys from the environment dict of the windows_cxx("msvc-14.3 cmake-superproject", ...) job so only CMake-related variables remain (keep environment entries like any CMAKE_* vars if present and leave globalenv unchanged); verify no other references to B2_TOOLSET or B2_CXXSTD in that job block (function/job identifier: windows_cxx("msvc-14.3 cmake-superproject", ...) and the environment dict) and commit the simplified environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.drone.star:
- Around line 13-40: The generate() call currently doesn't receive the
file-level globalenv (defined in globalenv), so generated matrix jobs use the
default B2_VARIANT='release' and miss B2_FLAGS; update the generate(...)
invocation to include the same environment settings (e.g., pass globalenv or at
minimum B2_VARIANT and B2_FLAGS) so generated jobs inherit 'debug,release' and
the extra warning flags, or if generate() lacks a globalenv parameter, duplicate
the B2_VARIANT and B2_FLAGS entries into the generate(...) call to ensure parity
with the explicit job helpers that use globalenv.
In @.drone/drone.bat:
- Around line 147-180: The cmake-subdirectory Windows branch only builds the
static variant (__build_static) and misses the shared-library variant; add a
mirrored shared build block after the existing __build_static steps that creates
__build_shared, runs cmake with -DBUILD_SHARED_LIBS=ON (preserving
!CMAKE_BUILD_TESTING! and !CMAKE_OPTIONS!) pointing at
../libs/!SELF!/test/cmake_test, then runs cmake --build . --config Debug, cmake
--build . --target check --config Debug, cmake --build . --config Release and
cmake --build . --target check --config Release so the shared-library tests run
on Windows too.
- Around line 138-145: The cmake-mainproject batch block incorrectly hardcodes
"--no-tests=error" and omits a shared-library build; update the two ctest
invocations that currently use "--no-tests=error" to use the variable
"--no-tests=!CMAKE_NO_TESTS!" (matching the earlier variable setup) and add the
mirror shared-library build step found in drone.sh: run "cmake --build .
--target install --config RelWithDebInfo" (or the same shared build config used
in drone.sh) and follow with the corresponding ctest call filtering boost_!SELF!
for that config; target the existing block around the mkdir __build_static /
cmake ... ../libs/!SELF! and modify the ctest lines and add the shared build +
ctest so both static and shared build paths are covered.
---
Nitpick comments:
In @.drone.star:
- Around line 56-99: The cmake-superproject job config sets B2_TOOLSET and
B2_CXXSTD which are B2-specific and unused by the cmake-superproject path;
remove these keys from the environment dict of the windows_cxx("msvc-14.3
cmake-superproject", ...) job so only CMake-related variables remain (keep
environment entries like any CMAKE_* vars if present and leave globalenv
unchanged); verify no other references to B2_TOOLSET or B2_CXXSTD in that job
block (function/job identifier: windows_cxx("msvc-14.3 cmake-superproject", ...)
and the environment dict) and commit the simplified environment.
In @.drone/drone.sh:
- Around line 128-160: The cmake-mainproject branch in the script (the elif for
"cmake-mainproject" that creates __build_static and __build_shared and uses
BUILD_TESTING/ CMAKE_NO_TESTS) omits an explicit "cmake --build . --target
tests" step and orders build/install vs test differently than cmake-superproject
and drone.bat; to align behavior and improve consistency, add an explicit build
target for tests (run "cmake --build . --target tests" after building/installing
each build dir or adjust ordering to match cmake-superproject: build -> install
-> build tests -> ctest) when BUILD_TESTING is enabled (i.e., when
CMAKE_BUILD_TESTING is set), and ensure the test build runs before/consistently
with ctest --output-on-failure --no-tests=$CMAKE_NO_TESTS in both __build_static
and __build_shared flows.
In `@include/boost/capy/io/any_buffer_sink.hpp`:
- Around line 658-715: The three identical comment blocks explaining the
consteval→constexpr rationale are duplicated around compute_max_size(),
compute_max_align(), and the static vtable value; remove the two redundant
copies and keep a single explanatory comment placed at the section divider (the
"//------" area) or immediately before compute_max_size(), so the rationale
covers compute_max_size(), compute_max_align(), and the static constexpr vtable
value = make_vtable() (symbol: vtable/value) without repetition.
In `@include/boost/capy/io/any_buffer_source.hpp`:
- Around line 489-548: The review says the rationale for switching
consteval→constexpr is only placed before compute_max_size(), making
compute_max_align() and make_vtable() unclear in isolation; update the comments
to match any_buffer_sink.hpp by either repeating the Clang/consteval rationale
comment immediately before compute_max_align() and make_vtable(), or move the
rationale up to the section divider (the //------ block) so it applies to all
three members (compute_max_size, compute_max_align, make_vtable) and users
scanning any one of those functions see the reason; modify the comments around
compute_max_align() and make_vtable() (and optionally the section divider) to
reflect this change.
Update Drone CI setup using the ci-automation generate() function for the main compiler matrix plus explicit jobs for special build types. generate() now handles (with defaults): - Full compiler matrix (GCC >=12, Clang >=17, MSVC >=14.1, ARM64, FreeBSD, Apple-Clang) - ASan, UBSan, coverage, and cmake-superproject Explicit jobs added for capabilities generate() does not provide: - Valgrind (Clang 17, Ubuntu 22.04) - cmake-mainproject and cmake-subdirectory (GCC 13, Ubuntu 24.04) - Windows cmake-superproject (MSVC 14.3) drone.sh: Rewrote cmake-superproject handler, added cmake-mainproject and cmake-subdirectory build types. drone.bat: Added cmake-superproject, cmake-mainproject, and cmake-subdirectory build types. Removed JSON-specific zlib clone.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #177 +/- ##
============================================
+ Coverage 78.40% 91.68% +13.27%
============================================
Files 36 76 +40
Lines 2084 4486 +2402
============================================
+ Hits 1634 4113 +2479
+ Misses 450 373 -77 see 45 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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
3 participants
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.
Replace GitHub Actions CI and code-coverage workflows with a comprehensive Drone CI setup using the ci-automation generate() function for the main compiler matrix plus explicit jobs for special build types.
generate() now handles (with defaults):
Explicit jobs added for capabilities generate() does not provide:
drone.sh: Rewrote cmake-superproject handler, added cmake-mainproject and cmake-subdirectory build types.
drone.bat: Added cmake-superproject, cmake-mainproject, and cmake-subdirectory build types. Removed JSON-specific zlib clone.
Deleted: .github/workflows/ci.yml, .github/workflows/code-coverage.yml
Kept: .github/workflows/docs.yml (GitHub Pages specific)
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores