Skip to content

[clang][ci] Move libc++ testing into the main PR pipeline #93318

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
May 27, 2024
Merged

[clang][ci] Move libc++ testing into the main PR pipeline #93318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
16ec4c7
[clang][ci] Move libc++ testing into the main PR pipeline
Endilll May 24, 2024
5be9db1
Add missing semicolons
Endilll May 24, 2024
d2ce8f2
Make a test change in clang
Endilll May 24, 2024
a585ece
Add `set -x` to pipeline generating script
Endilll May 24, 2024
9160288
Add missing echo
Endilll May 24, 2024
ad6a5df
Add more debug echoes
Endilll May 24, 2024
5d56a7e
Re-enable `set -x`
Endilll May 24, 2024
67ed5ce
Removed unncessary echo
Endilll May 24, 2024
6cd1e08
Remove debug echoes
Endilll May 24, 2024
cac43ab
Remove `set -x`
Endilll May 24, 2024
9bbf851
Remove `clang-ci` trigger from the main pipeline
Endilll May 24, 2024
e2ba15e
Revert "Make a test change in clang"
Endilll May 24, 2024
a739134
Delete `buildkite-pipeline.yml`
Endilll May 24, 2024
8d10d51
Add whitespaces to CMake arguments
Endilll May 24, 2024
9c1b04a
Intall Clang and use it for libc++ builds
Endilll May 24, 2024
34c2cb9
Add non-zero exit when only 3 arguments are passed to the `monolithic…
Endilll May 24, 2024
735def1
Fix `INSTALL_DIR` that was based off wrong path
Endilll May 24, 2024
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
42 changes: 30 additions & 12 deletions .ci/generate-buildkite-pipeline-premerge
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?
;;
Expand Down Expand Up @@ -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
Expand All @@ -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})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Endilll Endilll Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@joker-eph joker-eph Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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)
Expand All @@ -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

Expand Down
74 changes: 73 additions & 1 deletion .ci/monolithic-linux.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ set -o pipefail

MONOREPO_ROOT="${MONOREPO_ROOT:="$(git rev-parse --show-toplevel)"}"
BUILD_DIR="${BUILD_DIR:=${MONOREPO_ROOT}/build}"
INSTALL_DIR="${BUILD_DIR}/install"
rm -rf "${BUILD_DIR}"

ccache --zero-stats
Expand Down Expand Up @@ -49,8 +50,79 @@ cmake -S "${MONOREPO_ROOT}"/llvm -B "${BUILD_DIR}" \
-D LLVM_ENABLE_LLD=ON \
-D CMAKE_CXX_FLAGS=-gmlt \
-D LLVM_CCACHE_BUILD=ON \
-D MLIR_ENABLE_BINDINGS_PYTHON=ON
-D MLIR_ENABLE_BINDINGS_PYTHON=ON \
-D CMAKE_INSTALL_PREFIX="${INSTALL_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."
exit 1
fi

echo "--- ninja install-clang"

ninja -C ${BUILD_DIR} install-clang install-clang-resource-headers

RUNTIMES_BUILD_DIR="${MONOREPO_ROOT}/build-runtimes"
INSTALL_DIR="${BUILD_DIR}/install"
mkdir -p ${RUNTIMES_BUILD_DIR}

echo "--- cmake runtimes C++03"

cmake -S "${MONOREPO_ROOT}/runtimes" -B "${RUNTIMES_BUILD_DIR}" -GNinja \
-D CMAKE_C_COMPILER="${INSTALL_DIR}/bin/clang" \
-D CMAKE_CXX_COMPILER="${INSTALL_DIR}/bin/clang++" \
-D LLVM_ENABLE_RUNTIMES="${runtimes}" \
-D LIBCXX_CXX_ABI=libcxxabi \
-D CMAKE_BUILD_TYPE=RelWithDebInfo \
-D CMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
-D LIBCXX_TEST_PARAMS="std=c++03" \
-D LIBCXXABI_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 \
-D CMAKE_C_COMPILER="${INSTALL_DIR}/bin/clang" \
-D CMAKE_CXX_COMPILER="${INSTALL_DIR}/bin/clang++" \
-D LLVM_ENABLE_RUNTIMES="${runtimes}" \
-D LIBCXX_CXX_ABI=libcxxabi \
-D CMAKE_BUILD_TYPE=RelWithDebInfo \
-D CMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
-D LIBCXX_TEST_PARAMS="std=c++26" \
-D LIBCXXABI_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 \
-D CMAKE_C_COMPILER="${INSTALL_DIR}/bin/clang" \
-D CMAKE_CXX_COMPILER="${INSTALL_DIR}/bin/clang++" \
-D LLVM_ENABLE_RUNTIMES="${runtimes}" \
-D LIBCXX_CXX_ABI=libcxxabi \
-D CMAKE_BUILD_TYPE=RelWithDebInfo \
-D CMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
-D LIBCXX_TEST_PARAMS="enable_modules=clang" \
-D LIBCXXABI_TEST_PARAMS="enable_modules=clang"

echo "--- ninja runtimes clang modules"

ninja -vC "${RUNTIMES_BUILD_DIR}" ${runtime_targets}
fi
82 changes: 0 additions & 82 deletions clang/utils/ci/buildkite-pipeline.yml
View file
Open in desktop

This file was deleted.

Loading