Skip to content
Open
Changes from all commits
Commits
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
34 changes: 32 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ permissions:
actions: read
contents: read
packages: read
pull-requests: read

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can the whole PR be reduced to just

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index ce8191d..fbc1a50 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -8,6 +8,7 @@ permissions:
   actions: read
   contents: read
   packages: write
+  pull-requests: read
 
 concurrency:
   group: |
@@ -25,6 +26,7 @@ jobs:
     runs-on: ${{ vars.RUNNER_CHECK_SKIP || 'ubuntu-24.04-arm' }}
     outputs:
       skip: ${{ steps.skip-check.outputs.skip }}
+      run-build-tests: ${{ steps.classify-changes.outputs.run-build-tests }}
       runner-amd64: ${{ steps.select-runner.outputs.runner_amd64 }}
       runner-arm64: ${{ steps.select-runner.outputs.runner_arm64 }}
       use-blacksmith: ${{ steps.select-runner.outputs.use_blacksmith }}
@@ -47,6 +49,24 @@ jobs:
             echo "skip=false" >> $GITHUB_OUTPUT
           fi
 
+      - name: Classify changed paths
+        id: classify-changes
+        if: ${{ steps.skip-check.outputs.skip == 'false' }}
+        env:
+          GH_TOKEN: ${{ github.token }}
+        run: |
+          RUN=true
+          if [[ "${{ github.event_name }}" == "pull_request_target" ]]; then
+            FILES="$(gh api --paginate \
+              "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files" \
+              --jq '.[] | .filename, (.previous_filename // empty)')" || FILES=""
+            if [[ -n "$FILES" ]] && ! grep -qvE '\.md$' <<< "$FILES" && ! grep -qE '^src/' <<< "$FILES"; then
+              RUN=false
+            fi
+          fi
+          echo "Full build and test matrix: $RUN"
+          echo "run-build-tests=$RUN" >> "$GITHUB_OUTPUT"
+
       - name: Checkout code
         if: ${{ steps.skip-check.outputs.skip == 'false' }}
         uses: actions/checkout@v6
@@ -86,7 +106,9 @@ jobs:
   cache-sources:
     name: Cache depends sources
     needs: [check-skip]
-    if: ${{ needs.check-skip.outputs.skip == 'false' }}
+    if: |
+      needs.check-skip.outputs.skip == 'false' &&
+      needs.check-skip.outputs.run-build-tests == 'true'
     uses: ./.github/workflows/cache-depends-sources.yml
     with:
       runs-on: ${{ needs.check-skip.outputs['runner-arm64'] }}
@@ -94,7 +116,9 @@ jobs:
   container:
     name: Build container
     needs: [check-skip]
-    if: ${{ needs.check-skip.outputs.skip == 'false' }}
+    if: |
+      needs.check-skip.outputs.skip == 'false' &&
+      needs.check-skip.outputs.run-build-tests == 'true'
     uses: ./.github/workflows/build-container.yml
     with:
       context: ./contrib/containers/ci
-- 

?

This classify_ci_changes.py is over-complicated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. Reworked in 7ab20b0 to essentially this diff: a single bash step in check-skip gating container and cache-sources, with the PR number and repository passed via env instead of inline expressions. classify_ci_changes.py and its tests are gone.


🤖 Posted autonomously by Codex on behalf of pasta.


concurrency:
group: |
Expand All @@ -25,6 +26,7 @@ jobs:
runs-on: ${{ vars.RUNNER_CHECK_SKIP || 'ubuntu-24.04-arm' }}
outputs:
skip: ${{ steps.skip-check.outputs.skip }}
run-build-tests: ${{ steps.classify-changes.outputs.run-build-tests }}
runner-amd64: ${{ steps.select-runner.outputs.runner_amd64 }}
runner-arm64: ${{ steps.select-runner.outputs.runner_arm64 }}
runner-prelint: ${{ steps.select-runner.outputs.runner_prelint }}
Expand Down Expand Up @@ -53,6 +55,30 @@ jobs:
echo "skip=false" >> $GITHUB_OUTPUT
fi

- name: Classify changed paths
id: classify-changes
if: ${{ steps.skip-check.outputs.skip == 'false' }}
env:
EVENT_NAME: ${{ github.event_name }}
GH_TOKEN: ${{ github.token }}
CHANGED_FILES: ${{ github.event.pull_request.changed_files }}
PR_NUMBER: ${{ github.event.pull_request.number }}
REPOSITORY: ${{ github.repository }}
run: |
# Pull requests that only touch Markdown outside src/ skip the full
# build and test matrix. Any API failure, or a PR too large for the
# files endpoint (3000-file cap), leaves the full matrix on.
RUN=true
if [[ "$EVENT_NAME" == "pull_request_target" && "${CHANGED_FILES:-3000}" -lt 3000 ]]; then
FILES="$(gh api --paginate "repos/$REPOSITORY/pulls/$PR_NUMBER/files" \
--jq '.[] | .filename, (.previous_filename // empty)')" || FILES=""
Comment on lines +73 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fail open after the files endpoint limit

When a PR changes more than 3,000 files and a build-relevant path is outside the returned subset (for example, the returned entries are all doc/*.md additions while an omitted entry is src/net.cpp), this command still exits successfully with only Markdown paths, so RUN=false and the container/cache-sources gates skip every build and test job. gh api --help describes --paginate only as making additional requests for all available pages, while GitHub's endpoint documentation explicitly says “Responses include a maximum of 3000 files”; fail open when the PR's changed-file count reaches/exceeds that cap rather than treating pagination as complete.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Valid; fixed in 6b8898a. The classify step now only considers skipping when github.event.pull_request.changed_files is below 3000 (and treats a missing value as 3000), so a PR large enough to hit the files-endpoint cap always runs the full matrix.


🤖 Posted autonomously by Claude on behalf of pasta.

if [[ -n "$FILES" ]] && ! grep -qvE '\.md$' <<< "$FILES" && ! grep -qE '^src/' <<< "$FILES"; then
Comment thread
coderabbitai[bot] marked this conversation as resolved.
RUN=false
fi
fi
echo "Full build and test matrix: $RUN"
echo "run-build-tests=$RUN" >> "$GITHUB_OUTPUT"

- name: Checkout code
if: ${{ steps.skip-check.outputs.skip == 'false' }}
uses: actions/checkout@v6
Expand Down Expand Up @@ -117,7 +143,9 @@ jobs:
cache-sources:
name: Cache depends sources
needs: [check-skip]
if: ${{ needs.check-skip.outputs.skip == 'false' }}
if: |
needs.check-skip.outputs.skip == 'false' &&
needs.check-skip.outputs.run-build-tests == 'true'
uses: ./.github/workflows/cache-depends-sources.yml
with:
runs-on: ${{ needs.check-skip.outputs['runner-prelint'] }}
Expand All @@ -130,7 +158,9 @@ jobs:
packages: write
name: Build container
needs: [check-skip]
if: ${{ needs.check-skip.outputs.skip == 'false' }}
if: |
needs.check-skip.outputs.skip == 'false' &&
needs.check-skip.outputs.run-build-tests == 'true'
uses: ./.github/workflows/build-container.yml
with:
context: ./contrib/containers/ci
Expand Down
Loading