-
Notifications
You must be signed in to change notification settings - Fork 35
ci: add newdeps job testing newer versions of cmake and capnproto #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
CI_DESC="CI job using newest Cap'n Proto and cmake versions" | ||
CI_DIR=build-newdeps | ||
export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter -Wno-error=array-bounds" | ||
CAPNP_CHECKOUT=master | ||
NIX_ARGS=(--argstr capnprotoVersion "none" --argstr cmakeVersion "4.1.1") | ||
BUILD_ARGS=(-k) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,17 +21,43 @@ cmake --version | |
cmake_ver=$(cmake --version | awk '/version/{print $3; exit}') | ||
ver_ge() { [ "$(printf '%s\n' "$2" "$1" | sort -V | head -n1)" = "$2" ]; } | ||
|
||
# If CAPNP_CHECKOUT was requested, clone and install requested Cap'n Proto branch or tag | ||
capnp_prefix= | ||
if [ -n "${CAPNP_CHECKOUT-}" ]; then | ||
capnp_prefix="$PWD/capnp-install" | ||
[ -e "capnp" ] || git clone -b "${CAPNP_CHECKOUT}" "https://github.com/capnproto/capnproto" capnp | ||
mkdir -p capnp/build | ||
( | ||
cd capnp/build | ||
git --no-pager log -1 || true | ||
CXXFLAGS="-std=c++20" cmake .. "-DCMAKE_INSTALL_PREFIX=${capnp_prefix}" -DBUILD_TESTING=OFF -DWITH_OPENSSL=OFF -DWITH_ZLIB=OFF | ||
cmake --build . | ||
cmake --install . | ||
) | ||
export CMAKE_PREFIX_PATH="${capnp_prefix}:${CMAKE_PREFIX_PATH-}" | ||
fi | ||
|
||
src_dir=$PWD | ||
mkdir -p "$CI_DIR" | ||
cd "$CI_DIR" | ||
cmake "$src_dir" "${CMAKE_ARGS[@]+"${CMAKE_ARGS[@]}"}" | ||
git --no-pager log -1 || true | ||
cmake_args=("${CMAKE_ARGS[@]+"${CMAKE_ARGS[@]}"}") | ||
if ! cmake "$src_dir" "${cmake_args[@]}"; then | ||
# If cmake failed, try it again with debug options. | ||
# Could add --trace / --trace-expand here too but they are very verbose. | ||
cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) | ||
cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?" | ||
cat CMakeFiles/CMakeConfigureLog.yaml || true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving discussion from #209 (comment).
One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?
This would make sense if it were the only compatibility issue in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #212 (comment)
It would be helpful to know what complexity you are referring to specifically here. The find_packge(Threads REQUIRED) issue would have been impossible to debug without the line:
and I do not feel like it adds much complexity. This CI script in general seems short and simple to me. I don't know what problem you have with the other debug prints either. In general, I feel like it would be a lot easier to respond to your comments if they could make it clear:
|
||
find . -ls || true | ||
false | ||
fi | ||
if ver_ge "$cmake_ver" "3.15"; then | ||
cmake --build . -t "${BUILD_TARGETS[@]}" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
cmake --build . --parallel -t "${BUILD_TARGETS[@]}" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
else | ||
# Older versions of cmake can only build one target at a time with --target, | ||
# and do not support -t shortcut | ||
for t in "${BUILD_TARGETS[@]}"; do | ||
cmake --build . --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
cmake --build . --parallel --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
done | ||
fi | ||
ctest --output-on-failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this project maintains support for CMake versions as old as 3.12, I believe this support should also extend consistently to the CI infrastructure, particularly given the "olddeps" job, which runs CMake 3.12.4. Therefore, features incompatible with CMake 3.12, such as
--debug-find
and--log-level
, should be avoided. For example, the "olddeps" CI job may result in:Alternatively, the minimum supported CMake version could be bumped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #212 (comment)
If the problem you would like me to solve is that extra output potentially appearing in the olddeps job, it is trivial to skip the debug step there with:
But I don't see a problem with this extra output and I don't know if this the issue you actually care about. If you are arguing that systems using cmake 3.16 should not be supported because it does not support the --debug-find option, than that sounds like something to mention in #175 more than here.