Skip to content
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

[libc++] Move the check-generated-files job to Github Actions #68920

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 12, 2023

This allows running these quick checks faster than in our Buildkite pipeline, which has much more latency. This will also avoid blocking the rest of the testing pipeline in case the generated-files checks are failing.

@ldionne ldionne requested a review from a team as a code owner October 12, 2023 18:35
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. github:workflow labels Oct 12, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 12, 2023

@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This allows running these quick checks faster than in our Buildkite pipeline, which has much more latency. This will also avoid blocking the rest of the testing pipeline in case the generated-files checks are failing.


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

2 Files Affected:

  • (added) .github/workflows/libcxx-check-generated-files.yml (+29)
  • (modified) libcxx/utils/ci/buildkite-pipeline.yml (-18)
diff --git a/.github/workflows/libcxx-check-generated-files.yml b/.github/workflows/libcxx-check-generated-files.yml
new file mode 100644
index 000000000000000..609065c1e1c38aa
--- /dev/null
+++ b/.github/workflows/libcxx-check-generated-files.yml
@@ -0,0 +1,29 @@
+name: "Check libc++ generated files"
+on:
+  pull_request_target:
+    paths:
+      - 'libcxx/**'
+permissions:
+  pull-requests: write
+
+jobs:
+  check_generated_files:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Fetch LLVM sources
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 2
+
+      - name: Install clang-format
+        uses: aminya/setup-cpp@v1
+        with:
+          clangformat: 17.0.1
+
+      - name: Install Ninja
+        uses: seanmiddleditch/gha-setup-ninja@master
+        with:
+          destination: build/ninja-install
+
+      - name: Check generated files
+        run: libcxx/utils/ci/run-buildbot check-generated-output
diff --git a/libcxx/utils/ci/buildkite-pipeline.yml b/libcxx/utils/ci/buildkite-pipeline.yml
index ebfb35eee91e1ed..8866a33d04d5a2d 100644
--- a/libcxx/utils/ci/buildkite-pipeline.yml
+++ b/libcxx/utils/ci/buildkite-pipeline.yml
@@ -33,24 +33,6 @@ steps:
   #
   # Light pre-commit tests for things like forgetting to update generated files.
   #
-  - label: "Generated output"
-    command: "libcxx/utils/ci/run-buildbot check-generated-output"
-    artifact_paths:
-      - "**/generated_output.patch"
-      - "**/generated_output.status"
-    env:
-        CC: "clang-${LLVM_HEAD_VERSION}"
-        CXX: "clang++-${LLVM_HEAD_VERSION}"
-        CLANG_FORMAT: "/usr/bin/clang-format-${LLVM_STABLE_VERSION}"
-    agents:
-      queue: "libcxx-builders"
-      os: "linux"
-    retry:
-      automatic:
-        - exit_status: -1  # Agent was lost
-          limit: 2
-    timeout_in_minutes: 120
-
   - label: "Documentation"
     command: "libcxx/utils/ci/run-buildbot documentation"
     artifact_paths:

@EricWF
Copy link
Member

EricWF commented Oct 12, 2023

Can we just use & fix the existing formatting check?

@tstellar tstellar self-requested a review October 12, 2023 18:46
@ldionne
Copy link
Member Author

ldionne commented Oct 12, 2023

Can we just use & fix the existing formatting check?

As I explained on Discord:

There are two CI jobs in question here:

  1. The formatting job that was set up by @tobiashieta . This job ensures that whatever changes have been touched in a PR are formatted properly except for any file in ignore_format.txt. That job works as intended and the only problem with it is that it skips libc++ headers that have no extension.
  2. The check-generated-output job. That job runs various generation scripts to re-generate our modulemaps and a few other things, and then ensures that they have been re-generated by whoever created a PR. As part of this job, we ensure that the list of files to ignore format in (ignore_format.txt) is kept up to date. 90% of the problems we've been talking about in this thread have to do with the fact that this job has a few flaws

This patch tries to incrementally fix issues with (2)

Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

I would like a response to the comment about security, but otherwise LGTM.

clangformat: 17.0.1

- name: Install Ninja
uses: seanmiddleditch/gha-setup-ninja@master
Copy link
Member

Choose a reason for hiding this comment

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

Are we using these actions elsewhere? Because using random actions associated with the project might be a security risk?

Copy link
Member

Choose a reason for hiding this comment

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

We could specify a different container and skip a lot of these steps, no?

Copy link
Collaborator

@tstellar tstellar Oct 12, 2023

Choose a reason for hiding this comment

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

We have our own action for installing ninja: llvm/actions/install-ninja

But do we really need this if the job is always going to run on Linux? Can we just apt install it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it more idiomatic to use llvm/actions/install-ninja or to install it manually? I don't really mind either way. I think I've seen uses: being used more often in actions but I don't mind using apt-get directly either.

Copy link
Member

Choose a reason for hiding this comment

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

If we build a container and run the job in that container, we'll save time each iteration of the job, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I do want to investigate setting up a container for libc++ jobs and storing that in the GH Docker registry to gradually replace our current DockerHub thing, but I would like to tackle that as a separate task to improve the CI situation in the short term.

ldionne added a commit to ldionne/llvm-project that referenced this pull request Oct 12, 2023
This updates the clang-format we use in libc++ to 17. This is necessary
to start running the generated-files checks in GitHub Actions (in llvm#68920).
In fact this is a pre-existing issue regardless of llvm#68920 -- right now
our ignore_format.txt job disagrees with the LLVM-wide clang-format job.
@ldionne ldionne force-pushed the review/generated-files-action branch from c167ea6 to 8667396 Compare October 12, 2023 21:23
ldionne added a commit that referenced this pull request Oct 12, 2023
This updates the clang-format we use in libc++ to 17. This is necessary
to start running the generated-files checks in GitHub Actions (in
#68920). In fact this is a pre-existing issue regardless of #68920 --
right now our ignore_format.txt job disagrees with the LLVM-wide
clang-format job.
@ldionne ldionne force-pushed the review/generated-files-action branch from 8667396 to 24f81ee Compare October 12, 2023 21:32
This allows running these quick checks faster than in our Buildkite
pipeline, which has much more latency. This will also avoid blocking
the rest of the testing pipeline in case the generated-files checks
are failing.
@ldionne ldionne force-pushed the review/generated-files-action branch from 24f81ee to d1c371b Compare October 12, 2023 21:39
@ldionne
Copy link
Member Author

ldionne commented Oct 12, 2023

Sorry for the churn on the patch -- AFAICT this should be ready to review again now. I'm using on: pull_request instead to avoid security issues, and I tested this in ldionne#4.

Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

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

Looks good with some suggestions.

.github/workflows/libcxx-check-generated-files.yml Outdated Show resolved Hide resolved
.github/workflows/libcxx-check-generated-files.yml Outdated Show resolved Hide resolved
@ldionne
Copy link
Member Author

ldionne commented Oct 17, 2023

@tstellar @EricWF Do you folks have more comments on this patch?

@ldionne ldionne merged commit 1196e6d into llvm:main Oct 19, 2023
4 checks passed
@ldionne ldionne deleted the review/generated-files-action branch October 19, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github:workflow libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants