[clang][ci] Move libc++ testing into the main PR pipeline #93318
Conversation
|
https://buildkite.com/llvm-project/github-pull-requests/builds/67044 has both new additions and I see the main pipeIine used to take anywhere between 8 and 12 minutes based on a recently completed builds for Clang changes I see, so there's some error in those measurements. Based on the steps that we don't need to do anymore, we should be saving 4-6 minutes per build:
|
Done. |
Sure. libc++ jobs are not really new: I basically copied them over from llvm-project/clang/utils/ci/run-buildbot Lines 94 to 147 in b9d40a7 |
Endilll
added
infrastructure
|
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-clang Author: Vlad Serebrennikov (Endilll) ChangesFollowing the discussion in #93233 (comment), this patch merges Additional work we skip and total time savings we should see:
Note that because previously-split jobs for each mode are now under a single Linux job, it now takes around 8 minutes more see the Linux CI results despite total time savings. The primary goal of this patch is to reduce the load of CI by removing duplicated work. I consider this goal achieved. I could keep the job parallelism we had (3 libc++ jobs depending on a main Linux job), but I don't consider it worth the effort and opportunity cost, because parallelism is not helping once the pool of builders is fully subscribed. Full diff: https://github.com/llvm/llvm-project/pull/93318.diff 3 Files Affected:
diff --git a/.ci/generate-buildkite-pipeline-premerge b/.ci/generate-buildkite-pipeline-premerge
index e1c66ac18e7ac..3ed5eb96eceb5 100755
--- a/.ci/generate-buildkite-pipeline-premerge
+++ b/.ci/generate-buildkite-pipeline-premerge
@@ -85,6 +85,22 @@ function compute-projects-to-test() {
done
}
+function compute-runtimes-to-test() {
+ projects=${@}
+ for project in ${projects}; do
+ case ${project} in
+ clang)
+ for p in libcxx libcxxabi libunwind; do
+ echo $p
+ done
+ ;;
+ *)
+ # Nothing to do
+ ;;
+ esac
+ done
+}
+
function add-dependencies() {
projects=${@}
for project in ${projects}; do
@@ -178,6 +194,15 @@ function check-targets() {
cross-project-tests)
echo "check-cross-project"
;;
+ libcxx)
+ echo "check-cxx"
+ ;;
+ libcxxabi)
+ echo "check-cxxabi"
+ ;;
+ libunwind)
+ echo "check-unwind"
+ ;;
lldb)
echo "check-all" # TODO: check-lldb may not include all the LLDB tests?
;;
@@ -207,17 +232,6 @@ if echo "$modified_dirs" | grep -q -E "^(libcxx|libcxxabi|libunwind|runtimes|cma
EOF
fi
-# If clang changed.
-if echo "$modified_dirs" | grep -q -E "^(clang)$"; then
- cat <<EOF
-- trigger: "clang-ci"
- build:
- message: "${buildMessage}"
- commit: "${BUILDKITE_COMMIT}"
- branch: "${BUILDKITE_BRANCH}"
-EOF
-fi
-
# Generic pipeline for projects that have not defined custom steps.
#
# Individual projects should instead define the pre-commit CI tests that suits their
@@ -231,6 +245,10 @@ linux_projects_to_test=$(exclude-linux $(compute-projects-to-test ${modified_pro
linux_check_targets=$(check-targets ${linux_projects_to_test} | sort | uniq)
linux_projects=$(add-dependencies ${linux_projects_to_test} | sort | uniq)
+linux_runtimes_to_test=$(compute-runtimes-to-test ${linux_projects_to_test})
+linux_runtime_check_targets=$(check-targets ${linux_runtimes_to_test} | sort | uniq)
+linux_runtimes=$(echo ${linux_runtimes_to_test} | sort | uniq)
+
windows_projects_to_test=$(exclude-windows $(compute-projects-to-test ${modified_projects}))
windows_check_targets=$(check-targets ${windows_projects_to_test} | sort | uniq)
windows_projects=$(add-dependencies ${windows_projects_to_test} | sort | uniq)
@@ -255,7 +273,7 @@ if [[ "${linux_projects}" != "" ]]; then
CC: 'clang'
CXX: 'clang++'
commands:
- - './.ci/monolithic-linux.sh "$(echo ${linux_projects} | tr ' ' ';')" "$(echo ${linux_check_targets})"'
+ - './.ci/monolithic-linux.sh "$(echo ${linux_projects} | tr ' ' ';')" "$(echo ${linux_check_targets})" "$(echo ${linux_runtimes} | tr ' ' ';')" "$(echo ${linux_runtime_check_targets})"'
EOF
fi
diff --git a/.ci/monolithic-linux.sh b/.ci/monolithic-linux.sh
index b00a4b984a1d2..bbd90f7d496b1 100755
--- a/.ci/monolithic-linux.sh
+++ b/.ci/monolithic-linux.sh
@@ -54,3 +54,68 @@ cmake -S "${MONOREPO_ROOT}"/llvm -B "${BUILD_DIR}" \
echo "--- ninja"
# Targets are not escaped as they are passed as separate arguments.
ninja -C "${BUILD_DIR}" -k 0 ${targets}
+
+runtimes="${3}"
+runtime_targets="${4}"
+
+# Compiling runtimes with just-built Clang and running their tests
+# as an additional testing for Clang.
+if [[ "${runtimes}" != "" ]]; then
+ if [[ "${runtime_targets}" == "" ]]; then
+ echo "Runtimes to build are specified, but targets are not."
+ fi
+
+ RUNTIMES_BUILD_DIR="${MONOREPO_ROOT}/build-runtimes"
+ INSTALL_DIR="${RUNTIMES_BUILD_DIR}/install"
+ mkdir -p ${RUNTIMES_BUILD_DIR}
+
+ echo "--- cmake runtimes C++03"
+
+ cmake -S "${MONOREPO_ROOT}/runtimes" -B "${RUNTIMES_BUILD_DIR}" -GNinja \
+ -DCMAKE_C_COMPILER="${BUILD_DIR}/bin/clang" \
+ -DCMAKE_CXX_COMPILER="${BUILD_DIR}/bin/clang++" \
+ -DLLVM_ENABLE_RUNTIMES="${runtimes}" \
+ -DLIBCXX_CXX_ABI=libcxxabi \
+ -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+ -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
+ -DLIBCXX_TEST_PARAMS="std=c++03" \
+ -DLIBCXXABI_TEST_PARAMS="std=c++03"
+
+ echo "--- ninja runtimes C++03"
+
+ ninja -vC "${RUNTIMES_BUILD_DIR}" ${runtime_targets}
+
+ echo "--- cmake runtimes C++26"
+
+ rm -rf "${RUNTIMES_BUILD_DIR}"
+ cmake -S "${MONOREPO_ROOT}/runtimes" -B "${RUNTIMES_BUILD_DIR}" -GNinja \
+ -DCMAKE_C_COMPILER="${BUILD_DIR}/bin/clang" \
+ -DCMAKE_CXX_COMPILER="${BUILD_DIR}/bin/clang++" \
+ -DLLVM_ENABLE_RUNTIMES="${runtimes}" \
+ -DLIBCXX_CXX_ABI=libcxxabi \
+ -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+ -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
+ -DLIBCXX_TEST_PARAMS="std=c++26" \
+ -DLIBCXXABI_TEST_PARAMS="std=c++26"
+
+ echo "--- ninja runtimes C++26"
+
+ ninja -vC "${RUNTIMES_BUILD_DIR}" ${runtime_targets}
+
+ echo "--- cmake runtimes clang modules"
+
+ rm -rf "${RUNTIMES_BUILD_DIR}"
+ cmake -S "${MONOREPO_ROOT}/runtimes" -B "${RUNTIMES_BUILD_DIR}" -GNinja \
+ -DCMAKE_C_COMPILER="${BUILD_DIR}/bin/clang" \
+ -DCMAKE_CXX_COMPILER="${BUILD_DIR}/bin/clang++" \
+ -DLLVM_ENABLE_RUNTIMES="${runtimes}" \
+ -DLIBCXX_CXX_ABI=libcxxabi \
+ -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+ -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
+ -DLIBCXX_TEST_PARAMS="enable_modules=clang" \
+ -DLIBCXXABI_TEST_PARAMS="enable_modules=clang"
+
+ echo "--- ninja runtimes clang modules"
+
+ ninja -vC "${RUNTIMES_BUILD_DIR}" ${runtime_targets}
+fi
diff --git a/clang/utils/ci/buildkite-pipeline.yml b/clang/utils/ci/buildkite-pipeline.yml
deleted file mode 100644
index 86cfcf35cc867..0000000000000
--- a/clang/utils/ci/buildkite-pipeline.yml
+++ /dev/null
@@ -1,82 +0,0 @@
-#===----------------------------------------------------------------------===##
-#
-# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-# See https://llvm.org/LICENSE.txt for license information.
-# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-#
-#===----------------------------------------------------------------------===##
-
-#
-# This file describes the various pre-commit CI bots used to test Clang against
-# libc++ under various configurations. Unlike the usual libc++ CI pipeline,
-# which aims to test libc++ itself, this pipeline aims to test Clang by
-# compiling libc++ and running its test suite against the just-built Clang,
-# in various configurations.
-#
-env:
- # LLVM RELEASE bump version
- LLVM_HEAD_VERSION: "17"
-steps:
- - label: "Building Clang (Linux)"
- commands:
- - "clang/utils/ci/run-buildbot build-clang"
- agents:
- queue: "linux"
- retry:
- automatic:
- - exit_status: -1 # Agent was lost
- limit: 2
- timeout_in_minutes: 120
-
- - wait
-
- - label: "Testing libc++ with just-built Clang (C++03)"
- commands:
- - "clang/utils/ci/run-buildbot generic-cxx03"
- artifact_paths:
- - "**/test-results.xml"
- - "**/crash_diagnostics/*"
- env:
- LLVM_SYMBOLIZER_PATH: "/usr/bin/llvm-symbolizer-${LLVM_HEAD_VERSION}" # TODO: Should we build that from scratch?
- CLANG_CRASH_DIAGNOSTICS_DIR: "crash_diagnostics"
- agents:
- queue: "linux"
- retry:
- automatic:
- - exit_status: -1 # Agent was lost
- limit: 2
- timeout_in_minutes: 120
-
- - label: "Testing libc++ with just-built Clang (C++26)"
- commands:
- - "clang/utils/ci/run-buildbot generic-cxx26"
- artifact_paths:
- - "**/test-results.xml"
- - "**/crash_diagnostics/*"
- env:
- LLVM_SYMBOLIZER_PATH: "/usr/bin/llvm-symbolizer-${LLVM_HEAD_VERSION}" # TODO: Should we build that from scratch?
- CLANG_CRASH_DIAGNOSTICS_DIR: "crash_diagnostics"
- agents:
- queue: "linux"
- retry:
- automatic:
- - exit_status: -1 # Agent was lost
- limit: 2
- timeout_in_minutes: 120
-
- - label: "Testing libc++ with just-built Clang (w/ Clang Modules)"
- commands:
- - "clang/utils/ci/run-buildbot generic-modules"
- artifact_paths:
- - "**/test-results.xml"
- - "**/crash_diagnostics/*"
- env:
- LLVM_SYMBOLIZER_PATH: "/usr/bin/llvm-symbolizer-${LLVM_HEAD_VERSION}" # TODO: Should we build that from scratch?
- CLANG_CRASH_DIAGNOSTICS_DIR: "crash_diagnostics"
- agents:
- queue: "linux"
- retry:
- automatic:
- - exit_status: -1 # Agent was lost
- limit: 2
- timeout_in_minutes: 120
|
A follow up for #93318. Discussion happened at #93318 (comment)
A follow up for llvm#93318. Discussion happened at llvm#93318 (comment)
| @@ -231,6 +245,10 @@ linux_projects_to_test=$(exclude-linux $(compute-projects-to-test ${modified_pro | |||
| linux_check_targets=$(check-targets ${linux_projects_to_test} | sort | uniq) | |||
| linux_projects=$(add-dependencies ${linux_projects_to_test} | sort | uniq) | |||
|
|
|||
| linux_runtimes_to_test=$(compute-runtimes-to-test ${linux_projects_to_test}) | |||
There was a problem hiding this comment.
I was trying to understand why my PR which touches LLVM and MLIR but not clang triggered all the runtime tests, and ended up here.
It's not clear to me why is the compute-runtimes-to-test computed from the linux_projects_to_test and not from the modified_projects?
That is when clang is added to linux_projects_to_test because something else is modified (like LLVM), why do we need to test the runtimes?
There was a problem hiding this comment.
I believe this line does what it's supposed to do. Runtimes are tested here not for their sake, but as an additional testing for Clang.
So, if you make a change in LLVM, it's clear that Clang might be affected, so it needs to be tested. Runtimes are acting as an extension of clang/test.
There was a problem hiding this comment.
Sure, in theory... But that seems like a stretch to me.
I remember recently that clang folks argued to removed the flang tests when changing clang because of the likelihood of breaking Flang with clang tests.
I'm not sure the probability of a random LLVM change breaking clang in a way that would only show up with a runtime tests and no other LLVM/clang test is higher!
The ratio value / cost isn't clear to me right now.
There was a problem hiding this comment.
I remember recently that clang folks argued to removed the flang tests when changing clang because of the likelihood of breaking Flang with clang tests.
I believe Flang was tested on Clang changes to make sure Clang didn't make any incompatible changes to the driver, which Flang reuses. This is a different use case from testing Clang itself, and yes, we believed (and likely still) that it didn't bring enough value, especially when it was rendering the whole Clang CI unusable.
I'm not sure the probability of a random LLVM change breaking clang in a way that would only show up with a runtime tests and no other LLVM/clang test is higher!
I find it hard to prove this statement either way. But I find it much easier to believe that (a) Clang is insufficiently tested; (b) libc++ tests bring a lot of coverage. So I believe the value is there, and the cost is building the tests and running them, which is as good as it gets.
Additionally, "don't run those Clang tests if Clang is tested because of an LLVM change" kind of logic make it harder to reason about CI. Statements like "this change has passed Clang CI" would require additional context to understand which tests were actually ran.
The ratio value / cost isn't clear to me right now.
To me the ratio is rather clear. If you want to push for this, you'd have to open a PR yourself.
There was a problem hiding this comment.
I believe Flang was tested on Clang changes to make sure Clang didn't make any incompatible changes to the driver, which Flang reuses. This is a different use case from testing Clang itself,
I see exactly the same kind of transitive dependency issue: one project depends on another and running all the possible tests for the dependent project on a change to the upstream project is deemed "not worth it". We reduced the coverage for Flang tests for the sake of Clang "velocity": this is exactly the same for LLVM here.
Additionally, "don't run those Clang tests if Clang is tested because of an LLVM change" kind of logic make it harder to reason about CI.
Referring to "clang CI" is a rather vague statement to me: testing in generally goes into "tiers" and the "premerge" tiers is about tradeoff in terms of value/cost, while you're making a completeness arguments. There are tons of things that we don't test in pre-merge.
Also the buildbot does not seem to cover this, which is quite annoying because now pre-merge is broken (see https://buildkite.com/llvm-project/github-pull-requests/builds/104995#019239b1-1aaa-411a-9fd2-9dcfdce25c3d ) by libcxx while the post merge is green, which should only ever happen if the PR is responsible for the breakage.
I find it hard to prove this statement either way.
It's not very hard: the data may be difficult to surface but it exists. How many PR touching only LLVM failed libcxx tests but not clang?
We could also disable this and waiting for a merge to create this condition to even find a single occurence.
Following the discussion in #93233 (comment), this patch merges
clang-cipipeline into mainGitHub Pull Requestspipeline.clang-cienables additional test coverage for Clang by compiling it, and then using it to compile and test libc++, libc++abi, and libunwind in C++03, C++26, and Clang Modules modes.Additional work we skip and total time savings we should see:
Note that because previously-split jobs for each mode are now under a single Linux job, it now takes around 8 minutes more see the Linux CI results despite total time savings.
The primary goal of this patch is to reduce the load of CI by removing duplicated work. I consider this goal achieved. I could keep the job parallelism we had (3 libc++ jobs depending on a main Linux job), but I don't consider it worth the effort and opportunity cost, because parallelism is not helping once the pool of builders is fully subscribed.