-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Make CC and CXX environment variables mandatory in run-buildbot #166875
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: main
Are you sure you want to change the base?
Conversation
philnik777
left a comment
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.
IMO it'd be better if we just require CC and CXX. These defaults are probably wrong on most systems. For the CCACHE part I'd be much happier if we don't use any unless the user explicitly sets CCACHE.
|
@llvm/pr-subscribers-github-workflow @llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThe bootstrapping-build job defined in run-buildbot required the CC and CXX environment variables to be defined even though run-buildbot documents these environment variables as being optional. It also required ccache, which may not be available for local builds. This patch tweaks the script slightly to better deal with the absence of these environment variables/tools. Full diff: https://github.com/llvm/llvm-project/pull/166875.diff 1 Files Affected:
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 57ecf1e49dbf2..b7dcafaa3f68c 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -96,6 +96,14 @@ if [ -z "${CMAKE}" ]; then
fi
fi
+if [ -z "${CC}" ]; then
+ CC=cc
+fi
+
+if [ -z "${CXX}" ]; then
+ CXX=c++
+fi
+
function step() {
endstep
set +x
@@ -121,6 +129,13 @@ function error() {
echo "::error::$1"
}
+# Print the version of a few tools to aid diagnostics in some cases
+step "Diagnose tools in use"
+${CMAKE} --version
+${NINJA} --version
+${CC} --version
+${CXX} --version
+
function clean() {
rm -rf "${BUILD_DIR}"
}
@@ -240,12 +255,6 @@ function test-armv7m-picolibc() {
check-runtimes
}
-# Print the version of a few tools to aid diagnostics in some cases
-step "Diagnose tools in use"
-${CMAKE} --version
-${NINJA} --version
-if [ ! -z "${CXX}" ]; then ${CXX} --version; fi
-
case "${BUILDER}" in
check-generated-output)
# `! foo` doesn't work properly with `set -e`, use `! foo || false` instead.
@@ -382,12 +391,16 @@ generic-ubsan)
bootstrapping-build)
clean
+ if which ccache; then
+ CCACHE="-DCMAKE_CXX_COMPILER_LAUNCHER=ccache"
+ fi
+
step "Generating CMake"
${CMAKE} \
-S "${MONOREPO_ROOT}/llvm" \
-B "${BUILD_DIR}" \
-GNinja -DCMAKE_MAKE_PROGRAM="${NINJA}" \
- -DCMAKE_CXX_COMPILER_LAUNCHER="ccache" \
+ ${CCACHE} \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
-DLLVM_ENABLE_PROJECTS="clang;lldb" \
|
The bootstrapping-build job defined in run-buildbot required the CC and CXX environment variables to be defined even though run-buildbot documents these environment variables as being optional. It also required ccache, which may not be available for local builds. This patch tweaks the script slightly to better deal with the absence of these environment variables/tools.
09cc432 to
1129186
Compare
Previously, the bootstrapping-build job defined in run-buildbot required the CC and CXX environment variables to be defined even though run-buildbot documents these environment variables as being optional. It also relied on ccache being available.
Refactor run-buildbot to make CC and CXX mandatory, and refactor various places in the CI where we called run-buildbot without setting CC and CXX. After this patch, all places that use run-buildbot are setting CC and CXX before calling the script, which makes it easier to track what compiler is used where. This also allows simplifying run-buildbot itself.
Finally, this patch makes ccache optional for running the bootstrapping build.