Skip to content

[clang][ci] Remove unnecessary BuildKite jobs for Clang #93233

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

Merged
merged 2 commits into from
May 23, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented May 23, 2024

  1. Remove the format-checking job from the BuildKite pipeline. We now have a monorepo-wide format checker implemented with Github Actions, so that should not be necessary anymore.

  2. Stop building and testing Clang on Windows via the clang-ci pipeline. We already do that in the github-pull-requests pipeline, so that's just duplicate work.

  3. Stop testing Clang on Linux in the clang-ci pipeline. We already do that in the github-pull-requests pipeline too, so that's also duplicate work. For now we still build Clang because the other jobs in the clang-ci pipeline require its artifacts, but that could be improved.

1. Remove the format-checking job from the BuildKite pipeline.
   We now have a monorepo-wide format checker implemented with
   Github Actions, so that should not be necessary anymore.

2. Stop building and testing Clang on Windows via the clang-ci
   pipeline. We already do that in the github-pull-requests pipeline,
   so that's just duplicate work.

3. Stop testing Clang on Linux in the clang-ci pipeline. We already
   do that in the github-pull-requests pipeline too, so that's also
   duplicate work. For now we still build Clang because the other jobs
   in the clang-ci pipeline require its artifacts, but that could be
   improved.
@ldionne ldionne requested review from tstellar and Endilll May 23, 2024 19:36
@llvmbot llvmbot added the clang Clang issues not falling into any other category label May 23, 2024
@llvmbot
Copy link
Member

llvmbot commented May 23, 2024

@llvm/pr-subscribers-clang

Author: Louis Dionne (ldionne)

Changes
  1. Remove the format-checking job from the BuildKite pipeline. We now have a monorepo-wide format checker implemented with Github Actions, so that should not be necessary anymore.

  2. Stop building and testing Clang on Windows via the clang-ci pipeline. We already do that in the github-pull-requests pipeline, so that's just duplicate work.

  3. Stop testing Clang on Linux in the clang-ci pipeline. We already do that in the github-pull-requests pipeline too, so that's also duplicate work. For now we still build Clang because the other jobs in the clang-ci pipeline require its artifacts, but that could be improved.


Full diff: https://github.com/llvm/llvm-project/pull/93233.diff

2 Files Affected:

  • (modified) clang/utils/ci/buildkite-pipeline.yml (+4-27)
  • (modified) clang/utils/ci/run-buildbot (-24)
diff --git a/clang/utils/ci/buildkite-pipeline.yml b/clang/utils/ci/buildkite-pipeline.yml
index 7a679176038c6..86cfcf35cc867 100644
--- a/clang/utils/ci/buildkite-pipeline.yml
+++ b/clang/utils/ci/buildkite-pipeline.yml
@@ -17,18 +17,7 @@ env:
     # LLVM RELEASE bump version
     LLVM_HEAD_VERSION: "17"
 steps:
-  - label: "Format"
-    commands:
-      - "clang/utils/ci/run-buildbot check-format"
-    agents:
-      queue: "linux"
-    retry:
-      automatic:
-        - exit_status: -1  # Agent was lost
-          limit: 2
-    timeout_in_minutes: 120
-
-  - label: "Building and testing clang (Linux)"
+  - label: "Building Clang (Linux)"
     commands:
       - "clang/utils/ci/run-buildbot build-clang"
     agents:
@@ -39,21 +28,9 @@ steps:
           limit: 2
     timeout_in_minutes: 120
 
-  - label: "Building and testing clang (Windows)"
-    commands:
-      - "C:\\BuildTools\\Common7\\Tools\\VsDevCmd.bat -arch=amd64 -host_arch=amd64"
-      - "bash clang/utils/ci/run-buildbot build-clang-windows"
-    agents:
-      queue: "windows"
-    retry:
-      automatic:
-        - exit_status: -1  # Agent was lost
-          limit: 2
-    timeout_in_minutes: 120
-
   - wait
 
-  - label: "Running libc++ test suite in C++03"
+  - label: "Testing libc++ with just-built Clang (C++03)"
     commands:
       - "clang/utils/ci/run-buildbot generic-cxx03"
     artifact_paths:
@@ -70,7 +47,7 @@ steps:
           limit: 2
     timeout_in_minutes: 120
 
-  - label: "Running libc++ test suite in C++26"
+  - label: "Testing libc++ with just-built Clang (C++26)"
     commands:
       - "clang/utils/ci/run-buildbot generic-cxx26"
     artifact_paths:
@@ -87,7 +64,7 @@ steps:
           limit: 2
     timeout_in_minutes: 120
 
-  - label: "Running libc++ test suite with Clang Modules"
+  - label: "Testing libc++ with just-built Clang (w/ Clang Modules)"
     commands:
       - "clang/utils/ci/run-buildbot generic-modules"
     artifact_paths:
diff --git a/clang/utils/ci/run-buildbot b/clang/utils/ci/run-buildbot
index f47ffb5cbd38d..6490d8e8f5b3b 100755
--- a/clang/utils/ci/run-buildbot
+++ b/clang/utils/ci/run-buildbot
@@ -69,13 +69,6 @@ cmake --version
 ninja --version
 
 case "${BUILDER}" in
-check-format)
-    echo "*** Checking for trailing whitespace left in Clang source files ***"
-    if grep -rnI '[[:blank:]]$' clang/lib clang/include clang/docs; then
-        echo "*** Trailing whitespace has been found in Clang source files as described above ***"
-        exit 1
-    fi
-;;
 build-clang)
     mkdir install
     # We use Release here to avoid including debug information. Otherwise, the
@@ -96,23 +89,6 @@ build-clang)
     ccache -s
     tar -cJvf install.tar.xz install/
     buildkite-agent artifact upload --debug install.tar.xz
-
-    ninja -C ${BUILD_DIR} check-clang
-;;
-build-clang-windows)
-    cmake -S llvm -B ${BUILD_DIR} -G Ninja                                      \
-        -D CMAKE_C_COMPILER_LAUNCHER=sccache                                    \
-        -D CMAKE_CXX_COMPILER_LAUNCHER=sccache                                  \
-        -D CMAKE_BUILD_TYPE=Release                                             \
-        -D CMAKE_INSTALL_PREFIX=install-windows                                 \
-        -D LLVM_ENABLE_PROJECTS="clang;compiler-rt"                             \
-        -D LLVM_ENABLE_ASSERTIONS=ON                                            \
-        -D LLVM_BUILD_EXAMPLES=ON                                               \
-        -D COMPILER_RT_BUILD_LIBFUZZER=OFF                                      \
-        -D COMPILER_RT_BUILD_ORC=OFF
-
-    ninja -C ${BUILD_DIR} install-clang install-clang-resource-headers
-    ninja -C ${BUILD_DIR} check-clang
 ;;
 generic-cxx03)
     buildkite-agent artifact download install.tar.xz .

Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

If we aren't running the tests any more, can we build clang with -DLLVM_TARGETS_TO_BUILD=Native

@Endilll
Copy link
Contributor

Endilll commented May 23, 2024

Do we even need to build Clang and run libc++ jobs? I don't even see them linked in libc++ PR, even besides the fact that I think libc++ is fully tested with GitHub Actions.

@ldionne
Copy link
Member Author

ldionne commented May 23, 2024

Do we even need to build Clang and run libc++ jobs? I don't even see them linked in libc++ PR, even besides the fact that I think libc++ is fully tested with GitHub Actions.

What we're testing is that the changes to Clang didn't break libc++. We're testing Clang via libc++ -- we're not testing libc++ itself. So yes, this is still relevant.

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

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

OK, I'll try to add this testing to "GitHub Pull Requests" later so that we can avoid building Clang twice.

Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, this should help free up some builder resources.

@ldionne
Copy link
Member Author

ldionne commented May 23, 2024

I will merge once the CI has passed. If someone else sees the CI green, feel free to merge too.

@Endilll We could indeed "flatten" the clang-ci pipeline into the github-pull-requests pipeline. Basically, instead of triggering the clang-ci pipeline like this: https://github.com/llvm/llvm-project/blob/main/.ci/generate-buildkite-pipeline-premerge#L213

We would instead expand to the pipeline itself and it would run inside the github-pull-requests pipeline itself. It would then be easier to share artifacts between jobs within that pipeline, and the clang-ci pipeline could go away entirely.

The same treatment could potentially be applied to libc++ although there will be other challenges (namely generating a valid pipeline YAML inside .ci/generate-buildkite-pipeline-premerge may be challenging).

@Endilll Endilll merged commit dc1bfbc into llvm:main May 23, 2024
8 checks passed
@ldionne ldionne deleted the review/cleanup-clang-ci-buildkite branch May 23, 2024 21:57
Endilll added a commit that referenced this pull request May 27, 2024
Following the discussion in
#93233 (comment),
this patch merges `clang-ci` pipeline into main `GitHub Pull Requests`
pipeline. `clang-ci` enables 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:
1. Checking out the repo to generate the clang-ci pipeline (2 minutes)
2. Building Clang (3.5 minutes)
3. Uploading the artifacts once, then downloading them 3 times and
unpacking 3 times (0.5 minutes)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants