Split out clang-tidy to not run in merge (#4428)

Because clang-tidy is slow (and I'm not sure we can make it really
fast), trying to run it slightly less. Also, I noticed we can shave a
few minutes by disabling apt removal without losing too much free space.

Note that since this removes the old clang-tidy, I'll need to change the
branch protections before merging.

.github/actions/build-setup-ubuntu/action.yml

@@ -6,8 +6,8 @@ name: Setup build environment (Ubuntu)
 runs:
   using: composite
   steps:
-    # Ubuntu images start with 23GB available, and this adds 14GB more. For
-    # comparison, MacOS images have >100GB free.
+    # Ubuntu images start with ~23GB available; this takes a few seconds to add
+    # ~22GB more.
     #
     # Although we could delete more, if we run into a limit, not deleting
     # everything provides a little flexibility to get space while trying
@@ -18,6 +18,9 @@ runs:
         android: true
         dotnet: true
         haskell: true
+        # Enabling large-packages adds ~3 minutes to save ~4GB, so turn it off
+        # to save time.
+        large-packages: false
 
     # Cache and install a recent version of LLVM. This uses the GitHub action
     # cache to avoid directly downloading on each iteration and improve

.github/actions/test-setup/action.yml

@@ -0,0 +1,122 @@
+# Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+# Exceptions. See /LICENSE for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+name: Test setup
+
+inputs:
+  matrix_runner:
+    required: true
+  base_sha:
+    required: true
+  remote_cache_key:
+    required: true
+  targets_file:
+    required: true
+
+outputs:
+  has_code:
+    value: ${{ steps.filter.outputs.has_code}}
+
+runs:
+  using: composite
+  steps:
+    # Tests should only run on applicable paths, but we still need to have an
+    # action run for the merge queue. We filter steps based on the paths here,
+    # and condition steps on the output.
+    - id: filter
+      uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
+      with:
+        filters: |
+          has_code:
+            - '!{**/*.md,LICENSE,CODEOWNERS,.git*}'
+
+    # Disable uploads when the remote cache is read-only.
+    - name: Set up remote cache access (read-only)
+      if:
+        steps.filter.outputs.has_code == 'true' && github.event_name ==
+        'pull_request'
+      shell: bash
+      run: |
+        echo "remote_cache_upload=--remote_upload_local_results=false" \
+            >> $GITHUB_ENV
+
+    # Provide a cache key when the remote cache is read-write.
+    - name: Set up remote cache access (read-write)
+      if:
+        steps.filter.outputs.has_code == 'true' && github.event_name !=
+        'pull_request'
+      shell: bash
+      env:
+        REMOTE_CACHE_KEY: ${{ inputs.remote_cache_key }}
+      run: |
+        echo "$REMOTE_CACHE_KEY" | base64 -d > $HOME/remote_cache_key.json
+        echo "remote_cache_upload=--google_credentials=$HOME/remote_cache_key.json" \
+            >> $GITHUB_ENV
+
+    - uses: ./.github/actions/build-setup-common
+      if: steps.filter.outputs.has_code == 'true'
+      with:
+        matrix_runner: ${{ inputs.matrix_runner }}
+        remote_cache_upload: ${{ env.remote_cache_upload }}
+
+    # Just for visibility, print space before and after the build.
+    - name: Disk space before build
+      if: steps.filter.outputs.has_code == 'true'
+      shell: bash
+      run: df -h
+
+    - name: Verify MODULE.bazel.lock
+      if: steps.filter.outputs.has_code == 'true'
+      shell: bash
+      run: |
+        exit_code=0
+        ./scripts/run_bazel.py \
+          --attempts=5 \
+          mod deps --lockfile_mode=error || exit_code=$?
+        if (( $exit_code != 0 )); then
+          ./scripts/run_bazel.py \
+            --attempts=5 \
+            mod deps --lockfile_mode=update
+          echo "MODULE.bazel.lock is out of date! Use below file for update."
+          echo "Platforms may require merging output, for example by applying"
+          echo "an update, re-running triggers, and applying the next update."
+          echo "============================================================"
+          cat MODULE.bazel.lock
+          echo "============================================================"
+          exit 1
+        fi
+
+    # Build and run all targets on branch pushes to ensure we always have a
+    # clean tree. We don't expect this to be an interactive path and so don't
+    # optimize the latency of this step.
+    - name: Compute impacted pull request targets (for push)
+      if: steps.filter.outputs.has_code == 'true' && github.event_name == 'push'
+      shell: bash
+      env:
+        TARGETS_FILE: ${{ inputs.targets_file }}
+      run: |
+        echo "//..." >$TARGETS_FILE
+
+    # Compute the set of possible rules impacted by this change using
+    # Bazel-based diffing. This lets PRs and the merge queue have a much more
+    # efficient test CI action by avoiding even enumerating (and downloading)
+    # all of the unaffected Bazel targets.
+    - name: Compute impacted pull request targets
+      if: steps.filter.outputs.has_code == 'true' && github.event_name != 'push'
+      shell: bash
+      env:
+        # Compute the base SHA from the different event structures.
+        GIT_BASE_SHA: ${{ inputs.base_sha }}
+        TARGETS_FILE: ${{ inputs.targets_file }}
+      run: |
+        # First fetch the relevant base into the git repository.
+        git fetch --depth=1 origin $GIT_BASE_SHA
+
+        # Then use `target-determinator` as wrapped by our script.
+        ./scripts/target_determinator.py $GIT_BASE_SHA >$TARGETS_FILE
+
+        # Bazel requires a test target to run the test command. There may be
+        # no targets or there may only be non-test targets that we want to
+        # build, so simply inject an explicit no-op test target.
+        echo "//scripts:no_op_test" >> $TARGETS_FILE

.github/workflows/clang_tidy.yaml

@@ -0,0 +1,78 @@
+# Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+# Exceptions. See /LICENSE for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+name: 'Clang Tidy'
+
+on:
+  push:
+    branches: [trunk, action-test]
+  pull_request:
+
+permissions:
+  contents: read # For actions/checkout.
+  pull-requests: read # For dorny/paths-filter to read pull requests.
+
+# Cancel previous workflows on the PR when there are multiple fast commits.
+# https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency
+concurrency:
+  group: ${{ github.workflow }}-${{ github.head_ref || github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  test:
+    runs-on: ubuntu-22.04
+
+    steps:
+      - name: Harden Runner
+        uses: step-security/harden-runner@17d0e2bd7d51742c71671bd19fa12bdc9d40a3d6 # v2.8.1
+        with:
+          egress-policy: block
+          # When adding endpoints, see README.md.
+          # prettier-ignore
+          allowed-endpoints: >
+            *.dl.sourceforge.net:443
+            api.github.com:443
+            bcr.bazel.build:443
+            downloads.sourceforge.net:443
+            github.com:443
+            mirrors.kernel.org:443
+            nodejs.org:443
+            oauth2.googleapis.com:443
+            objects.githubusercontent.com:443
+            pypi.org:443
+            releases.bazel.build:443
+            sourceforge.net:443
+            storage.googleapis.com:443
+
+      - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
+
+      - id: test-setup
+        uses: ./.github/actions/test-setup
+        with:
+          matrix_runner: 'ubuntu-22.04'
+          base_sha:
+            ${{ github.event_name == 'pull_request' &&
+            github.event.pull_request.base.sha ||
+            github.event.merge_group.base_sha }}
+          remote_cache_key: ${{ secrets.CARBON_BUILDS_GITHUB }}
+          targets_file: ${{ runner.temp }}/targets
+
+      # Run in the clang-tidy config. This is done as part of tests so that we
+      # aren't duplicating bazel/llvm setup.
+      #
+      # The `-k` flag is used to print all clang-tidy errors.
+      - name: clang-tidy
+        if: steps.test-setup.outputs.has_code == 'true'
+        env:
+          TARGETS_FILE: ${{ runner.temp }}/targets
+        run: |
+          ./scripts/run_bazel.py \
+            --attempts=5 \
+            build --config=clang-tidy -k \
+            --target_pattern_file=$TARGETS_FILE
+
+      # See "Disk space before build" in `test-setup`.
+      - name: Disk space after build
+        if: steps.test-setup.outputs.has_code == 'true'
+        run: df -h

.github/workflows/tests.yaml

@@ -27,10 +27,6 @@ jobs:
         # Test a recent version of each supported OS.
         runner: ['ubuntu-22.04', 'macos-14']
         build_mode: [fastbuild, opt]
-        include:
-          # The clang-tidy config doesn't work on macos (missing `truncate`).
-          - runner: ubuntu-22.04
-            build_mode: clang-tidy
     runs-on: ${{ matrix.runner }}
 
     steps:
@@ -55,121 +51,23 @@ jobs:
             sourceforge.net:443
             storage.googleapis.com:443
 
-      # Checkout the pull request head or the branch.
-      - name: Checkout pull request
-        if: github.event_name == 'pull_request'
-        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
-        with:
-          ref: ${{ github.event.pull_request.head.sha }}
-
-      - name: Checkout branch
-        if: github.event_name != 'pull_request'
-        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
+      - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
 
-      # Tests should only run on applicable paths, but we still need to have an
-      # action run for the merge queue. We filter steps based on the paths here,
-      # and condition steps on the output.
-      - id: filter
-        uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
-        with:
-          filters: |
-            has_code:
-              - '!{**/*.md,LICENSE,CODEOWNERS,.git*}'
-
-      # Disable uploads when the remote cache is read-only.
-      - name: Set up remote cache access (read-only)
-        if:
-          steps.filter.outputs.has_code == 'true' && github.event_name ==
-          'pull_request'
-        run: |
-          echo "remote_cache_upload=--remote_upload_local_results=false" \
-              >> $GITHUB_ENV
-
-      # Provide a cache key when the remote cache is read-write.
-      - name: Set up remote cache access (read-write)
-        if:
-          steps.filter.outputs.has_code == 'true' && github.event_name !=
-          'pull_request'
-        env:
-          REMOTE_CACHE_KEY: ${{ secrets.CARBON_BUILDS_GITHUB }}
-        run: |
-          echo "$REMOTE_CACHE_KEY" | base64 -d > $HOME/remote_cache_key.json
-          echo "remote_cache_upload=--google_credentials=$HOME/remote_cache_key.json" \
-              >> $GITHUB_ENV
-
-      - uses: ./.github/actions/build-setup-common
-        if: steps.filter.outputs.has_code == 'true'
+      - id: test-setup
+        uses: ./.github/actions/test-setup
         with:
           matrix_runner: ${{ matrix.runner }}
-          remote_cache_upload: ${{ env.remote_cache_upload }}
-
-      # Just for visibility, print space before and after the build.
-      - name: Disk space before build
-        if: steps.filter.outputs.has_code == 'true'
-        run: df -h
-
-      - name: Verify MODULE.bazel.lock
-        if: steps.filter.outputs.has_code == 'true'
-        run: |
-          exit_code=0
-          ./scripts/run_bazel.py \
-            --attempts=5 \
-            mod deps --lockfile_mode=error || exit_code=$?
-          if (( $exit_code != 0 )); then
-            ./scripts/run_bazel.py \
-              --attempts=5 \
-              mod deps --lockfile_mode=update
-            echo "MODULE.bazel.lock is out of date! Use below file for update."
-            echo "Platforms may require merging output, for example by applying"
-            echo "an update, re-running triggers, and applying the next update."
-            echo "============================================================"
-            cat MODULE.bazel.lock
-            echo "============================================================"
-            exit 1
-          fi
-
-      # Build and run all targets on branch pushes to ensure we always have a
-      # clean tree. We don't expect this to be an interactive path and so don't
-      # optimize the latency of this step.
-      - name: Compute impacted pull request targets (for push)
-        if:
-          steps.filter.outputs.has_code == 'true' && github.event_name == 'push'
-        env:
-          TARGETS_FILE: ${{ runner.temp }}/targets
-        run: |
-          echo "//..." >$TARGETS_FILE
-
-      # Compute the set of possible rules impacted by this change using
-      # Bazel-based diffing. This lets PRs and the merge queue have a much more
-      # efficient test CI action by avoiding even enumerating (and downloading)
-      # all of the unaffected Bazel targets.
-      - name: Compute impacted pull request targets
-        if:
-          steps.filter.outputs.has_code == 'true' && github.event_name != 'push'
-        env:
-          # Compute the base SHA from the different event structures.
-          GIT_BASE_SHA:
+          base_sha:
             ${{ github.event_name == 'pull_request' &&
             github.event.pull_request.base.sha ||
             github.event.merge_group.base_sha }}
-          TARGETS_FILE: ${{ runner.temp }}/targets
-        run: |
-          # First fetch the relevant base into the git repository.
-          git fetch --depth=1 origin $GIT_BASE_SHA
-
-          # Then use `target-determinator` as wrapped by our script.
-          ./scripts/target_determinator.py $GIT_BASE_SHA >$TARGETS_FILE
-
-          # Bazel requires a test target to run the test command. There may be
-          # no targets or there may only be non-test targets that we want to
-          # build, so simply inject an explicit no-op test target.
-          echo "//scripts:no_op_test" >> $TARGETS_FILE
+          remote_cache_key: ${{ secrets.CARBON_BUILDS_GITHUB }}
+          targets_file: ${{ runner.temp }}/targets
 
       # Build and run just the tests impacted by the PR or merge group.
       - name: Test (${{ matrix.build_mode }})
-        if:
-          steps.filter.outputs.has_code == 'true' && matrix.build_mode !=
-          'clang-tidy'
+        if: steps.test-setup.outputs.has_code == 'true'
+        shell: bash
         env:
           # 'libtool_check_unique failed to generate' workaround.
           # https://github.com/bazelbuild/bazel/issues/14113#issuecomment-999794586
@@ -184,23 +82,7 @@ jobs:
             test -c ${{ matrix.build_mode }} \
             --target_pattern_file=$TARGETS_FILE
 
-      # Run in the clang-tidy config. This is done as part of tests so that we
-      # aren't duplicating bazel/llvm setup.
-      #
-      # The `-k` flag is used to print all clang-tidy errors.
-      - name: clang-tidy
-        if:
-          steps.filter.outputs.has_code == 'true' && matrix.build_mode ==
-          'clang-tidy'
-        env:
-          TARGETS_FILE: ${{ runner.temp }}/targets
-        run: |
-          ./scripts/run_bazel.py \
-            --attempts=5 \
-            build --config=clang-tidy -k \
-            --target_pattern_file=$TARGETS_FILE
-
-      # See "Disk space before build".
+      # See "Disk space before build" in `test-setup`.
       - name: Disk space after build
-        if: steps.filter.outputs.has_code == 'true'
+        if: steps.test-setup.outputs.has_code == 'true'
         run: df -h