Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:
uses: vmactions/freebsd-vm@v1
with:
prepare: |
pkg install -y cmake ninja bash capnproto
pkg install -y cmake ninja bash capnproto git
sync: 'rsync'
copyback: false

Expand Down Expand Up @@ -79,7 +79,7 @@ jobs:
strategy:
fail-fast: false
matrix:
config: [default, llvm, gnu32, sanitize, olddeps]
config: [default, llvm, gnu32, sanitize, olddeps, newdeps]

name: build • ${{ matrix.config }}

Expand Down
1 change: 1 addition & 0 deletions ci/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ CI_CONFIG=ci/configs/llvm.bash ci/scripts/run.sh
CI_CONFIG=ci/configs/gnu32.bash ci/scripts/run.sh
CI_CONFIG=ci/configs/sanitize.bash ci/scripts/run.sh
CI_CONFIG=ci/configs/olddeps.bash ci/scripts/run.sh
CI_CONFIG=ci/configs/newdeps.bash ci/scripts/run.sh
```

By default CI jobs will reuse their build directories. `CI_CLEAN=1` can be specified to delete them before running instead.
6 changes: 6 additions & 0 deletions ci/configs/newdeps.bash
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)
2 changes: 1 addition & 1 deletion ci/configs/sanitize.bash
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ CI_DIR=build-sanitize
export CXX=clang++
export CXXFLAGS="-ggdb -Werror -Wall -Wextra -Wpedantic -Wthread-safety-analysis -Wno-unused-parameter -fsanitize=thread"
CMAKE_ARGS=()
BUILD_ARGS=(-k -j4)
BUILD_ARGS=(-k)
BUILD_TARGETS=(mptest)
32 changes: 29 additions & 3 deletions ci/scripts/ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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:

+ cmake /home/runner/work/libmultiprocess/libmultiprocess --debug-find --debug-output --debug-trycompile --log-level=DEBUG
CMake Error: The source directory "/home/runner/work/libmultiprocess/libmultiprocess/build-olddeps/--log-level=DEBUG" does not exist.

Alternatively, the minimum supported CMake version could be bumped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #212 (comment)

For example, the "olddeps" CI job may result in

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:

-if ! cmake "$src_dir" "${cmake_args[@]}"; then
+if ! cmake "$src_dir" "${cmake_args[@]}" && ver_ge "$cmake_ver" "3.17"; then

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.

cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"
cat CMakeFiles/CMakeConfigureLog.yaml || true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving discussion from #209 (comment).

re: #209 (comment)

6ba1050
This log file name has been used only since CMake 3.26.

Yes that's part of the reason for adding || true here. I think this code could potentially print log files used by older versions of cmake too, and originally this change tried that, but since all CI jobs except one are using new enough versions of cmake it didn't seem worth complexity.

One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?

Would happily accept a PR to patch to improve this though.

This would make sense if it were the only compatibility issue in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #212 (comment)

One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?

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:

cat CMakeFiles/CMakeConfigureLog.yaml || true

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:

  1. What observable problem you think this change causes, or could potentially cause.
  2. What change you would suggest to fix the problem.

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
7 changes: 6 additions & 1 deletion shell.nix
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ let
clang-tools = llvm.clang-tools.override { inherit enableLibcxx; };
cmakeHashes = {
"3.12.4" = "sha256-UlVYS/0EPrcXViz/iULUcvHA5GecSUHYS6raqbKOMZQ=";
"4.1.1" = "sha256-sp9vGXM6oiS3djUHoQikJ+1Ixojh+vIrKcROHDBUkoI=";
};
cmakeBuild = if cmakeVersion == null then pkgs.cmake else (pkgs.cmake.overrideAttrs (old: {
version = cmakeVersion;
Expand All @@ -50,11 +51,12 @@ let
patches = [];
})).override { isMinimalBuild = true; };
in crossPkgs.mkShell {
buildInputs = [
buildInputs = lib.optionals (capnprotoVersion != "none") [
capnproto
];
nativeBuildInputs = with pkgs; [
cmakeBuild
git
include-what-you-use
ninja
] ++ lib.optionals (!minimal) [
Expand All @@ -64,4 +66,7 @@ in crossPkgs.mkShell {

# Tell IWYU where its libc++ mapping lives
IWYU_MAPPING_FILE = if enableLibcxx then "${llvm.libcxx.dev}/include/c++/v1/libcxx.imp" else null;

# Avoid "SSL certificate problem: unable to get local issuer certificate" error during git clone in ci/scripts/ci.sh
NIX_SSL_CERT_FILE = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt";
}
Loading