From 2da90cee67e57b126cc58abe9017605cfae1e58d Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Thu, 10 Sep 2026 18:08:43 +0000 Subject: [PATCH 1/8] ci: share one Linux native build across CI workflows --- .github/workflows/README.md | 120 +++++------ .github/workflows/build_linux_native.yml | 82 ++++++++ .github/workflows/ci.yml | 51 ++++- .../workflows/iceberg_spark_test_reusable.yml | 76 ++----- .github/workflows/pr_build_linux.yml | 71 ++----- .github/workflows/spark_sql_test_reusable.yml | 71 ++----- dev/ci/check-ci-config.py | 198 +++++++++++++++--- dev/ci/compute-changes.py | 9 + dev/ci/test-ci-config.py | 149 +++++++++++++ dev/ci/test-native-build-selection.py | 176 ++++++++++++++++ 10 files changed, 723 insertions(+), 280 deletions(-) create mode 100644 .github/workflows/build_linux_native.yml create mode 100644 dev/ci/test-ci-config.py create mode 100644 dev/ci/test-native-build-selection.py diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 43b5be5a5c..15fff79ca2 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -35,61 +35,54 @@ run, a `Cargo.lock` or `pom.xml` change would leave the cargo-registry, Maven and TPC-H/TPC-DS caches on `main` stale until the next unrelated change. ``` - pull_request | merge_group | push to main | workflow_dispatch +pull_request | merge_group | push to main | workflow_dispatch + | + preflight + | + changes + | + +---------------+--------------------+ + | | + macOS / docs / build_linux_native + benchmark (if selected) (if any consumer is selected) | - v - +-----------------------+ - | preflight | ubuntu-slim - | (RAT, prettier, | - | missing-suites, | - | actionlint) | - +-----------+-----------+ - | on success - v - +-----------------------+ - | changes | ubuntu-slim - | (compute-changes.py: | - | one boolean per | - | heavy job) | - +-----------+-----------+ - | - +-----------------------------------+-----------------------------------+ - | | | - v v v - PR + queue tier push to main only queue tier, or PR with label - --------------- ----------------- --------------------------- - pr_build_linux (+ push, for cache) docs pr_build_macos run-macos-tests - spark_4_1 (catalyst + sql_core) pr_benchmark_check run-benchmark-check - iceberg_1_11 spark_4_1 sql_hive run-spark-4.1-hive-tests - spark_3_4 run-spark-3.4-tests - spark_3_5 run-spark-3.5-tests - spark_4_0 run-spark-4.0-tests - iceberg_1_8 run-iceberg-tests - iceberg_1_9 run-iceberg-tests - iceberg_1_10 run-iceberg-tests - - | | | - +-----------------------------------+-----------------------------------+ - v - +-----------------------+ - | required_checks | ubuntu-slim - | one flat name that | - | is safe to require | - +-----------------------+ - - reusable workflows invoked via `uses:`: - pr_build_linux.yml spark_sql_test_reusable.yml - pr_build_macos.yml iceberg_spark_test_reusable.yml - pr_benchmark_check.yml - docs.yaml + +----------------+----------------+ + | | | + pr_build_linux spark_3_* / iceberg_1_* + spark_4_* + +Every job above reports to required_checks (except the docs deployment). ``` +`build_linux_native.yml` builds the default Linux `libcomet.so` once per run +with JDK 17, the Cargo `ci` profile, and the existing x86-64-v3/bfd flags. +Every selected Linux, Spark SQL, and Iceberg caller waits for that producer +and receives `native-lib-linux` through its required `native-library-artifact` +input. Consumers keep their own Spark/JDK versions and download the library +into `native/target/release/`, where Maven expects it. Spark still pre-compiles +and shares its JVM test classes separately for each Spark/JDK version. + +The producer's condition is the union of those callers' existing path and +event/label conditions. A Spark-patch-only change therefore gets a native +build when its Spark caller is selected, even if the Linux build is not. +Documentation-only changes, benchmark-only changes, and unrelated label +events do not start an unused native build. The event-selection regression +test checks that the producer and its consumers stay in agreement. + +The Linux reusable workflow, including its lint and Rust debug-test jobs, +now starts after the shared native build. Rust formatting also runs in the +producer before compilation so formatting failures still stop that build +early. Rust debug tests, macOS, and feature-specific workflows continue to +build their own binaries. The shared producer is the only writer of the +Linux CI-profile Cargo cache, and only writes on `main`. + ## What runs when | Job in `ci.yml` | Triggered by | Routing rule | | -------------------- | ---------------------------------------------------------------------------------------------------------------------- | ----------------------------------- | | `preflight` | every PR / merge group / push / dispatch / label | none (always runs) | | `changes` | every PR / merge group / push / dispatch / label | runs `dev/ci/compute-changes.py` | +| `build_linux_native` | any selected Linux/Spark/Iceberg consumer | caller conditions in `ci.yml` | | `pr_build_linux` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | | `pr_build_macos` | merge group, **or** PR with `run-macos-tests` | `dev/ci/compute-changes.py` | | `pr_benchmark_check` | merge group, **or** PR with `run-benchmark-check` | benchmark sources only | @@ -163,6 +156,7 @@ umbrella doesn't watch, or operate independently of the rest of CI: | File | Called from `ci.yml` job(s) | | --------------------------------- | ------------------------------------------------------------ | +| `build_linux_native.yml` | `build_linux_native` | | `pr_build_linux.yml` | `pr_build_linux` | | `pr_build_macos.yml` | `pr_build_macos` | | `pr_benchmark_check.yml` | `pr_benchmark_check` | @@ -205,22 +199,24 @@ a routing table in `dev/ci/check-ci-config.py`, which `preflight` runs. ## Artifact names must be unique per producer Artifact names are scoped to the workflow **run**, not to the calling -workflow. `ci.yml` calls `spark_sql_test_reusable.yml` once per Spark -version and `iceberg_spark_test_reusable.yml` once per Iceberg version, all -inside the same run, so an unqualified name like `native-lib-linux` would be -claimed by several producers at once. That breaks two things: - -- `download-artifact` resolves a name to the highest matching artifact ID. - Nothing ties it to the producer the consumer declared in `needs`. -- `upload-artifact` with `overwrite: true` deletes the newest record with - that name before uploading, which can be a sibling's finished artifact. - The retry wrapper below forces `overwrite` on attempts 2 and 3. - -So every artifact published by a reusable workflow that `ci.yml` calls more -than once carries its version inputs, e.g. -`native-lib-spark-4.1.3-jdk17`. `dev/ci/check-ci-config.py` enforces this, -and also that every `download-artifact` name is produced by an upload in the -same workflow. +workflow. `build_linux_native.yml` is called exactly once and is the sole +producer of `native-lib-linux`. Its consumers declare a required +`native-library-artifact` input; `ci.yml` passes that name and makes every +consumer depend on the shared producer. They download the existing artifact +without publishing copies under version-specific names. + +Artifacts with multiple producers still carry their version inputs. For +example, `spark_sql_test_reusable.yml` publishes +`jvm-compiled-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }}` because +each Spark version has different compiled classes. Publishing two artifacts +under the same name can make a download select a sibling's artifact and let +an upload retry overwrite that sibling's output. + +`dev/ci/check-ci-config.py` verifies both contracts: local uploads and +downloads must match, and shared native-library consumers must be wired to +the one declared producer. Artifact retention remains one day; a failed-job +rerun can reuse a successful producer's artifact during that retention +window. If it has expired, rerun the full workflow to rebuild it. ## Retrying flaky network operations diff --git a/.github/workflows/build_linux_native.yml b/.github/workflows/build_linux_native.yml new file mode 100644 index 0000000000..05a5c821ee --- /dev/null +++ b/.github/workflows/build_linux_native.yml @@ -0,0 +1,82 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Single producer for the default Linux CI native library. The umbrella runs +# this whenever any Linux, Spark SQL, or Iceberg test consumer is selected. +name: Build Linux Native Library + +on: + workflow_call: + +env: + RUST_VERSION: stable + RUST_BACKTRACE: 1 + +jobs: + # Build native library once and share with all test jobs + build-native: + name: Build Native Library + runs-on: ubuntu-24.04 + container: + image: amd64/rust + steps: + - uses: actions/checkout@v7 + - name: Setup Rust toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: 17 # Matches the existing Linux producer, including libjvm linkage. + + - name: Check Rust formatting + run: cd native && cargo fmt --all -- --check + + - name: Restore Cargo cache + uses: actions/cache/restore@v6 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + native/target + key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} + restore-keys: | + ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- + + - name: Build native library (CI profile) + run: | + cd native + # CI profile: same overflow behavior as release, but faster compilation + # (no LTO, parallel codegen) + cargo build --profile ci + env: + RUSTFLAGS: "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" + + - name: Upload native library + uses: ./.github/actions/upload-artifact-retry + with: + name: native-lib-linux + path: native/target/ci/libcomet.so + retention-days: 1 + + - name: Save Cargo cache + uses: actions/cache/save@v6 + if: github.ref == 'refs/heads/main' + with: + path: | + ~/.cargo/registry + ~/.cargo/git + native/target + key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd3a0aa99c..5376e22ce6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -115,7 +115,10 @@ jobs: run: python3 dev/ci/test-iceberg-shards.py - name: Check CI config invariants - run: python3 dev/ci/check-ci-config.py + run: | + python3 dev/ci/check-ci-config.py + python3 dev/ci/test-ci-config.py + python3 dev/ci/test-native-build-selection.py - name: Install actionlint # Pure network, and preflight gates every other job, so a single reset @@ -225,11 +228,30 @@ jobs: # can actually test it. # --------------------------------------------------------------------------- + # Build once when any native-library consumer is selected. Each output + # already includes the path, event, and label policy from compute-changes.py. + build_linux_native: + name: Shared Linux Native Library + needs: changes + if: | + needs.changes.outputs.build_linux == 'true' || + needs.changes.outputs.spark_3_4 == 'true' || + needs.changes.outputs.spark_3_5 == 'true' || + needs.changes.outputs.spark_4_0 == 'true' || + needs.changes.outputs.spark_4_1 == 'true' || + needs.changes.outputs.iceberg_1_8 == 'true' || + needs.changes.outputs.iceberg_1_9 == 'true' || + needs.changes.outputs.iceberg_1_10 == 'true' || + needs.changes.outputs.iceberg_1_11 == 'true' + uses: ./.github/workflows/build_linux_native.yml + pr_build_linux: name: PR Build (Linux) - needs: changes + needs: [changes, build_linux_native] if: needs.changes.outputs.build_linux == 'true' uses: ./.github/workflows/pr_build_linux.yml + with: + native-library-artifact: native-lib-linux pr_build_macos: name: PR Build (macOS) @@ -254,42 +276,44 @@ jobs: spark_3_4: name: Spark SQL Tests (Spark 3.4) - needs: changes + needs: [changes, build_linux_native] # Queue-only by default; PRs need the `run-spark-3.4-tests` label. if: needs.changes.outputs.spark_3_4 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: + native-library-artifact: native-lib-linux spark-short: '3.4' spark-full: '3.4.3' java: 11 spark_3_5: name: Spark SQL Tests (Spark 3.5) - needs: changes - # Queue-only by default; PRs need the `run-spark-3.5-tests` label. + needs: [changes, build_linux_native] if: needs.changes.outputs.spark_3_5 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: + native-library-artifact: native-lib-linux spark-short: '3.5' spark-full: '3.5.9' java: 17 spark_4_0: name: Spark SQL Tests (Spark 4.0) - needs: changes + needs: [changes, build_linux_native] # Queue-only by default; PRs need the `run-spark-4.0-tests` label. Swapped # with spark_4_1 on the `oom` branch to validate the memory caps against # Spark 4.1 by default. if: needs.changes.outputs.spark_4_0 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: + native-library-artifact: native-lib-linux spark-short: '4.0' spark-full: '4.0.4' java: 17 spark_4_1: name: Spark SQL Tests (Spark 4.1) - needs: changes + needs: [changes, build_linux_native] # Two POLICY outputs feed one call, so the queue gets every module from a # single 40-minute build instead of two. `spark_4_1` (PR tier) covers # catalyst and the sql_core shards; `spark_4_1_hive` (queue-only, or the @@ -298,6 +322,7 @@ jobs: if: needs.changes.outputs.spark_4_1 == 'true' || needs.changes.outputs.spark_4_1_hive == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: + native-library-artifact: native-lib-linux spark-short: '4.1' spark-full: '4.1.3' java: 17 @@ -308,11 +333,12 @@ jobs: iceberg_1_8: name: Iceberg Spark SQL Tests (Iceberg 1.8) - needs: changes + needs: [changes, build_linux_native] # Queue-only by default; PRs need the `run-iceberg-tests` label. if: needs.changes.outputs.iceberg_1_8 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: + native-library-artifact: native-lib-linux iceberg-short: '1.8' iceberg-full: '1.8.1' spark-short: '3.4' @@ -321,11 +347,12 @@ jobs: iceberg_1_9: name: Iceberg Spark SQL Tests (Iceberg 1.9) - needs: changes + needs: [changes, build_linux_native] # Queue-only by default; PRs need the `run-iceberg-tests` label. if: needs.changes.outputs.iceberg_1_9 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: + native-library-artifact: native-lib-linux iceberg-short: '1.9' iceberg-full: '1.9.1' spark-short: '3.5' @@ -334,12 +361,13 @@ jobs: iceberg_1_10: name: Iceberg Spark SQL Tests (Iceberg 1.10) - needs: changes + needs: [changes, build_linux_native] # Queue-only by default; PRs need the `run-iceberg-tests` label. Iceberg 1.11 # (Spark 4.1) is the PR-gated Iceberg job; 1.10 covers the Spark 3.5 path. if: needs.changes.outputs.iceberg_1_10 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: + native-library-artifact: native-lib-linux iceberg-short: '1.10' iceberg-full: '1.10.0' spark-short: '3.5' @@ -348,11 +376,12 @@ jobs: iceberg_1_11: name: Iceberg Spark SQL Tests (Iceberg 1.11) - needs: changes + needs: [changes, build_linux_native] # Runs on every PR: Iceberg 1.11 is our only Spark 4.1 Iceberg coverage. if: needs.changes.outputs.iceberg_1_11 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: + native-library-artifact: native-lib-linux iceberg-short: '1.11' iceberg-full: '1.11.0' spark-short: '4.1' diff --git a/.github/workflows/iceberg_spark_test_reusable.yml b/.github/workflows/iceberg_spark_test_reusable.yml index ae20b6d768..8235d55fcf 100644 --- a/.github/workflows/iceberg_spark_test_reusable.yml +++ b/.github/workflows/iceberg_spark_test_reusable.yml @@ -24,6 +24,10 @@ name: Iceberg Spark SQL Tests (reusable) on: workflow_call: inputs: + native-library-artifact: + description: 'Default Linux CI library produced by the calling workflow' + required: true + type: string iceberg-short: description: 'Iceberg minor version, e.g. 1.10' required: true @@ -59,72 +63,24 @@ env: RUSTFLAGS: "-Clink-arg=-fuse-ld=bfd" jobs: - # Build native library once and share with all test jobs - build-native: - name: Build Native Library - runs-on: ubuntu-24.04 + # The calling workflow has already built the shared native library. + prepare-shards: + name: Define Iceberg test shards + runs-on: ubuntu-slim outputs: shard-matrix: ${{ steps.shards.outputs.matrix }} shard-count: ${{ steps.shards.outputs.count }} - container: - image: amd64/rust steps: - uses: actions/checkout@v7 - - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: 17 - - name: Define Iceberg test shards id: shards run: python3 dev/ci/check-iceberg-shards.py --github-output "$GITHUB_OUTPUT" - - name: Restore Cargo cache - uses: actions/cache/restore@v6 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - restore-keys: | - ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- - - - name: Build native library - # Use CI profile for faster builds (no LTO) and to share cache with pr_build_linux.yml. - run: | - cd native && cargo build --profile ci - env: - RUSTFLAGS: "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" - - - name: Save Cargo cache - uses: actions/cache/save@v6 - if: github.ref == 'refs/heads/main' - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - - - name: Upload native library - uses: ./.github/actions/upload-artifact-retry - with: - # Version-qualified: ci.yml calls this workflow once per Iceberg - # version inside a single run, and artifact names are scoped to the - # run, not to the calling workflow. See "Artifact names must be - # unique per producer" in .github/workflows/README.md. - name: native-lib-iceberg-${{ inputs.iceberg-full }}-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} - path: native/target/ci/libcomet.so - retention-days: 1 - iceberg-spark: - needs: build-native + needs: prepare-shards strategy: fail-fast: false - matrix: ${{ fromJSON(needs.build-native.outputs.shard-matrix) }} + matrix: ${{ fromJSON(needs.prepare-shards.outputs.shard-matrix) }} name: iceberg-spark/iceberg-${{ inputs.iceberg-full }}/spark-${{ inputs.spark-full }}/scala-${{ inputs.scala }}/java-${{ inputs.java }}/shard-${{ matrix.shard }} runs-on: ubuntu-24.04 container: @@ -141,7 +97,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-iceberg-${{ inputs.iceberg-full }}-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Build Comet run: | @@ -158,7 +114,7 @@ jobs: :iceberg-spark:iceberg-spark-${{ inputs.spark-short }}_${{ inputs.scala }}:test \ --init-script ../dev/ci/iceberg-test-shards.gradle \ -PcometShardTask=:iceberg-spark:iceberg-spark-${{ inputs.spark-short }}_${{ inputs.scala }}:test \ - -PcometShardIndex=${{ matrix.shard }} -PcometShardCount=${{ needs.build-native.outputs.shard-count }} \ + -PcometShardIndex=${{ matrix.shard }} -PcometShardCount=${{ needs.prepare-shards.outputs.shard-count }} \ -Pquick=true -x javadoc - name: Upload Iceberg shard inventory and test reports if: ${{ !cancelled() }} @@ -192,7 +148,7 @@ jobs: --task :iceberg-spark:iceberg-spark-${{ inputs.spark-short }}_${{ inputs.scala }}:test iceberg-spark-extensions: - needs: build-native + needs: prepare-shards name: iceberg-spark-extensions/iceberg-${{ inputs.iceberg-full }}/spark-${{ inputs.spark-full }}/scala-${{ inputs.scala }}/java-${{ inputs.java }} runs-on: ubuntu-24.04 container: @@ -209,7 +165,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-iceberg-${{ inputs.iceberg-full }}-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Build Comet run: | @@ -227,7 +183,7 @@ jobs: -Pquick=true -x javadoc iceberg-spark-runtime: - needs: build-native + needs: prepare-shards name: iceberg-spark-runtime/iceberg-${{ inputs.iceberg-full }}/spark-${{ inputs.spark-full }}/scala-${{ inputs.scala }}/java-${{ inputs.java }} runs-on: ubuntu-24.04 container: @@ -244,7 +200,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-iceberg-${{ inputs.iceberg-full }}-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Build Comet run: | diff --git a/.github/workflows/pr_build_linux.yml b/.github/workflows/pr_build_linux.yml index 4cd30e66cd..18a3b25c89 100644 --- a/.github/workflows/pr_build_linux.yml +++ b/.github/workflows/pr_build_linux.yml @@ -21,6 +21,11 @@ name: PR Build (Linux) # live in the umbrella workflow. on: workflow_call: + inputs: + native-library-artifact: + description: 'Default Linux CI library produced by the calling workflow' + required: true + type: string env: RUST_VERSION: stable @@ -247,59 +252,7 @@ jobs: -Dsuites=org.apache.comet.shuffle.CelebornReflectionCompatibilitySuite \ -DfailIfNoTests=false - # Build native library once and share with all test jobs - build-native: - needs: lint - name: Build Native Library - runs-on: ubuntu-24.04 - container: - image: amd64/rust - steps: - - uses: actions/checkout@v7 - - name: Setup Rust toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: 17 # JDK only needed for JVM module proto generation - - - name: Restore Cargo cache - uses: actions/cache/restore@v6 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - restore-keys: | - ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- - - - name: Build native library (CI profile) - run: | - cd native - # CI profile: same overflow behavior as release, but faster compilation - # (no LTO, parallel codegen) - cargo build --profile ci - env: - RUSTFLAGS: "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" - - - name: Upload native library - uses: ./.github/actions/upload-artifact-retry - with: - name: native-lib-linux - path: native/target/ci/libcomet.so - retention-days: 1 - - - name: Save Cargo cache - uses: actions/cache/save@v6 - if: github.ref == 'refs/heads/main' - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - - # Run Rust tests (runs in parallel with build-native, uses debug builds) + # Rust tests use a separate debug build, not the shared CI library. linux-test-rust: needs: lint name: ubuntu-latest/rust-test @@ -341,7 +294,7 @@ jobs: key: ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} linux-test: - needs: build-native + needs: lint strategy: matrix: # the goal with these profiles is to get coverage of all Java, Scala, and Spark @@ -527,7 +480,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-linux + name: ${{ inputs.native-library-artifact }} # Download to release/ since Maven's -Prelease expects libcomet.so there path: native/target/release/ @@ -553,7 +506,7 @@ jobs: # TPC-H correctness test - verifies benchmark queries produce correct results verify-benchmark-results-tpch: - needs: build-native + needs: lint name: Verify TPC-H Results runs-on: ubuntu-24.04 container: @@ -572,7 +525,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-linux + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Cache Maven dependencies @@ -613,7 +566,7 @@ jobs: # TPC-DS correctness tests - verifies benchmark queries produce correct results. # The three join strategies run sequentially in one job so the project is built once. verify-benchmark-results-tpcds: - needs: build-native + needs: lint name: Verify TPC-DS Results runs-on: ubuntu-24.04 container: @@ -632,7 +585,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-linux + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Cache Maven dependencies diff --git a/.github/workflows/spark_sql_test_reusable.yml b/.github/workflows/spark_sql_test_reusable.yml index 9fa254f9b6..aa2c860e2f 100644 --- a/.github/workflows/spark_sql_test_reusable.yml +++ b/.github/workflows/spark_sql_test_reusable.yml @@ -24,6 +24,10 @@ name: Spark SQL Tests (reusable) on: workflow_call: inputs: + native-library-artifact: + description: 'Default Linux CI library produced by the calling workflow' + required: true + type: string spark-short: description: 'Spark minor version, e.g. 3.5' required: true @@ -63,22 +67,11 @@ env: jobs: - # Build the native library AND pre-compile Spark sources + Test classes in a - # single runner, then publish two artifacts the matrix consumes: - # - native-lib-spark--jdk: libcomet.so (~50 MB) - # - jvm-compiled-spark--jdk: apache-spark.tar.gz (sources + - # target/ + Zinc state, ~500 MB-1 GB) - # Combining them avoids a second runner cold-start and an extra inter-job - # artifact round-trip for the native lib, since the JVM build already - # depends on it (the Comet Maven install bundles libcomet.so into the - # Comet JAR before SBT resolves Spark's classpath). - # - # Both names carry the Spark/JDK version because ci.yml calls this workflow - # once per Spark version inside a single run, and artifact names are scoped - # to the run, not to the calling workflow. See "Artifact names must be unique - # per producer" in .github/workflows/README.md. + # Pre-compile Spark sources and test classes once per Spark/JDK version. + # The native library comes from the umbrella's shared Linux producer; only + # the version-specific JVM artifact is published by this workflow. build: - name: Build Native + JVM Test Classes + name: Build JVM Test Classes runs-on: ubuntu-24.04 container: image: amd64/rust @@ -97,49 +90,11 @@ jobs: rust-version: ${{ env.RUST_VERSION }} jdk-version: ${{ inputs.java }} - - name: Restore Cargo cache - uses: actions/cache/restore@v6 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - restore-keys: | - ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- - - - name: Build native library (CI profile) - run: | - cd native - cargo build --profile ci - env: - RUSTFLAGS: "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" - - - name: Save Cargo cache - uses: actions/cache/save@v6 - if: github.ref == 'refs/heads/main' - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - - - name: Upload native library - uses: ./.github/actions/upload-artifact-retry + - name: Download native library + uses: actions/download-artifact@v8 with: - name: native-lib-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} - path: native/target/ci/libcomet.so - retention-days: 1 - - - name: Stage native library at release path - run: | - # setup-spark-builder's `mvnw install -DskipTests` (skip-native-build - # path) bundles native/target/release/libcomet.so into the Comet JAR. - # We built with --profile ci to avoid LTO, so the file lives at - # native/target/ci/. Copy it to where the Maven build expects it. - mkdir -p native/target/release - cp native/target/ci/libcomet.so native/target/release/libcomet.so + name: ${{ inputs.native-library-artifact }} + path: native/target/release/ - name: Setup Spark uses: ./.github/actions/setup-spark-builder @@ -210,7 +165,7 @@ jobs: - name: Download native library uses: ./.github/actions/download-artifact-retry with: - name: native-lib-spark-${{ inputs.spark-full }}-jdk${{ inputs.java }} + name: ${{ inputs.native-library-artifact }} path: native/target/release/ - name: Download JVM compile artifact uses: ./.github/actions/download-artifact-retry diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 6ccd22979a..53687b7f23 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -22,11 +22,8 @@ # that job skip, so the edit merges with only preflight having looked at # it. The table below pins the routing for the shared build inputs. # -# 2. Event policy. The same script decides which events may run each job. -# That used to be a `${{ }}` expression on every job in ci.yml, where it -# could not be tested; POLICY_CASES below is the test it never had. The -# expected sets are transcribed from the `if:` expressions ci.yml carried -# before the policy moved, so a regression here is a behaviour change. +# 2. Event policy. The routing script also decides which events may run +# each job; POLICY_CASES pins the behavior of the former workflow gates. # # 3. Required-check coverage. `Required Checks` in ci.yml is the job that # `.asf.yaml` can name in `required_status_checks` for main. A heavy job @@ -38,8 +35,9 @@ # GitHub keeps the most recent check run per name per commit, so a label # run publishing the required name would overwrite the real verdict. # -# 4. Artifact-name uniqueness. Artifact names are scoped to the *run*, not -# to the calling workflow, and ci.yml calls the Spark SQL and Iceberg +# 4. Artifact-name uniqueness and explicit shared-producer wiring. Names +# are scoped to the *run*, not the calling workflow. ci.yml calls the +# Spark SQL and Iceberg # reusable workflows several times in one run. Two producers sharing a # name make `download-artifact` pick by highest artifact ID rather than # by `needs`, and make the forced `overwrite` on an upload retry delete @@ -106,6 +104,8 @@ ([".github/actions/download-artifact-retry/action.yaml"], BUILD_JOBS), # The Maven bootstrap composite is called only from pr_build_linux.yml. ([".github/actions/maven-bootstrap/action.yaml"], {"build_linux"}), + # Editing the shared Linux producer must exercise every Linux consumer. + ([".github/workflows/build_linux_native.yml"], BUILD_JOBS - {"build_macos"}), # Spot checks that the additions above did not widen unrelated routes. (["docs/source/user-guide/overview.md"], {"docs"}), (["native/core/benches/parquet_read.rs"], {"benchmark"}), @@ -226,9 +226,7 @@ # `uses:` values that publish an artifact, and the one that consumes it. UPLOAD_USES = re.compile(r"uses:\s*(\./\.github/actions/upload-artifact-retry|actions/upload-artifact@)") DOWNLOAD_USES = re.compile(r"uses:\s*(\./\.github/actions/download-artifact-retry|actions/download-artifact@)") -# The artifact name is the first `name:` key of the step's `with:` block. A -# following step starts with `- `, which distinguishes it from a `with:` key. -WITH_NAME = re.compile(r"^\s+name:\s*(\S.*?)\s*$") +# A following step starts with `- `, unlike the current step's `with:` keys. NEW_STEP = re.compile(r"^\s*-\s") # A job id in a workflow file, and the two `uses:` shapes the checkout guard @@ -238,6 +236,16 @@ JOB_KEY = re.compile(r"^ ([A-Za-z0-9_-]+):\s*$") LOCAL_ACTION_USES = re.compile(r"uses:\s*(\./\.github/actions/\S+)") CHECKOUT_USES = re.compile(r"uses:\s*actions/checkout@") +SHARED_NATIVE_WORKFLOW = "build_linux_native.yml" +SHARED_NATIVE_JOB = "build_linux_native" +SHARED_NATIVE_INPUT = "native-library-artifact" +SHARED_NATIVE_ARTIFACT = "native-lib-linux" +SHARED_NATIVE_EXPRESSION = "${{ inputs.native-library-artifact }}" +SHARED_NATIVE_CONSUMERS = { + "pr_build_linux.yml", + "spark_sql_test_reusable.yml", + "iceberg_spark_test_reusable.yml", +} def load_filters(): @@ -330,50 +338,180 @@ def check_event_policy(): return not failures -def artifact_names(path): - """Return ([upload names], [download names]) for one workflow file.""" - uploads, downloads = [], [] +def artifact_steps(path): + """Return artifact steps and their direct `with:` inputs.""" + artifacts = [] lines = path.read_text(encoding="utf-8").splitlines() for index, line in enumerate(lines): if UPLOAD_USES.search(line): - bucket = uploads + kind = "upload" elif DOWNLOAD_USES.search(line): - bucket = downloads + kind = "download" else: continue + indent = len(line) - len(line.lstrip()) + body = [] for following in lines[index + 1:]: - if NEW_STEP.match(following): - break # step ended without a `name:`; download-all, or the default - match = WITH_NAME.match(following) - if match: - bucket.append(match.group(1)) - break - return uploads, downloads + if following.strip() and not following.lstrip().startswith("#"): + if NEW_STEP.match(following) or len(following) - len(following.lstrip()) < indent: + break + body.append(following) + text = "\n".join(body) + with_key = re.search(r"^( +)with:\s*$", text, re.MULTILINE) + if with_key: + with_indent = len(with_key.group(1)) + inputs = block_mapping(block_mapping(text, with_indent)["with"][1], with_indent + 2) + artifacts.append((kind, {key: scalar(value) for key, (value, _) in inputs.items()})) + return artifacts -def check_artifact_names(): - ci = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8") +def artifact_names(path): + """Return ([upload names], [download names]) for one workflow file.""" + steps = artifact_steps(path) + return tuple([inputs["name"] for kind, inputs in steps if kind == expected and "name" in inputs] + for expected in ("upload", "download")) + + +def block_mapping(text, indent): + """Read block-style keys at the workflow files' conventional indentation. + + This only inspects the small mapping subset needed by the guards below; + actionlint remains responsible for validating GitHub Actions YAML syntax. + Values are (inline value, indented body), so scalar inputs and nested + workflow/job mappings can be checked without a third-party YAML dependency. + """ + pattern = re.compile(r"^" + " " * indent + r"([\w-]+):[^\S\n]*(.*)$", re.MULTILINE) + matches = list(pattern.finditer(text)) + return { + match.group(1): (match.group(2).strip(), + text[match.end():matches[index + 1].start() + if index + 1 < len(matches) else len(text)]) + for index, match in enumerate(matches) + } + + +def scalar(value): + return value.strip().strip("\"'") + + +def dependencies(job): + value, body = block_mapping(job, 4).get("needs", ("", "")) + if value.startswith("[") and value.endswith("]"): + return {scalar(item) for item in value[1:-1].split(",")} + if value: + return {scalar(value)} + return {scalar(item) for item in re.findall(r"^\s+- (.+)$", body, re.MULTILINE)} + + +def shared_native_failures(workflows, jobs, artifacts): + """Validate the sole allowed cross-workflow artifact producer/consumer edge.""" + failures = [] + producer_uses = f"./.github/workflows/{SHARED_NATIVE_WORKFLOW}" + calls = [job_id for job_id, (_, body) in jobs.items() + if scalar(block_mapping(body, 4).get("uses", ("", ""))[0]) == producer_uses] + if calls != [SHARED_NATIVE_JOB]: + failures.append(f"ci.yml: expected exactly one {SHARED_NATIVE_JOB} call to {producer_uses}") + + if SHARED_NATIVE_JOB in jobs: + body = jobs[SHARED_NATIVE_JOB][1] + if "changes" not in dependencies(body): + failures.append(f"ci.yml: {SHARED_NATIVE_JOB} must need changes") + if "strategy" in block_mapping(body, 4): + failures.append(f"ci.yml: {SHARED_NATIVE_JOB} must not use a matrix") + for filename, (uploads, _) in artifacts.items(): + if filename != SHARED_NATIVE_WORKFLOW and SHARED_NATIVE_ARTIFACT in uploads: + failures.append(f"{filename}: only {SHARED_NATIVE_WORKFLOW} may upload '{SHARED_NATIVE_ARTIFACT}'") + + producer = workflows / SHARED_NATIVE_WORKFLOW + if not producer.exists(): + failures.append(f"{producer}: shared native producer is missing") + else: + producer_jobs = block_mapping( + block_mapping(producer.read_text(encoding="utf-8"), 0).get("jobs", ("", ""))[1], 2) + uploads = artifacts[SHARED_NATIVE_WORKFLOW][0] + if len(producer_jobs) != 1 or uploads != [SHARED_NATIVE_ARTIFACT]: + failures.append(f"{producer}: expected one job uploading '{SHARED_NATIVE_ARTIFACT}' once") + for _, body in producer_jobs.values(): + if "strategy" in block_mapping(body, 4): + failures.append(f"{producer}: shared native producer must not use a matrix") + + seen_consumers = set() + for job_id, (_, body) in jobs.items(): + fields = block_mapping(body, 4) + called = scalar(fields.get("uses", ("", ""))[0]).removeprefix("./.github/workflows/") + if called not in SHARED_NATIVE_CONSUMERS: + continue + seen_consumers.add(called) + if not {"changes", SHARED_NATIVE_JOB}.issubset(dependencies(body)): + failures.append(f"ci.yml: {job_id} must need changes and {SHARED_NATIVE_JOB}") + inputs = block_mapping(fields.get("with", ("", ""))[1], 6) + if scalar(inputs.get(SHARED_NATIVE_INPUT, ("", ""))[0]) != SHARED_NATIVE_ARTIFACT: + failures.append(f"ci.yml: {job_id} must pass {SHARED_NATIVE_INPUT}: {SHARED_NATIVE_ARTIFACT}") + + for filename in sorted(SHARED_NATIVE_CONSUMERS): + path = workflows / filename + if filename not in seen_consumers: + failures.append(f"ci.yml: shared native consumer {filename} is not called") + if not path.exists(): + failures.append(f"{path}: shared native consumer is missing") + continue + text = path.read_text(encoding="utf-8") + declaration = text + for key, indent in (("on", 0), ("workflow_call", 2), ("inputs", 4), + (SHARED_NATIVE_INPUT, 6)): + declaration = block_mapping(declaration, indent).get(key, ("", ""))[1] + fields = block_mapping(declaration, 8) + if (scalar(fields.get("required", ("", ""))[0]) != "true" + or scalar(fields.get("type", ("", ""))[0]) != "string"): + failures.append(f"{path}: {SHARED_NATIVE_INPUT} must be a required string input") + uploads, downloads = artifacts[filename] + if SHARED_NATIVE_EXPRESSION not in downloads: + failures.append(f"{path}: must download {SHARED_NATIVE_EXPRESSION}") + if any(name.startswith("native-lib") or name == SHARED_NATIVE_EXPRESSION for name in uploads): + failures.append(f"{path}: native library must only be uploaded by {SHARED_NATIVE_WORKFLOW}") + native_destinations = [inputs.get("name") for kind, inputs in artifact_steps(path) + if kind == "download" and inputs.get("path", "").startswith("native/target")] + if (any(name.startswith("native-lib") for name in downloads) + or any(name != SHARED_NATIVE_EXPRESSION for name in native_destinations)): + failures.append(f"{path}: native downloads must use {SHARED_NATIVE_EXPRESSION}") + if re.search(r"^\s*(?:cargo build\b|make (?:release|core)\b)", text, re.MULTILINE): + failures.append(f"{path}: must consume the shared native library instead of building it") + return failures + + +def artifact_failures(workflows): + ci = (workflows / "ci.yml").read_text(encoding="utf-8") + jobs = block_mapping(block_mapping(ci, 0).get("jobs", ("", ""))[1], 2) call_counts = {} for called in re.findall(r"uses:\s*\./\.github/workflows/(\S+)", ci): call_counts[called] = call_counts.get(called, 0) + 1 - failures = [] - for path in sorted(WORKFLOWS.glob("*.y*ml")): - uploads, downloads = artifact_names(path) - if call_counts.get(path.name, 0) > 1: + artifacts = {path.name: artifact_names(path) for path in sorted(workflows.glob("*.y*ml"))} + failures = shared_native_failures(workflows, jobs, artifacts) + shared_wiring_valid = not failures + for filename, (uploads, downloads) in artifacts.items(): + path = workflows / filename + if call_counts.get(filename, 0) > 1: for name in uploads: if "inputs." not in name: failures.append( f"{path}: artifact '{name}' is uploaded by a workflow ci.yml calls " - f"{call_counts[path.name]} times; qualify the name with an input " + f"{call_counts[filename]} times; qualify the name with an input " f"(e.g. ${{{{ inputs.spark-full }}}}) so the parallel producers stay distinct" ) for name in downloads: - if name not in uploads: + explicitly_shared = (shared_wiring_valid and filename in SHARED_NATIVE_CONSUMERS + and name == SHARED_NATIVE_EXPRESSION) + if name not in uploads and not explicitly_shared: failures.append( f"{path}: artifact '{name}' is downloaded but never uploaded in the same " f"workflow; a producer rename probably missed its consumer" ) + return failures + + +def check_artifact_names(): + failures = artifact_failures(WORKFLOWS) for failure in failures: print(f"artifact name: {failure}") return not failures diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 3380a1e443..cc1ecdd67d 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -40,6 +40,7 @@ FILTERS = { "build_linux": [ + ".github/workflows/build_linux_native.yml", "native/**", "common/**", "spark/**", @@ -108,6 +109,7 @@ "spark/src/main/spark-*/**", ], "spark_3_4": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -135,6 +137,7 @@ "mvnw", ], "spark_3_5": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -162,6 +165,7 @@ "mvnw", ], "spark_4_0": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -189,6 +193,7 @@ "mvnw", ], "spark_4_1": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -221,6 +226,7 @@ # input. Populated below, after the dict, so the two lists cannot drift. "spark_4_1_hive": [], "iceberg_1_8": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -245,6 +251,7 @@ "mvnw", ], "iceberg_1_9": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -269,6 +276,7 @@ "mvnw", ], "iceberg_1_10": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", @@ -293,6 +301,7 @@ "mvnw", ], "iceberg_1_11": [ + ".github/workflows/build_linux_native.yml", "native/**/src/**", "native/**/Cargo.toml", "native/Cargo.lock", diff --git a/dev/ci/test-ci-config.py b/dev/ci/test-ci-config.py new file mode 100644 index 0000000000..c89d499a66 --- /dev/null +++ b/dev/ci/test-ci-config.py @@ -0,0 +1,149 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Regression tests for shared artifacts using mutations of the real workflows.""" + +import importlib.util +from pathlib import Path +import shutil +import tempfile +import unittest + + +SPEC = importlib.util.spec_from_file_location( + "check_ci_config", Path(__file__).with_name("check-ci-config.py")) +CHECK = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CHECK) +WORKFLOWS = Path(__file__).resolve().parents[2] / ".github/workflows" + + +class SharedNativeArtifactTest(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="comet-ci-config-test-") + self.addCleanup(self.temp.cleanup) + self.workflows = Path(self.temp.name) / "workflows" + shutil.copytree(WORKFLOWS, self.workflows) + + def replace(self, filename, old, new): + path = self.workflows / filename + text = path.read_text(encoding="utf-8") + self.assertIn(old, text, f"Fixture changed: {filename}") + path.write_text(text.replace(old, new, 1), encoding="utf-8") + + def assert_rejected(self, expected): + failures = CHECK.artifact_failures(self.workflows) + self.assertTrue(any(expected in failure for failure in failures), failures) + + def test_real_workflows_have_valid_shared_artifacts(self): + self.assertEqual(CHECK.artifact_failures(self.workflows), []) + + def test_missing_shared_producer_is_rejected(self): + (self.workflows / CHECK.SHARED_NATIVE_WORKFLOW).unlink() + self.assert_rejected("shared native producer is missing") + + def test_second_producer_call_is_rejected(self): + path = self.workflows / "ci.yml" + with path.open("a", encoding="utf-8") as stream: + stream.write("\n duplicate_native:\n needs: changes\n" + " uses: ./.github/workflows/build_linux_native.yml\n") + self.assert_rejected("expected exactly one") + + def test_another_workflow_cannot_publish_the_shared_name(self): + shutil.copyfile(self.workflows / CHECK.SHARED_NATIVE_WORKFLOW, + self.workflows / "duplicate_native.yml") + self.assert_rejected("only build_linux_native.yml may upload") + + def test_producer_matrix_is_rejected(self): + self.replace(CHECK.SHARED_NATIVE_WORKFLOW, " runs-on:", + " strategy:\n matrix:\n duplicate: [1, 2]\n runs-on:") + self.assert_rejected("shared native producer must not use a matrix") + + def test_producer_artifact_rename_is_rejected(self): + self.replace(CHECK.SHARED_NATIVE_WORKFLOW, "name: native-lib-linux", + "name: native-lib-renamed") + self.assert_rejected("uploading 'native-lib-linux' once") + + def test_missing_dependency_in_each_consumer_call_is_rejected(self): + path = self.workflows / "ci.yml" + original = path.read_text(encoding="utf-8") + job_ids = {"pr_build_linux"} | (CHECK.BUILD_JOBS - {"build_linux", "build_macos"}) + for job_id in sorted(job_ids): + with self.subTest(job=job_id): + start = original.index(f"\n {job_id}:\n") + before, body = original[:start], original[start:] + self.assertIn("needs: [changes, build_linux_native]", body) + body = body.replace("needs: [changes, build_linux_native]", "needs: changes", 1) + path.write_text(before + body, encoding="utf-8") + self.assert_rejected(f"{job_id} must need changes and build_linux_native") + path.write_text(original, encoding="utf-8") + + def test_wrong_caller_artifact_is_rejected(self): + self.replace("ci.yml", "native-library-artifact: native-lib-linux", + "native-library-artifact: native-lib-wrong") + self.assert_rejected("must pass native-library-artifact: native-lib-linux") + + def test_missing_caller_artifact_is_rejected(self): + self.replace("ci.yml", " native-library-artifact: native-lib-linux\n", "") + self.assert_rejected("must pass native-library-artifact: native-lib-linux") + + def test_consumer_input_must_be_required(self): + self.replace("pr_build_linux.yml", "required: true", "required: false") + self.assert_rejected("native-library-artifact must be a required string input") + + def test_consumer_input_must_be_string(self): + self.replace("pr_build_linux.yml", "type: string", "type: boolean") + self.assert_rejected("native-library-artifact must be a required string input") + + def test_literal_native_download_is_rejected(self): + self.replace("pr_build_linux.yml", "name: ${{ inputs.native-library-artifact }}", + "name: native-lib-linux") + self.assert_rejected("native downloads must use") + + def test_jvm_artifact_cannot_replace_a_native_download(self): + path = self.workflows / "spark_sql_test_reusable.yml" + uploads, _ = CHECK.artifact_names(path) + jvm_name = next(name for name in uploads if name.startswith("jvm-compiled-spark-")) + self.replace(path.name, "name: ${{ inputs.native-library-artifact }}", f"name: {jvm_name}") + self.assert_rejected("native downloads must use") + + def test_unrelated_input_does_not_bypass_producer_check(self): + self.replace("pr_build_linux.yml", "name: ${{ inputs.native-library-artifact }}", + "name: ${{ inputs.unrelated-artifact }}") + self.assert_rejected("artifact '${{ inputs.unrelated-artifact }}' is downloaded but never uploaded") + + def test_native_build_in_consumer_is_rejected(self): + path = self.workflows / "iceberg_spark_test_reusable.yml" + with path.open("a", encoding="utf-8") as stream: + stream.write("\n - run: |\n cargo build --profile ci\n") + self.assert_rejected("consume the shared native library instead of building it") + + def test_jvm_artifact_rename_still_fails(self): + self.replace("spark_sql_test_reusable.yml", "name: jvm-compiled-spark-", + "name: renamed-jvm-compiled-spark-") + self.assert_rejected("is downloaded but never uploaded in the same workflow") + + def test_unqualified_jvm_upload_still_fails(self): + path = self.workflows / "spark_sql_test_reusable.yml" + text = path.read_text(encoding="utf-8") + uploads, _ = CHECK.artifact_names(path) + name = next(name for name in uploads if name.startswith("jvm-compiled-spark-")) + path.write_text(text.replace(f"name: {name}", "name: jvm-compiled-spark"), encoding="utf-8") + self.assert_rejected("qualify the name with an input") + + +if __name__ == "__main__": + unittest.main() diff --git a/dev/ci/test-native-build-selection.py b/dev/ci/test-native-build-selection.py new file mode 100644 index 0000000000..e1ed1970af --- /dev/null +++ b/dev/ci/test-native-build-selection.py @@ -0,0 +1,176 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Exercise the native producer and consumer conditions from the real workflow. + +Only the small expression subset used by these conditions is translated. This +keeps the tests dependency-free like check-ci-config.py; actionlint separately +checks GitHub's expression syntax and workflow dependency graph. +""" + +import importlib.util +import itertools +import re +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +CONSUMERS = { + "pr_build_linux": "build_linux", + "spark_3_4": "spark_3_4", + "spark_3_5": "spark_3_5", + "spark_4_0": "spark_4_0", + "spark_4_1": "spark_4_1", + "iceberg_1_8": "iceberg_1_8", + "iceberg_1_9": "iceberg_1_9", + "iceberg_1_10": "iceberg_1_10", + "iceberg_1_11": "iceberg_1_11", +} +DEFAULT = {"pr_build_linux", "spark_3_5", "spark_4_1", "iceberg_1_11"} +OPT_IN = ("run-spark-3.4-tests", "run-spark-4.0-tests", "run-iceberg-tests") + + +def conditions(): + workflow = (ROOT / ".github/workflows/ci.yml").read_text() + expressions = {} + for job in ["build_linux_native", *CONSUMERS]: + block = re.search( + r"^ " + job + r":\n.*?(?=^ [a-z][a-z_0-9]*:\n|\Z)", + workflow, + re.M | re.S, + ).group() + expression = block.split(" if: |\n", 1)[1].split(" uses:", 1)[0] + expression = " ".join(expression.split()) + expression = re.sub( + r"needs\.changes\.outputs\.([a-z_0-9]+)", + lambda match: f"changes[{match.group(1)!r}]", + expression, + ) + for before, after in ( + ("github.event.pull_request.labels.*.name", "labels"), + ("github.event.label.name", "label"), + ("github.event.action", "action"), + ("github.event_name", "event"), + ): + expression = expression.replace(before, after) + expression = expression.replace("&&", " and ").replace("||", " or ") + expressions[job] = compile(expression, str(ROOT / ".github/workflows/ci.yml"), "eval") + return expressions + + +class NativeBuildSelectionTest(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.expressions = conditions() + spec = importlib.util.spec_from_file_location( + "compute_changes", ROOT / "dev/ci/compute-changes.py" + ) + cls.filters = importlib.util.module_from_spec(spec) + spec.loader.exec_module(cls.filters) + + def evaluate(self, flags, event="pull_request", action="synchronize", labels=(), label=""): + context = { + "changes": {key: str(value).lower() for key, value in flags.items()}, + "event": event, + "action": action, + "labels": labels, + "label": label, + "contains": lambda values, value: value in values, + } + return { + name: eval(expression, {"__builtins__": {}}, context) + for name, expression in self.expressions.items() + } + + def assert_selection(self, files, expected, **event): + flags = { + key: self.filters.matches(patterns, files) + for key, patterns in self.filters.FILTERS.items() + } + # ci.yml forces every output true for a manual workflow_dispatch. + if event.get("event") == "workflow_dispatch": + flags = dict.fromkeys(flags, True) + selected = self.evaluate(flags, **event) + self.assertEqual({name for name in CONSUMERS if selected[name]}, expected) + self.assertEqual(selected["build_linux_native"], bool(expected)) + + def test_native_change_uses_default_pr_coverage(self): + self.assert_selection(["native/core/src/lib.rs"], DEFAULT) + + def test_docs_and_benchmarks_do_not_build_native(self): + for path in ("docs/source/user-guide/overview.md", "native/core/benches/parquet_read.rs"): + with self.subTest(path=path): + self.assert_selection([path], set()) + + def test_spark_patch_does_not_require_linux_test_workflow(self): + self.assert_selection(["dev/diffs/3.5.9.diff"], {"spark_3_5"}) + + def test_legacy_patch_needs_opt_in(self): + self.assert_selection(["dev/diffs/3.4.3.diff"], set()) + self.assert_selection( + ["dev/diffs/3.4.3.diff"], {"spark_3_4"}, labels=("run-spark-3.4-tests",) + ) + + def test_unrelated_label_does_not_duplicate_existing_runs(self): + self.assert_selection( + ["native/core/src/lib.rs"], set(), action="labeled", + labels=(*OPT_IN, "dependencies"), label="dependencies", + ) + + def test_new_spark_label_runs_only_selected_version(self): + self.assert_selection( + ["native/core/src/lib.rs"], {"spark_3_4"}, action="labeled", + labels=OPT_IN, label="run-spark-3.4-tests", + ) + + def test_new_iceberg_label_runs_only_opt_in_versions(self): + self.assert_selection( + ["native/core/src/lib.rs"], {"iceberg_1_8", "iceberg_1_9", "iceberg_1_10"}, + action="labeled", labels=OPT_IN, label="run-iceberg-tests", + ) + + def test_main_and_manual_runs_include_legacy_consumers(self): + self.assert_selection(["native/core/src/lib.rs"], set(CONSUMERS), event="push") + self.assert_selection([], set(CONSUMERS), event="workflow_dispatch") + + def test_producer_change_exercises_all_default_linux_consumers(self): + self.assert_selection([".github/workflows/build_linux_native.yml"], DEFAULT) + + def test_producer_condition_matches_all_consumer_combinations(self): + keys = list(CONSUMERS.values()) + label_sets = [ + tuple(label for label, selected in zip(OPT_IN, mask) if selected) + for mask in itertools.product((False, True), repeat=len(OPT_IN)) + ] + events = [{"event": "push"}, {"event": "workflow_dispatch"}] + for labels in label_sets: + events.append({"labels": labels}) + for label in (*labels, "dependencies"): + events.append({"action": "labeled", "labels": labels, "label": label}) + for mask in itertools.product((False, True), repeat=len(keys)): + flags = dict(zip(keys, mask)) + for event in events: + selected = self.evaluate(flags, **event) + self.assertEqual( + selected["build_linux_native"], + any(selected[name] for name in CONSUMERS), + (flags, event), + ) + + +if __name__ == "__main__": + unittest.main() From ba5f9fb43a57692b224e731c65e491719ed74b47 Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Thu, 10 Sep 2026 18:18:36 +0000 Subject: [PATCH 2/8] ci: run independent Linux checks alongside native build --- .github/workflows/README.md | 30 +- .github/workflows/ci.yml | 9 + .github/workflows/pr_build_linux.yml | 260 +----------------- .github/workflows/pr_build_linux_checks.yml | 290 ++++++++++++++++++++ dev/ci/check-ci-config.py | 65 ++++- dev/ci/compute-changes.py | 1 + dev/ci/test-ci-config.py | 62 +++++ dev/ci/test-native-build-selection.py | 14 +- 8 files changed, 458 insertions(+), 273 deletions(-) create mode 100644 .github/workflows/pr_build_linux_checks.yml diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 15fff79ca2..c9890fdcad 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -43,8 +43,8 @@ pull_request | merge_group | push to main | workflow_dispatch | +---------------+--------------------+ | | - macOS / docs / build_linux_native - benchmark (if selected) (if any consumer is selected) + Linux checks / macOS / build_linux_native + docs / benchmark (if selected) (if any consumer is selected) | +----------------+----------------+ | | | @@ -56,25 +56,31 @@ Every job above reports to required_checks (except the docs deployment). `build_linux_native.yml` builds the default Linux `libcomet.so` once per run with JDK 17, the Cargo `ci` profile, and the existing x86-64-v3/bfd flags. -Every selected Linux, Spark SQL, and Iceberg caller waits for that producer +Every selected Linux test, Spark SQL, and Iceberg caller waits for that producer and receives `native-lib-linux` through its required `native-library-artifact` input. Consumers keep their own Spark/JDK versions and download the library into `native/target/release/`, where Maven expects it. Spark still pre-compiles and shares its JVM test classes separately for each Spark/JDK version. -The producer's condition is the union of those callers' existing path and -event/label conditions. A Spark-patch-only change therefore gets a native +The producer's condition is the union of those callers' `changes` outputs, +which already include the path and event/label policy in `compute-changes.py`. A Spark-patch-only change therefore gets a native build when its Spark caller is selected, even if the Linux build is not. Documentation-only changes, benchmark-only changes, and unrelated label events do not start an unused native build. The event-selection regression test checks that the producer and its consumers stay in agreement. -The Linux reusable workflow, including its lint and Rust debug-test jobs, -now starts after the shared native build. Rust formatting also runs in the -producer before compilation so formatting failures still stop that build -early. Rust debug tests, macOS, and feature-specific workflows continue to -build their own binaries. The shared producer is the only writer of the -Linux CI-profile Cargo cache, and only writes on `main`. +Linux lint, compile-only checks, Celeborn compatibility tests, and Rust debug +tests run in `pr_build_linux_checks.yml` as soon as change selection completes. +They run alongside the native producer and still report results if it fails. +Only the JVM/TPC test consumers in `pr_build_linux.yml` wait for the shared +artifact. Both Linux callers use the same path and event selection. Regression +checks preserve this separation and prevent independent checks from acquiring +a native-build dependency. + +Rust formatting runs before native compilation and before the independent +Linux build/test jobs. Rust debug tests, macOS, and feature-specific workflows +continue to build their own binaries. The shared producer is the only writer +of the Linux CI-profile Cargo cache, and only writes on `main`. ## What runs when @@ -83,6 +89,7 @@ Linux CI-profile Cargo cache, and only writes on `main`. | `preflight` | every PR / merge group / push / dispatch / label | none (always runs) | | `changes` | every PR / merge group / push / dispatch / label | runs `dev/ci/compute-changes.py` | | `build_linux_native` | any selected Linux/Spark/Iceberg consumer | caller conditions in `ci.yml` | +| `pr_build_linux_checks` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | | `pr_build_linux` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | | `pr_build_macos` | merge group, **or** PR with `run-macos-tests` | `dev/ci/compute-changes.py` | | `pr_benchmark_check` | merge group, **or** PR with `run-benchmark-check` | benchmark sources only | @@ -157,6 +164,7 @@ umbrella doesn't watch, or operate independently of the rest of CI: | File | Called from `ci.yml` job(s) | | --------------------------------- | ------------------------------------------------------------ | | `build_linux_native.yml` | `build_linux_native` | +| `pr_build_linux_checks.yml` | `pr_build_linux_checks` | | `pr_build_linux.yml` | `pr_build_linux` | | `pr_build_macos.yml` | `pr_build_macos` | | `pr_benchmark_check.yml` | `pr_benchmark_check` | diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5376e22ce6..9501aa4255 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -228,6 +228,13 @@ jobs: # can actually test it. # --------------------------------------------------------------------------- + # Independent checks start immediately after change selection. + pr_build_linux_checks: + name: PR Checks (Linux) + needs: changes + if: needs.changes.outputs.build_linux == 'true' + uses: ./.github/workflows/pr_build_linux_checks.yml + # Build once when any native-library consumer is selected. Each output # already includes the path, event, and label policy from compute-changes.py. build_linux_native: @@ -427,6 +434,8 @@ jobs: needs: - preflight - changes + - pr_build_linux_checks + - build_linux_native - pr_build_linux - pr_build_macos - pr_benchmark_check diff --git a/.github/workflows/pr_build_linux.yml b/.github/workflows/pr_build_linux.yml index 18a3b25c89..5945fbbd5d 100644 --- a/.github/workflows/pr_build_linux.yml +++ b/.github/workflows/pr_build_linux.yml @@ -37,264 +37,8 @@ env: jobs: - # Fast lint check - gates all other jobs - lint: - name: Lint - runs-on: ubuntu-24.04 - container: - image: amd64/rust - steps: - - uses: actions/checkout@v7 - - - name: Check Rust formatting - run: | - rustup component add rustfmt - cd native && cargo fmt --all -- --check - - # Fast syntactic-only scalafix check. Parses sources without compiling, so it - # surfaces version-independent style issues (e.g. RedundantSyntax) in seconds, - # long before the lint-java matrix finishes its ~3.5 min build. It also scans - # the spark-4.1 / spark-4.2 sources that lint-java skips (those profiles can't - # run -Psemanticdb yet), so it is the only gate covering them. The full rule - # set, including the semantic rules, still runs in lint-java. - scalafix-syntactic: - name: Lint Scala (syntactic) - runs-on: ubuntu-24.04 - steps: - - uses: actions/checkout@v7 - - - name: Setup coursier - uses: coursier/setup-action@v3 - with: - jvm: temurin:21 - - # Split the network from the lint. `cs launch` resolves and downloads the - # scalafix artifacts from Maven Central, which resets connections now and - # then, so the warm-up retries a no-op `--version` run. The real check then - # runs `--mode offline` against the populated cache: no network, and a - # nonzero exit can only mean a lint violation, never a download failure. - - name: Fetch scalafix - run: | - for attempt in 1 2 3; do - if cs launch scalafix:0.14.6 -- --version; then - break - fi - if [ "$attempt" = 3 ]; then - echo "::error::scalafix download failed after 3 attempts." - exit 1 - fi - echo "::warning::scalafix download failed (attempt $attempt of 3); retrying in $((attempt * 15))s." - sleep $((attempt * 15)) - done - - - name: Run syntactic scalafix check (no compile) - run: | - cs launch --mode offline scalafix:0.14.6 -- \ - --check \ - --syntactic \ - --config .scalafix-syntactic.conf \ - --exclude '**/target/**' \ - spark - - lint-java: - needs: lint - name: Lint Java (${{ matrix.profile.name }}) - runs-on: ubuntu-24.04 - container: - image: amd64/rust - env: - JAVA_TOOL_OPTIONS: ${{ (matrix.profile.java_version == '17' || matrix.profile.java_version == '21') && '--add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-exports=java.base/sun.util.calendar=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED' || '' }} - strategy: - matrix: - profile: - - name: "Spark 3.4, JDK 11, Scala 2.12" - java_version: "11" - maven_opts: "-Pspark-3.4 -Pscala-2.12" - - name: "Spark 3.5, JDK 17, Scala 2.12" - java_version: "17" - maven_opts: "-Pspark-3.5 -Pscala-2.12" - - name: "Spark 4.0, JDK 17" - java_version: "17" - maven_opts: "-Pspark-4.0" - - name: "Spark 4.0, JDK 21" - java_version: "21" - maven_opts: "-Pspark-4.0" - # Spark 4.1 and 4.2 are intentionally absent: the lint job invokes -Psemanticdb, - # but semanticdb-scalac for those Scala patch versions (2.13.17 / 2.13.18) is not - # yet published, so we cannot currently run scalafix against the spark-4.1 or - # spark-4.2 profiles. - fail-fast: false - steps: - - uses: actions/checkout@v7 - - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: ${{ matrix.profile.java_version }} - - - name: Cache Maven dependencies - uses: actions/cache@v6 - with: - path: | - ~/.m2/repository - /root/.m2/repository - key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-lint - restore-keys: | - ${{ runner.os }}-java-maven- - - - name: Bootstrap Maven - uses: ./.github/actions/maven-bootstrap - - - name: Run scalafix check - run: | - ./mvnw -B package -DskipTests scalafix:scalafix -Dscalafix.mode=CHECK -Psemanticdb ${{ matrix.profile.maven_opts }} - - - name: Setup Node.js - uses: actions/setup-node@v7 - with: - node-version: '24' - - - name: Install prettier - run: | - npm install -g prettier - - - name: Run prettier - run: | - npx prettier "**/*.md" --write - - - name: Mark workspace as safe for git - run: | - git config --global --add safe.directory "$GITHUB_WORKSPACE" - - - name: Check for any local git changes (such as generated docs) - run: | - ./dev/ci/check-working-tree-clean.sh - - # Compile-only verification for Spark 4.1. Tests are intentionally skipped: the spark-4.1 - # profile is currently a build target only, and several runtime/test failures are tracked - # in follow-up PRs. Excluded from lint-java because semanticdb-scalac_2.13.17 is not yet - # published and the lint job activates -Psemanticdb. - build-spark-4-1: - needs: lint - name: Build Spark 4.1, JDK 17 - runs-on: ubuntu-24.04 - container: - image: amd64/rust - steps: - - uses: actions/checkout@v7 - - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: 17 - - - name: Cache Maven dependencies - uses: actions/cache@v6 - with: - path: | - ~/.m2/repository - /root/.m2/repository - key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-spark-4.1-build - restore-keys: | - ${{ runner.os }}-java-maven- - - - name: Bootstrap Maven - uses: ./.github/actions/maven-bootstrap - - - name: Compile (skip tests) - run: ./mvnw -B install -DskipTests -Dmaven.test.skip=true -Pspark-4.1 - - celeborn-reflection-compatibility: - needs: lint - name: Celeborn ${{ matrix.celeborn_version }} reflection compatibility - runs-on: ubuntu-24.04 - container: - image: amd64/rust - env: - JAVA_TOOL_OPTIONS: --add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-exports=java.base/sun.util.calendar=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED - strategy: - fail-fast: false - matrix: - celeborn_version: ["0.6.0", "0.7.0"] - steps: - - uses: actions/checkout@v7 - - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: 17 - - - name: Cache Maven dependencies - uses: actions/cache@v6 - with: - path: | - ~/.m2/repository - /root/.m2/repository - key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-celeborn-${{ matrix.celeborn_version }} - restore-keys: | - ${{ runner.os }}-java-maven- - - - name: Bootstrap Maven - uses: ./.github/actions/maven-bootstrap - - - name: Verify reflected Celeborn internals - env: - SPARK_LOCAL_HOSTNAME: localhost - SPARK_LOCAL_IP: 127.0.0.1 - run: | - SPARK_HOME="$GITHUB_WORKSPACE" ./mvnw -B clean test \ - -Pspark-3.5,scala-2.12,celeborn-reflection-compatibility \ - -Dceleborn.version="${{ matrix.celeborn_version }}" \ - -Dtest=none \ - -Dsuites=org.apache.comet.shuffle.CelebornReflectionCompatibilitySuite \ - -DfailIfNoTests=false - - # Rust tests use a separate debug build, not the shared CI library. - linux-test-rust: - needs: lint - name: ubuntu-latest/rust-test - runs-on: ubuntu-24.04 - container: - image: amd64/rust - steps: - - uses: actions/checkout@v7 - - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder - with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: 17 - - - name: Restore Cargo cache - uses: actions/cache/restore@v6 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - # Note: Java version intentionally excluded - Rust target is JDK-independent - key: ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - restore-keys: | - ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- - - - name: Rust test steps - uses: ./.github/actions/rust-test - - - name: Save Cargo cache - uses: actions/cache/save@v6 - if: github.ref == 'refs/heads/main' - with: - path: | - ~/.cargo/registry - ~/.cargo/git - native/target - key: ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} - + # Native artifact readiness and Rust formatting are checked by the shared producer. linux-test: - needs: lint strategy: matrix: # the goal with these profiles is to get coverage of all Java, Scala, and Spark @@ -506,7 +250,6 @@ jobs: # TPC-H correctness test - verifies benchmark queries produce correct results verify-benchmark-results-tpch: - needs: lint name: Verify TPC-H Results runs-on: ubuntu-24.04 container: @@ -566,7 +309,6 @@ jobs: # TPC-DS correctness tests - verifies benchmark queries produce correct results. # The three join strategies run sequentially in one job so the project is built once. verify-benchmark-results-tpcds: - needs: lint name: Verify TPC-DS Results runs-on: ubuntu-24.04 container: diff --git a/.github/workflows/pr_build_linux_checks.yml b/.github/workflows/pr_build_linux_checks.yml new file mode 100644 index 0000000000..c66e6667ed --- /dev/null +++ b/.github/workflows/pr_build_linux_checks.yml @@ -0,0 +1,290 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: PR Checks (Linux) + +# Independent lint, compilation, compatibility, and Rust debug checks. +# Run alongside the shared native producer; these jobs do not consume its artifact. +# Triggering and path filters live in the umbrella workflow. +on: + workflow_call: + +env: + RUST_VERSION: stable + RUST_BACKTRACE: 1 + # Force GNU ld on Linux: recent Rust stable defaults to rust-lld on + # x86_64-unknown-linux-gnu, and rust-lld cannot resolve -ljvm against the + # Zulu JDK layout installed by setup-java. Keep bfd for all cargo invocations. + RUSTFLAGS: "-Clink-arg=-fuse-ld=bfd" + +jobs: + + # Rust formatting gates the build and test jobs below + lint: + name: Lint + runs-on: ubuntu-24.04 + container: + image: amd64/rust + steps: + - uses: actions/checkout@v7 + + - name: Check Rust formatting + run: | + rustup component add rustfmt + cd native && cargo fmt --all -- --check + + # Fast syntactic-only scalafix check. Parses sources without compiling, so it + # surfaces version-independent style issues (e.g. RedundantSyntax) in seconds, + # long before the lint-java matrix finishes its ~3.5 min build. It also scans + # the spark-4.1 / spark-4.2 sources that lint-java skips (those profiles can't + # run -Psemanticdb yet), so it is the only gate covering them. The full rule + # set, including the semantic rules, still runs in lint-java. + scalafix-syntactic: + name: Lint Scala (syntactic) + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v7 + + - name: Setup coursier + uses: coursier/setup-action@v3 + with: + jvm: temurin:21 + + # Split the network from the lint. `cs launch` resolves and downloads the + # scalafix artifacts from Maven Central, which resets connections now and + # then, so the warm-up retries a no-op `--version` run. The real check then + # runs `--mode offline` against the populated cache: no network, and a + # nonzero exit can only mean a lint violation, never a download failure. + - name: Fetch scalafix + run: | + for attempt in 1 2 3; do + if cs launch scalafix:0.14.6 -- --version; then + break + fi + if [ "$attempt" = 3 ]; then + echo "::error::scalafix download failed after 3 attempts." + exit 1 + fi + echo "::warning::scalafix download failed (attempt $attempt of 3); retrying in $((attempt * 15))s." + sleep $((attempt * 15)) + done + + - name: Run syntactic scalafix check (no compile) + run: | + cs launch --mode offline scalafix:0.14.6 -- \ + --check \ + --syntactic \ + --config .scalafix-syntactic.conf \ + --exclude '**/target/**' \ + spark + + lint-java: + needs: lint + name: Lint Java (${{ matrix.profile.name }}) + runs-on: ubuntu-24.04 + container: + image: amd64/rust + env: + JAVA_TOOL_OPTIONS: ${{ (matrix.profile.java_version == '17' || matrix.profile.java_version == '21') && '--add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-exports=java.base/sun.util.calendar=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED' || '' }} + strategy: + matrix: + profile: + - name: "Spark 3.4, JDK 11, Scala 2.12" + java_version: "11" + maven_opts: "-Pspark-3.4 -Pscala-2.12" + - name: "Spark 3.5, JDK 17, Scala 2.12" + java_version: "17" + maven_opts: "-Pspark-3.5 -Pscala-2.12" + - name: "Spark 4.0, JDK 17" + java_version: "17" + maven_opts: "-Pspark-4.0" + - name: "Spark 4.0, JDK 21" + java_version: "21" + maven_opts: "-Pspark-4.0" + # Spark 4.1 and 4.2 are intentionally absent: the lint job invokes -Psemanticdb, + # but semanticdb-scalac for those Scala patch versions (2.13.17 / 2.13.18) is not + # yet published, so we cannot currently run scalafix against the spark-4.1 or + # spark-4.2 profiles. + fail-fast: false + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: ${{ matrix.profile.java_version }} + + - name: Cache Maven dependencies + uses: actions/cache@v6 + with: + path: | + ~/.m2/repository + /root/.m2/repository + key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-lint + restore-keys: | + ${{ runner.os }}-java-maven- + + - name: Bootstrap Maven + uses: ./.github/actions/maven-bootstrap + + - name: Run scalafix check + run: | + ./mvnw -B package -DskipTests scalafix:scalafix -Dscalafix.mode=CHECK -Psemanticdb ${{ matrix.profile.maven_opts }} + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '24' + + - name: Install prettier + run: | + npm install -g prettier + + - name: Run prettier + run: | + npx prettier "**/*.md" --write + + - name: Mark workspace as safe for git + run: | + git config --global --add safe.directory "$GITHUB_WORKSPACE" + + - name: Check for any local git changes (such as generated docs) + run: | + ./dev/ci/check-working-tree-clean.sh + + # Compile-only verification for Spark 4.1. Tests are intentionally skipped: the spark-4.1 + # profile is currently a build target only, and several runtime/test failures are tracked + # in follow-up PRs. Excluded from lint-java because semanticdb-scalac_2.13.17 is not yet + # published and the lint job activates -Psemanticdb. + build-spark-4-1: + needs: lint + name: Build Spark 4.1, JDK 17 + runs-on: ubuntu-24.04 + container: + image: amd64/rust + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: 17 + + - name: Cache Maven dependencies + uses: actions/cache@v6 + with: + path: | + ~/.m2/repository + /root/.m2/repository + key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-spark-4.1-build + restore-keys: | + ${{ runner.os }}-java-maven- + + - name: Bootstrap Maven + uses: ./.github/actions/maven-bootstrap + + - name: Compile (skip tests) + run: ./mvnw -B install -DskipTests -Dmaven.test.skip=true -Pspark-4.1 + + celeborn-reflection-compatibility: + needs: lint + name: Celeborn ${{ matrix.celeborn_version }} reflection compatibility + runs-on: ubuntu-24.04 + container: + image: amd64/rust + env: + JAVA_TOOL_OPTIONS: --add-exports=java.base/sun.nio.ch=ALL-UNNAMED --add-exports=java.base/sun.util.calendar=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED + strategy: + fail-fast: false + matrix: + celeborn_version: ["0.6.0", "0.7.0"] + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: 17 + + - name: Cache Maven dependencies + uses: actions/cache@v6 + with: + path: | + ~/.m2/repository + /root/.m2/repository + key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-celeborn-${{ matrix.celeborn_version }} + restore-keys: | + ${{ runner.os }}-java-maven- + + - name: Bootstrap Maven + uses: ./.github/actions/maven-bootstrap + + - name: Verify reflected Celeborn internals + env: + SPARK_LOCAL_HOSTNAME: localhost + SPARK_LOCAL_IP: 127.0.0.1 + run: | + SPARK_HOME="$GITHUB_WORKSPACE" ./mvnw -B clean test \ + -Pspark-3.5,scala-2.12,celeborn-reflection-compatibility \ + -Dceleborn.version="${{ matrix.celeborn_version }}" \ + -Dtest=none \ + -Dsuites=org.apache.comet.shuffle.CelebornReflectionCompatibilitySuite \ + -DfailIfNoTests=false + + # Rust tests use a separate debug build, not the shared CI library. + linux-test-rust: + needs: lint + name: ubuntu-latest/rust-test + runs-on: ubuntu-24.04 + container: + image: amd64/rust + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: 17 + + - name: Restore Cargo cache + uses: actions/cache/restore@v6 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + native/target + # Note: Java version intentionally excluded - Rust target is JDK-independent + key: ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} + restore-keys: | + ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- + + - name: Rust test steps + uses: ./.github/actions/rust-test + + - name: Save Cargo cache + uses: actions/cache/save@v6 + if: github.ref == 'refs/heads/main' + with: + path: | + ~/.cargo/registry + ~/.cargo/git + native/target + key: ${{ runner.os }}-cargo-debug-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 53687b7f23..5ec3c7d0fe 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -# Guards five CI invariants that are silent when broken: +# Guards CI invariants that are silent when broken: # # 1. Change-filter routing. dev/ci/compute-changes.py decides which heavy # jobs run. A file that a job depends on but that no filter lists makes @@ -48,6 +48,9 @@ # run only under an input or a label can carry that for a long time # before anyone runs them. # +# 6. Independent Linux checks. Lint, compile-only checks and debug Rust +# tests must remain runnable without waiting for the native CI build. +# # Run from the repository root: python3 dev/ci/check-ci-config.py import importlib.util @@ -106,6 +109,8 @@ ([".github/actions/maven-bootstrap/action.yaml"], {"build_linux"}), # Editing the shared Linux producer must exercise every Linux consumer. ([".github/workflows/build_linux_native.yml"], BUILD_JOBS - {"build_macos"}), + # The independent lint/compile/Rust workflow belongs only to Linux CI. + ([".github/workflows/pr_build_linux_checks.yml"], {"build_linux"}), # Spot checks that the additions above did not widen unrelated routes. (["docs/source/user-guide/overview.md"], {"docs"}), (["native/core/benches/parquet_read.rs"], {"benchmark"}), @@ -248,6 +253,14 @@ } +LINUX_CHECKS_WORKFLOW = "pr_build_linux_checks.yml" +LINUX_CHECKS_JOB = "pr_build_linux_checks" +INDEPENDENT_LINUX_JOBS = { + "lint", "scalafix-syntactic", "lint-java", "build-spark-4-1", + "celeborn-reflection-compatibility", "linux-test-rust", +} + + def load_filters(): spec = importlib.util.spec_from_file_location("compute_changes", "dev/ci/compute-changes.py") module = importlib.util.module_from_spec(spec) @@ -479,6 +492,55 @@ def shared_native_failures(workflows, jobs, artifacts): return failures +def linux_checks_failures(workflows, jobs): + """Keep lint, compile-only and debug Rust checks independent of native CI.""" + failures = [] + body = jobs.get(LINUX_CHECKS_JOB, ("", ""))[1] + fields = block_mapping(body, 4) + expected_uses = f"./.github/workflows/{LINUX_CHECKS_WORKFLOW}" + if scalar(fields.get("uses", ("", ""))[0]) != expected_uses: + failures.append(f"ci.yml: {LINUX_CHECKS_JOB} must call {expected_uses}") + if dependencies(body) != {"changes"}: + failures.append(f"ci.yml: {LINUX_CHECKS_JOB} must need only changes, independently of native CI") + + def condition(job): + value, body = block_mapping(job, 4).get("if", ("", "")) + text = body if value in {"|", ">"} else value + return " ".join(line.strip() for line in text.splitlines() + if line.strip() and not line.lstrip().startswith("#")) + + if condition(body) != condition(jobs.get("pr_build_linux", ("", ""))[1]): + failures.append(f"ci.yml: {LINUX_CHECKS_JOB} must use the Linux test selection condition") + + checks = workflows / LINUX_CHECKS_WORKFLOW + if not checks.exists(): + failures.append(f"{checks}: independent Linux checks workflow is missing") + return failures + text = checks.read_text(encoding="utf-8") + check_jobs = block_mapping(block_mapping(text, 0).get("jobs", ("", ""))[1], 2) + missing = INDEPENDENT_LINUX_JOBS - check_jobs.keys() + if missing: + failures.append(f"{checks}: independent jobs are missing: {', '.join(sorted(missing))}") + events = block_mapping(text, 0).get("on", ("", ""))[1] + workflow_call = block_mapping(events, 2).get("workflow_call", ("", ""))[1] + inputs = block_mapping(block_mapping(workflow_call, 4).get("inputs", ("", ""))[1], 6) + if SHARED_NATIVE_INPUT in inputs or any( + kind == "download" and (inputs.get("name", "").startswith("native-lib") + or inputs.get("path", "").startswith("native/target")) + for kind, inputs in artifact_steps(checks)): + failures.append(f"{checks}: independent Linux checks must not consume the shared native artifact") + + consumers = workflows / "pr_build_linux.yml" + if consumers.exists(): + consumer_jobs = block_mapping( + block_mapping(consumers.read_text(encoding="utf-8"), 0).get("jobs", ("", ""))[1], 2) + misplaced = INDEPENDENT_LINUX_JOBS & consumer_jobs.keys() + if misplaced: + failures.append(f"{consumers}: independent jobs must stay in {LINUX_CHECKS_WORKFLOW}: " + f"{', '.join(sorted(misplaced))}") + return failures + + def artifact_failures(workflows): ci = (workflows / "ci.yml").read_text(encoding="utf-8") jobs = block_mapping(block_mapping(ci, 0).get("jobs", ("", ""))[1], 2) @@ -488,6 +550,7 @@ def artifact_failures(workflows): artifacts = {path.name: artifact_names(path) for path in sorted(workflows.glob("*.y*ml"))} failures = shared_native_failures(workflows, jobs, artifacts) + failures.extend(linux_checks_failures(workflows, jobs)) shared_wiring_valid = not failures for filename, (uploads, downloads) in artifacts.items(): path = workflows / filename diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index cc1ecdd67d..e874479652 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -54,6 +54,7 @@ "dev/ci/**", ".github/workflows/ci.yml", ".github/workflows/pr_build_linux.yml", + ".github/workflows/pr_build_linux_checks.yml", ".github/actions/setup-builder/**", ".github/actions/java-test/**", ".github/actions/maven-bootstrap/**", diff --git a/dev/ci/test-ci-config.py b/dev/ci/test-ci-config.py index c89d499a66..9a52b861e3 100644 --- a/dev/ci/test-ci-config.py +++ b/dev/ci/test-ci-config.py @@ -145,5 +145,67 @@ def test_unqualified_jvm_upload_still_fails(self): self.assert_rejected("qualify the name with an input") + def test_independent_checks_cannot_wait_for_native(self): + path = self.workflows / "ci.yml" + text = path.read_text(encoding="utf-8") + start = text.index("\n pr_build_linux_checks:\n") + before, body = text[:start], text[start:] + self.assertIn("needs: changes", body) + path.write_text(before + body.replace("needs: changes", + "needs: [changes, build_linux_native]", 1), encoding="utf-8") + self.assert_rejected("pr_build_linux_checks must need only changes") + + def test_missing_independent_checks_call_is_rejected(self): + self.replace("ci.yml", "uses: ./.github/workflows/pr_build_linux_checks.yml", + "uses: ./.github/workflows/pr_build_linux.yml") + self.assert_rejected("pr_build_linux_checks must call") + + def test_missing_independent_checks_workflow_is_rejected(self): + (self.workflows / CHECK.LINUX_CHECKS_WORKFLOW).unlink() + self.assert_rejected("independent Linux checks workflow is missing") + + def test_independent_checks_keep_linux_selection(self): + path = self.workflows / "ci.yml" + text = path.read_text(encoding="utf-8") + start = text.index("\n pr_build_linux_checks:\n") + before, body = text[:start], text[start:] + body = body.replace("needs.changes.outputs.build_linux == 'true'", + "needs.changes.outputs.build_linux == 'false'", 1) + path.write_text(before + body, encoding="utf-8") + self.assert_rejected("pr_build_linux_checks must use the Linux test selection condition") + + def test_each_independent_job_must_remain_available(self): + path = self.workflows / CHECK.LINUX_CHECKS_WORKFLOW + original = path.read_text(encoding="utf-8") + for job_id in sorted(CHECK.INDEPENDENT_LINUX_JOBS): + with self.subTest(job=job_id): + self.assertIn(f"\n {job_id}:\n", original) + path.write_text(original.replace(f"\n {job_id}:\n", + f"\n missing-{job_id}:\n", 1), encoding="utf-8") + self.assert_rejected(f"independent jobs are missing: {job_id}") + path.write_text(original, encoding="utf-8") + + def test_independent_job_cannot_return_to_native_consumer(self): + path = self.workflows / "pr_build_linux.yml" + with path.open("a", encoding="utf-8") as stream: + stream.write("\n linux-test-rust:\n runs-on: ubuntu-24.04\n" + " steps:\n - run: true\n") + self.assert_rejected("independent jobs must stay in pr_build_linux_checks.yml") + + def test_independent_checks_cannot_require_native_artifact_input(self): + self.replace(CHECK.LINUX_CHECKS_WORKFLOW, " workflow_call:\n", + " workflow_call:\n inputs:\n native-library-artifact:\n" + " required: true\n type: string\n") + self.assert_rejected("independent Linux checks must not consume the shared native artifact") + + def test_independent_checks_cannot_download_native_artifact(self): + path = self.workflows / CHECK.LINUX_CHECKS_WORKFLOW + with path.open("a", encoding="utf-8") as stream: + stream.write("\n - uses: actions/download-artifact@v8\n" + " with:\n name: native-lib-linux\n" + " path: native/target/release/\n") + self.assert_rejected("independent Linux checks must not consume the shared native artifact") + + if __name__ == "__main__": unittest.main() diff --git a/dev/ci/test-native-build-selection.py b/dev/ci/test-native-build-selection.py index e1ed1970af..c6b8f7bd09 100644 --- a/dev/ci/test-native-build-selection.py +++ b/dev/ci/test-native-build-selection.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -"""Exercise the native producer and consumer conditions from the real workflow. +"""Exercise native-build and independent-check conditions from the real workflow. Only the small expression subset used by these conditions is translated. This keeps the tests dependency-free like check-ci-config.py; actionlint separately @@ -42,12 +42,13 @@ } DEFAULT = {"pr_build_linux", "spark_3_5", "spark_4_1", "iceberg_1_11"} OPT_IN = ("run-spark-3.4-tests", "run-spark-4.0-tests", "run-iceberg-tests") +LINUX_CHECKS = "pr_build_linux_checks" def conditions(): workflow = (ROOT / ".github/workflows/ci.yml").read_text() expressions = {} - for job in ["build_linux_native", *CONSUMERS]: + for job in ["build_linux_native", LINUX_CHECKS, *CONSUMERS]: block = re.search( r"^ " + job + r":\n.*?(?=^ [a-z][a-z_0-9]*:\n|\Z)", workflow, @@ -107,6 +108,7 @@ def assert_selection(self, files, expected, **event): selected = self.evaluate(flags, **event) self.assertEqual({name for name in CONSUMERS if selected[name]}, expected) self.assertEqual(selected["build_linux_native"], bool(expected)) + self.assertEqual(selected[LINUX_CHECKS], "pr_build_linux" in expected) def test_native_change_uses_default_pr_coverage(self): self.assert_selection(["native/core/src/lib.rs"], DEFAULT) @@ -150,6 +152,11 @@ def test_main_and_manual_runs_include_legacy_consumers(self): def test_producer_change_exercises_all_default_linux_consumers(self): self.assert_selection([".github/workflows/build_linux_native.yml"], DEFAULT) + def test_independent_checks_change_selects_linux_checks_and_tests(self): + self.assert_selection( + [".github/workflows/pr_build_linux_checks.yml"], {"pr_build_linux"} + ) + def test_producer_condition_matches_all_consumer_combinations(self): keys = list(CONSUMERS.values()) label_sets = [ @@ -170,6 +177,9 @@ def test_producer_condition_matches_all_consumer_combinations(self): any(selected[name] for name in CONSUMERS), (flags, event), ) + self.assertEqual( + selected[LINUX_CHECKS], selected["pr_build_linux"], (flags, event) + ) if __name__ == "__main__": From 7374633d49d0e39c650d9a2bcbf9288eee923e74 Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Thu, 10 Sep 2026 18:41:58 +0000 Subject: [PATCH 3/8] ci: restrict shared native build and checks token permissions --- .github/workflows/build_linux_native.yml | 3 +++ .github/workflows/ci.yml | 4 ++++ .github/workflows/pr_build_linux_checks.yml | 3 +++ 3 files changed, 10 insertions(+) diff --git a/.github/workflows/build_linux_native.yml b/.github/workflows/build_linux_native.yml index 05a5c821ee..08c11a0de4 100644 --- a/.github/workflows/build_linux_native.yml +++ b/.github/workflows/build_linux_native.yml @@ -22,6 +22,9 @@ name: Build Linux Native Library on: workflow_call: +permissions: + contents: read + env: RUST_VERSION: stable RUST_BACKTRACE: 1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9501aa4255..6a642aa27b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -232,6 +232,8 @@ jobs: pr_build_linux_checks: name: PR Checks (Linux) needs: changes + permissions: + contents: read if: needs.changes.outputs.build_linux == 'true' uses: ./.github/workflows/pr_build_linux_checks.yml @@ -240,6 +242,8 @@ jobs: build_linux_native: name: Shared Linux Native Library needs: changes + permissions: + contents: read if: | needs.changes.outputs.build_linux == 'true' || needs.changes.outputs.spark_3_4 == 'true' || diff --git a/.github/workflows/pr_build_linux_checks.yml b/.github/workflows/pr_build_linux_checks.yml index c66e6667ed..a68bed26e0 100644 --- a/.github/workflows/pr_build_linux_checks.yml +++ b/.github/workflows/pr_build_linux_checks.yml @@ -23,6 +23,9 @@ name: PR Checks (Linux) on: workflow_call: +permissions: + contents: read + env: RUST_VERSION: stable RUST_BACKTRACE: 1 From 8ae99c89bf5ba359b826345322ce3d76d1a85855 Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Fri, 11 Sep 2026 02:02:32 +0000 Subject: [PATCH 4/8] ci: validate shared native selection with centralized routing --- .github/workflows/README.md | 4 +- dev/ci/test-native-build-selection.py | 160 +++++++++++++++++++------- 2 files changed, 122 insertions(+), 42 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index c9890fdcad..6836cfe7d7 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -174,12 +174,14 @@ umbrella doesn't watch, or operate independently of the rest of CI: ## Changing what runs when -Every heavy job in `ci.yml` is gated on exactly one thing: +Consumer jobs in `ci.yml` use a single routing output, for example: ```yaml if: needs.changes.outputs.spark_3_5 == 'true' ``` +The shared native producer runs when any of its consumers' outputs is true. + That single boolean folds together two separate decisions, both of which live in `dev/ci/compute-changes.py`: diff --git a/dev/ci/test-native-build-selection.py b/dev/ci/test-native-build-selection.py index c6b8f7bd09..600aaf14fb 100644 --- a/dev/ci/test-native-build-selection.py +++ b/dev/ci/test-native-build-selection.py @@ -17,15 +17,22 @@ """Exercise native-build and independent-check conditions from the real workflow. -Only the small expression subset used by these conditions is translated. This -keeps the tests dependency-free like check-ci-config.py; actionlint separately -checks GitHub's expression syntax and workflow dependency graph. +The routing script owns event policy; the workflow only reads its boolean +outputs. Only that small expression subset is translated here, keeping these +tests dependency-free. actionlint separately checks GitHub's expression syntax +and workflow dependency graph. """ import importlib.util import itertools +import json +import os import re +import subprocess +import sys +import tempfile import unittest +from unittest import mock from pathlib import Path ROOT = Path(__file__).resolve().parents[2] @@ -54,21 +61,27 @@ def conditions(): workflow, re.M | re.S, ).group() - expression = block.split(" if: |\n", 1)[1].split(" uses:", 1)[0] - expression = " ".join(expression.split()) + match = re.search( + r"^ if:[ \t]*(?:\|[ \t]*\n(?P(?:^ .*\n?)+)|(?P[^\n]+))", + block, + re.M, + ) + expression = " ".join((match.group("block") or match.group("inline")).split()) + # Event policy belongs in compute-changes.py. Reject a caller that + # silently reintroduces an independent github.event condition. + remaining = re.sub( + r"needs\.changes\.outputs\.[a-z_0-9]+|==|'true'|\|\||[()\s]", + "", + expression, + ) + if remaining: + raise ValueError(f"{job}: unsupported routing condition: {expression}") expression = re.sub( r"needs\.changes\.outputs\.([a-z_0-9]+)", lambda match: f"changes[{match.group(1)!r}]", expression, ) - for before, after in ( - ("github.event.pull_request.labels.*.name", "labels"), - ("github.event.label.name", "label"), - ("github.event.action", "action"), - ("github.event_name", "event"), - ): - expression = expression.replace(before, after) - expression = expression.replace("&&", " and ").replace("||", " or ") + expression = expression.replace("||", " or ") expressions[job] = compile(expression, str(ROOT / ".github/workflows/ci.yml"), "eval") return expressions @@ -83,33 +96,57 @@ def setUpClass(cls): cls.filters = importlib.util.module_from_spec(spec) spec.loader.exec_module(cls.filters) - def evaluate(self, flags, event="pull_request", action="synchronize", labels=(), label=""): - context = { - "changes": {key: str(value).lower() for key, value in flags.items()}, - "event": event, - "action": action, - "labels": labels, - "label": label, - "contains": lambda values, value: value in values, - } + def evaluate(self, flags): + context = {"changes": {key: str(value).lower() for key, value in flags.items()}} return { name: eval(expression, {"__builtins__": {}}, context) for name, expression in self.expressions.items() } - def assert_selection(self, files, expected, **event): - flags = { - key: self.filters.matches(patterns, files) - for key, patterns in self.filters.FILTERS.items() + def cli_outputs(self, files, event): + env = { + **os.environ, + "EVENT_NAME": event["name"], + "EVENT_ACTION": event.get("action", ""), + "LABEL_NAME": event.get("label", ""), + "PR_LABELS": json.dumps(event.get("labels", [])), } - # ci.yml forces every output true for a manual workflow_dispatch. - if event.get("event") == "workflow_dispatch": - flags = dict.fromkeys(flags, True) - selected = self.evaluate(flags, **event) + with tempfile.NamedTemporaryFile(mode="w", encoding="utf-8") as changed: + changed.write("\n".join(files)) + changed.flush() + result = subprocess.run( + [sys.executable, str(ROOT / "dev/ci/compute-changes.py"), changed.name], + env=env, + check=True, + capture_output=True, + text=True, + ) + flags = {} + for line in result.stdout.splitlines(): + key, value = line.split("=", 1) + self.assertIn(value, ("true", "false")) + flags[key] = value == "true" + self.assertEqual(set(flags), set(self.filters.FILTERS)) + return flags + + def assert_selected(self, flags, expected): + selected = self.evaluate(flags) self.assertEqual({name for name in CONSUMERS if selected[name]}, expected) self.assertEqual(selected["build_linux_native"], bool(expected)) self.assertEqual(selected[LINUX_CHECKS], "pr_build_linux" in expected) + def assert_selection( + self, files, expected, event="pull_request", action="synchronize", labels=(), label="" + ): + event = {"name": event, "action": action, "labels": labels, "label": label} + # Manual dispatch bypasses path filtering in the script's CLI. Exercise + # the actual entry point so an empty changed-file list still runs all jobs. + if event["name"] == "workflow_dispatch": + flags = self.cli_outputs(files, event) + else: + flags = self.filters.compute(files, event) + self.assert_selected(flags, expected) + def test_native_change_uses_default_pr_coverage(self): self.assert_selection(["native/core/src/lib.rs"], DEFAULT) @@ -157,29 +194,70 @@ def test_independent_checks_change_selects_linux_checks_and_tests(self): [".github/workflows/pr_build_linux_checks.yml"], {"pr_build_linux"} ) + def test_label_event_cli_uses_only_the_new_gating_label(self): + for label, expected in ( + ("run-iceberg-tests", {"iceberg_1_8", "iceberg_1_9", "iceberg_1_10"}), + ("dependencies", set()), + ): + with self.subTest(label=label): + event = { + "name": "pull_request", + "action": "labeled", + "labels": [*OPT_IN, "dependencies"], + "label": label, + } + flags = self.cli_outputs(["native/core/src/lib.rs"], event) + self.assert_selected(flags, expected) + + def test_producer_follows_policy_changes_without_workflow_edits(self): + with mock.patch.dict(self.filters.POLICY, {"spark_3_4": ["pr", "push"]}): + self.assert_selection(["dev/diffs/3.4.3.diff"], {"spark_3_4"}) + with mock.patch.dict( + self.filters.POLICY, {"build_linux": ["push", "label:run-linux-tests"]} + ): + files = ["common/src/test/ExampleTest.java"] + self.assert_selection(files, set()) + self.assert_selection( + files, {"pr_build_linux"}, action="labeled", + labels=("run-linux-tests",), label="run-linux-tests", + ) + def test_producer_condition_matches_all_consumer_combinations(self): keys = list(CONSUMERS.values()) label_sets = [ tuple(label for label, selected in zip(OPT_IN, mask) if selected) for mask in itertools.product((False, True), repeat=len(OPT_IN)) ] - events = [{"event": "push"}, {"event": "workflow_dispatch"}] + events = [{"name": "push"}, {"name": "workflow_dispatch"}, {"name": "schedule"}] for labels in label_sets: - events.append({"labels": labels}) - for label in (*labels, "dependencies"): - events.append({"action": "labeled", "labels": labels, "label": label}) + for action in ("opened", "synchronize", "reopened"): + events.append({"name": "pull_request", "action": action, "labels": labels}) + for label in (*OPT_IN, "dependencies"): + events.append({ + "name": "pull_request", "action": "labeled", + "labels": labels, "label": label, + }) for mask in itertools.product((False, True), repeat=len(keys)): - flags = dict(zip(keys, mask)) + raw_flags = dict(zip(keys, mask)) for event in events: - selected = self.evaluate(flags, **event) + # The flags consumed by ci.yml already include the policy. + # Exhaust the raw path combinations through that same policy, + # including the CLI's manual-dispatch override. + flags = { + key: event["name"] == "workflow_dispatch" or ( + raw_flags[key] and self.filters.event_allows(key, event) + ) + for key in keys + } + selected = self.evaluate(flags) + for consumer, key in CONSUMERS.items(): + self.assertEqual(selected[consumer], flags[key], (raw_flags, event, consumer)) self.assertEqual( selected["build_linux_native"], - any(selected[name] for name in CONSUMERS), - (flags, event), - ) - self.assertEqual( - selected[LINUX_CHECKS], selected["pr_build_linux"], (flags, event) + any(flags.values()), + (raw_flags, event), ) + self.assertEqual(selected[LINUX_CHECKS], flags["build_linux"], (raw_flags, event)) if __name__ == "__main__": From e72cdb93a3a674f356483dca853468f61efe2eec Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Fri, 11 Sep 2026 16:33:23 +0000 Subject: [PATCH 5/8] ci: derive shared native selection in the routing script --- .github/workflows/README.md | 9 +- .github/workflows/ci.yml | 15 +-- dev/ci/check-ci-config.py | 38 ++++++ dev/ci/compute-changes.py | 45 +++++-- dev/ci/test-ci-config.py | 52 ++++++++ dev/ci/test-native-build-selection.py | 176 ++++++++++++++------------ 6 files changed, 227 insertions(+), 108 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 6836cfe7d7..325fa8cf0a 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -44,7 +44,7 @@ pull_request | merge_group | push to main | workflow_dispatch +---------------+--------------------+ | | Linux checks / macOS / build_linux_native - docs / benchmark (if selected) (if any consumer is selected) + docs / benchmark (if selected) (if any consumer is selected) | +----------------+----------------+ | | | @@ -62,8 +62,9 @@ input. Consumers keep their own Spark/JDK versions and download the library into `native/target/release/`, where Maven expects it. Spark still pre-compiles and shares its JVM test classes separately for each Spark/JDK version. -The producer's condition is the union of those callers' `changes` outputs, -which already include the path and event/label policy in `compute-changes.py`. A Spark-patch-only change therefore gets a native +`compute-changes.py` derives `build_linux_native` as the union of the selected +consumer outputs, after applying path and event/label policy. The workflow +reads that single output. A Spark-patch-only change therefore gets a native build when its Spark caller is selected, even if the Linux build is not. Documentation-only changes, benchmark-only changes, and unrelated label events do not start an unused native build. The event-selection regression @@ -88,7 +89,7 @@ of the Linux CI-profile Cargo cache, and only writes on `main`. | -------------------- | ---------------------------------------------------------------------------------------------------------------------- | ----------------------------------- | | `preflight` | every PR / merge group / push / dispatch / label | none (always runs) | | `changes` | every PR / merge group / push / dispatch / label | runs `dev/ci/compute-changes.py` | -| `build_linux_native` | any selected Linux/Spark/Iceberg consumer | caller conditions in `ci.yml` | +| `build_linux_native` | any selected Linux/Spark/Iceberg consumer | `dev/ci/compute-changes.py` | | `pr_build_linux_checks` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | | `pr_build_linux` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | | `pr_build_macos` | merge group, **or** PR with `run-macos-tests` | `dev/ci/compute-changes.py` | diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a642aa27b..a155651b2b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,6 +156,7 @@ jobs: needs: preflight runs-on: ubuntu-slim outputs: + build_linux_native: ${{ steps.compute.outputs.build_linux_native }} build_linux: ${{ steps.compute.outputs.build_linux }} build_macos: ${{ steps.compute.outputs.build_macos }} benchmark: ${{ steps.compute.outputs.benchmark }} @@ -237,23 +238,13 @@ jobs: if: needs.changes.outputs.build_linux == 'true' uses: ./.github/workflows/pr_build_linux_checks.yml - # Build once when any native-library consumer is selected. Each output - # already includes the path, event, and label policy from compute-changes.py. + # compute-changes.py derives this output from the selected native consumers. build_linux_native: name: Shared Linux Native Library needs: changes permissions: contents: read - if: | - needs.changes.outputs.build_linux == 'true' || - needs.changes.outputs.spark_3_4 == 'true' || - needs.changes.outputs.spark_3_5 == 'true' || - needs.changes.outputs.spark_4_0 == 'true' || - needs.changes.outputs.spark_4_1 == 'true' || - needs.changes.outputs.iceberg_1_8 == 'true' || - needs.changes.outputs.iceberg_1_9 == 'true' || - needs.changes.outputs.iceberg_1_10 == 'true' || - needs.changes.outputs.iceberg_1_11 == 'true' + if: needs.changes.outputs.build_linux_native == 'true' uses: ./.github/workflows/build_linux_native.yml pr_build_linux: diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 5ec3c7d0fe..e3c3c6d053 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -541,7 +541,44 @@ def condition(job): return failures +def native_selection_failures(jobs): + """Return errors when workflow gates diverge from the Python selector. + + `jobs` is the ci.yml job mapping returned by block_mapping, with each value + holding an inline scalar and indented body. Read the selector's consumer + keys and require a matching caller, direct output export, and simple gate + for each one, plus the producer. This checks wiring without evaluating YAML + expressions. Inputs and files are not mutated; import/read errors propagate. + """ + consumers = {"pr_build_linux" if key == "build_linux" else key: key + for key in load_filters().NATIVE_CONSUMERS} + actual_consumers = { + job_id for job_id, (_, body) in jobs.items() + if scalar(block_mapping(body, 4).get("uses", ("", ""))[0]) + .removeprefix("./.github/workflows/") in SHARED_NATIVE_CONSUMERS + } + failures = [] + if actual_consumers != consumers.keys(): + failures.append("ci.yml: native consumer calls must match NATIVE_CONSUMERS " + "in compute-changes.py") + changes = block_mapping(jobs.get("changes", ("", ""))[1], 4) + outputs = block_mapping(changes.get("outputs", ("", ""))[1], 6) + for job_id, output in {SHARED_NATIVE_JOB: SHARED_NATIVE_JOB, **consumers}.items(): + fields = block_mapping(jobs.get(job_id, ("", ""))[1], 4) + if fields.get("if", ("", ""))[0] != f"needs.changes.outputs.{output} == 'true'": + failures.append(f"ci.yml: {job_id} must select only changes.outputs.{output}") + if outputs.get(output, ("", ""))[0] != f"${{{{ steps.compute.outputs.{output} }}}}": + failures.append(f"ci.yml: changes must export steps.compute.outputs.{output}") + return failures + + def artifact_failures(workflows): + """Read workflow files and return artifact, routing, and independence errors. + + `workflows` is a directory Path containing ci.yml and the reusable workflows. + Files and parsed mappings are read only. An empty list means all invariants + passed; file-read and selector-import errors propagate to the caller. + """ ci = (workflows / "ci.yml").read_text(encoding="utf-8") jobs = block_mapping(block_mapping(ci, 0).get("jobs", ("", ""))[1], 2) call_counts = {} @@ -551,6 +588,7 @@ def artifact_failures(workflows): artifacts = {path.name: artifact_names(path) for path in sorted(workflows.glob("*.y*ml"))} failures = shared_native_failures(workflows, jobs, artifacts) failures.extend(linux_checks_failures(workflows, jobs)) + failures.extend(native_selection_failures(jobs)) shared_wiring_valid = not failures for filename, (uploads, downloads) in artifacts.items(): path = workflows / filename diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index e874479652..023985d991 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -384,6 +384,21 @@ "iceberg_1_11": ["pr", "queue"], } +# Output keys for callers that download the shared Linux native library. +# The producer has no independent path or event policy: it runs exactly when +# at least one of these consumers is selected after FILTERS and POLICY apply. +NATIVE_CONSUMERS = ( + "build_linux", + "spark_3_4", + "spark_3_5", + "spark_4_0", + "spark_4_1", + "iceberg_1_8", + "iceberg_1_9", + "iceberg_1_10", + "iceberg_1_11", +) + def gating_labels(job): return [t[len("label:"):] for t in POLICY[job] if t.startswith("label:")] @@ -426,11 +441,23 @@ def event_allows(job, event): def compute(files, event): - """Return {job: bool}, folding the path filter and the event policy.""" - return { - name: event_allows(name, event) and matches(patterns, files) + """Return a new {output: bool} mapping for consumers and their native build. + + `files` is a reusable sequence of repository-relative changed paths; + `event` has the fields described by event_allows(). Neither input is + mutated. Manual dispatch selects every route even with no changed files; + other events require both path and event matches. The shared native build + is selected only after those decisions, so a denied opt-in consumer cannot + start an unused producer. Unknown events select nothing. Configuration + lookup failures propagate as KeyError rather than returning partial output. + """ + manual = event.get("name") == "workflow_dispatch" + outputs = { + name: event_allows(name, event) and (manual or matches(patterns, files)) for name, patterns in FILTERS.items() } + outputs["build_linux_native"] = any(outputs[name] for name in NATIVE_CONSUMERS) + return outputs def event_from_env(): @@ -485,12 +512,12 @@ def matches(patterns, files): if __name__ == "__main__": event = event_from_env() # workflow_dispatch has no meaningful base to diff against, so the caller - # passes an empty list and every path filter is treated as matched. + # passes an empty list. compute() applies its override before deriving the + # native producer, and every event emits the same complete set of outputs. if event["name"] == "workflow_dispatch": - for name in FILTERS: - print(f"{name}=true") - sys.exit(0) - files_path = Path(sys.argv[1]) - files = [line.strip() for line in files_path.read_text().splitlines() if line.strip()] + files = [] + else: + files_path = Path(sys.argv[1]) + files = [line.strip() for line in files_path.read_text().splitlines() if line.strip()] for name, flag in compute(files, event).items(): print(f"{name}={'true' if flag else 'false'}") diff --git a/dev/ci/test-ci-config.py b/dev/ci/test-ci-config.py index 9a52b861e3..7e67d32c7f 100644 --- a/dev/ci/test-ci-config.py +++ b/dev/ci/test-ci-config.py @@ -51,6 +51,58 @@ def assert_rejected(self, expected): def test_real_workflows_have_valid_shared_artifacts(self): self.assertEqual(CHECK.artifact_failures(self.workflows), []) + def test_native_gates_cannot_bypass_selected_outputs(self): + """Reject an inverted gate for each producer/consumer in a temporary copy. + + Each mutation starts from the original workflow and must report the + changed job. The source tree is untouched and the fixture is restored. + """ + path = self.workflows / "ci.yml" + original = path.read_text(encoding="utf-8") + consumers = CHECK.load_filters().NATIVE_CONSUMERS + for output in (CHECK.SHARED_NATIVE_JOB, *consumers): + job_id = "pr_build_linux" if output == "build_linux" else output + with self.subTest(job=job_id): + start = original.index(f"\n {job_id}:\n") + body = original[start:].replace( + f"needs.changes.outputs.{output} == 'true'", + f"needs.changes.outputs.{output} == 'false'", 1) + path.write_text(original[:start] + body, encoding="utf-8") + self.assert_rejected(f"{job_id} must select only changes.outputs.{output}") + path.write_text(original, encoding="utf-8") + + def test_native_outputs_must_be_exported_without_remapping(self): + """Reject missing and remapped output exports in temporary workflows. + + Every producer/consumer output is checked independently, covering the + case where Python selects a producer that the workflow never starts. + Only the fixture is mutated and it is restored after all assertions. + """ + path = self.workflows / "ci.yml" + original = path.read_text(encoding="utf-8") + consumers = CHECK.load_filters().NATIVE_CONSUMERS + for output in (CHECK.SHARED_NATIVE_JOB, *consumers): + expression = f"${{{{ steps.compute.outputs.{output} }}}}" + self.assertIn(expression, original) + for replacement in ("", "${{ steps.compute.outputs.docs }}"): + with self.subTest(output=output, replacement=replacement): + path.write_text(original.replace(expression, replacement, 1), encoding="utf-8") + self.assert_rejected(f"changes must export steps.compute.outputs.{output}") + path.write_text(original, encoding="utf-8") + + def test_new_native_consumer_must_join_selector(self): + """Reject an extra consumer absent from Python's producer union. + + Append a caller to the temporary workflow only. The fixture directory + is removed by tear-down; no repository file is changed. + """ + path = self.workflows / "ci.yml" + with path.open("a", encoding="utf-8") as stream: + stream.write("\n spark_future:\n needs: [changes, build_linux_native]\n" + " uses: ./.github/workflows/spark_sql_test_reusable.yml\n" + " with:\n native-library-artifact: native-lib-linux\n") + self.assert_rejected("native consumer calls must match NATIVE_CONSUMERS") + def test_missing_shared_producer_is_rejected(self): (self.workflows / CHECK.SHARED_NATIVE_WORKFLOW).unlink() self.assert_rejected("shared native producer is missing") diff --git a/dev/ci/test-native-build-selection.py b/dev/ci/test-native-build-selection.py index 600aaf14fb..21ce5f96db 100644 --- a/dev/ci/test-native-build-selection.py +++ b/dev/ci/test-native-build-selection.py @@ -15,19 +15,17 @@ # specific language governing permissions and limitations # under the License. -"""Exercise native-build and independent-check conditions from the real workflow. +"""Exercise shared native-build selection through the routing script and CLI. -The routing script owns event policy; the workflow only reads its boolean -outputs. Only that small expression subset is translated here, keeping these -tests dependency-free. actionlint separately checks GitHub's expression syntax -and workflow dependency graph. +Consumer outputs already include path and event policy. The native producer +must be their union, including for manual dispatch. check-ci-config.py checks +the workflow's output wiring; actionlint validates its syntax and dependencies. """ import importlib.util import itertools import json import os -import re import subprocess import sys import tempfile @@ -49,61 +47,31 @@ } DEFAULT = {"pr_build_linux", "spark_3_5", "spark_4_1", "iceberg_1_11"} OPT_IN = ("run-spark-3.4-tests", "run-spark-4.0-tests", "run-iceberg-tests") -LINUX_CHECKS = "pr_build_linux_checks" - - -def conditions(): - workflow = (ROOT / ".github/workflows/ci.yml").read_text() - expressions = {} - for job in ["build_linux_native", LINUX_CHECKS, *CONSUMERS]: - block = re.search( - r"^ " + job + r":\n.*?(?=^ [a-z][a-z_0-9]*:\n|\Z)", - workflow, - re.M | re.S, - ).group() - match = re.search( - r"^ if:[ \t]*(?:\|[ \t]*\n(?P(?:^ .*\n?)+)|(?P[^\n]+))", - block, - re.M, - ) - expression = " ".join((match.group("block") or match.group("inline")).split()) - # Event policy belongs in compute-changes.py. Reject a caller that - # silently reintroduces an independent github.event condition. - remaining = re.sub( - r"needs\.changes\.outputs\.[a-z_0-9]+|==|'true'|\|\||[()\s]", - "", - expression, - ) - if remaining: - raise ValueError(f"{job}: unsupported routing condition: {expression}") - expression = re.sub( - r"needs\.changes\.outputs\.([a-z_0-9]+)", - lambda match: f"changes[{match.group(1)!r}]", - expression, - ) - expression = expression.replace("||", " or ") - expressions[job] = compile(expression, str(ROOT / ".github/workflows/ci.yml"), "eval") - return expressions class NativeBuildSelectionTest(unittest.TestCase): @classmethod def setUpClass(cls): - cls.expressions = conditions() + """Load one routing module for this suite; import errors fail setup. + + Store it on the test class. Individual policy/filter patches restore + this module's dictionaries when their context exits, including failure. + """ spec = importlib.util.spec_from_file_location( "compute_changes", ROOT / "dev/ci/compute-changes.py" ) cls.filters = importlib.util.module_from_spec(spec) spec.loader.exec_module(cls.filters) - def evaluate(self, flags): - context = {"changes": {key: str(value).lower() for key, value in flags.items()}} - return { - name: eval(expression, {"__builtins__": {}}, context) - for name, expression in self.expressions.items() - } - def cli_outputs(self, files, event): + """Return validated boolean outputs from one real CLI invocation. + + Pass repository-relative `files` through a temporary text file and the + event fields through a child-only environment. Neither input nor the + parent environment changes. The temporary file closes on success or + failure; a failed process raises, and malformed or missing outputs fail + assertions rather than being interpreted as a skipped native build. + """ env = { **os.environ, "EVENT_NAME": event["name"], @@ -125,26 +93,33 @@ def cli_outputs(self, files, event): for line in result.stdout.splitlines(): key, value = line.split("=", 1) self.assertIn(value, ("true", "false")) + self.assertNotIn(key, flags) flags[key] = value == "true" - self.assertEqual(set(flags), set(self.filters.FILTERS)) + self.assertEqual(set(flags), set(self.filters.FILTERS) | {"build_linux_native"}) return flags def assert_selected(self, flags, expected): - selected = self.evaluate(flags) - self.assertEqual({name for name in CONSUMERS if selected[name]}, expected) - self.assertEqual(selected["build_linux_native"], bool(expected)) - self.assertEqual(selected[LINUX_CHECKS], "pr_build_linux" in expected) + """Assert consumer job IDs and producer selection without mutating inputs. + + `flags` is the complete output-to-bool mapping; `expected` contains CI + consumer job IDs, whose output keys are pinned in CONSUMERS. Return + None on success; any missing key or selection mismatch fails the test. + """ + self.assertEqual({job for job, key in CONSUMERS.items() if flags[key]}, expected) + self.assertEqual(flags["build_linux_native"], bool(expected)) def assert_selection( self, files, expected, event="pull_request", action="synchronize", labels=(), label="" ): + """Check compute() for changed paths, event fields, and expected job IDs. + + Build a fresh event mapping from the supplied values and assert the + complete native selection. No caller input changes; return None or + propagate the computation/assertion failure. CLI behavior is exercised + separately so both entry points cover manual dispatch's empty input. + """ event = {"name": event, "action": action, "labels": labels, "label": label} - # Manual dispatch bypasses path filtering in the script's CLI. Exercise - # the actual entry point so an empty changed-file list still runs all jobs. - if event["name"] == "workflow_dispatch": - flags = self.cli_outputs(files, event) - else: - flags = self.filters.compute(files, event) + flags = self.filters.compute(files, event) self.assert_selected(flags, expected) def test_native_change_uses_default_pr_coverage(self): @@ -182,9 +157,42 @@ def test_new_iceberg_label_runs_only_opt_in_versions(self): action="labeled", labels=OPT_IN, label="run-iceberg-tests", ) - def test_main_and_manual_runs_include_legacy_consumers(self): + def test_main_runs_include_legacy_consumers(self): + """Pin the current main-push policy until the merge-queue policy lands.""" self.assert_selection(["native/core/src/lib.rs"], set(CONSUMERS), event="push") + + def test_manual_runs_include_legacy_consumers_without_changed_files(self): + """Assert empty-input dispatch selects every consumer in compute and CLI.""" self.assert_selection([], set(CONSUMERS), event="workflow_dispatch") + flags = self.cli_outputs([], {"name": "workflow_dispatch"}) + self.assert_selected(flags, set(CONSUMERS)) + self.assertTrue(all(flags.values())) + + def test_empty_changes_and_unsupported_events_skip_native(self): + """Assert ordinary empty diffs and unsupported events select no consumers.""" + self.assert_selection([], set()) + self.assert_selection(["native/core/src/lib.rs"], set(), event="schedule") + + def test_nonconsumer_outputs_do_not_select_native(self): + """Select each unrelated route alone and ensure it cannot start native CI. + + Temporarily give every filter a distinct synthetic path to separate + macOS from its normally overlapping Linux inputs. The patch restores + the real filters on exit, including when an assertion fails. + """ + filters = {key: [key] for key in self.filters.FILTERS} + with mock.patch.dict(self.filters.FILTERS, filters, clear=True): + for key in ("build_macos", "benchmark", "docs"): + with self.subTest(key=key): + flags = self.filters.compute([key], {"name": "push"}) + self.assertEqual({name for name, selected in flags.items() if selected}, {key}) + + def test_cli_emits_native_output_for_selected_and_skipped_runs(self): + """Assert real CLI output includes the producer on both true and false paths.""" + event = {"name": "pull_request", "action": "synchronize", "labels": []} + for files, expected in ((["native/core/src/lib.rs"], DEFAULT), ([], set())): + with self.subTest(files=files): + self.assert_selected(self.cli_outputs(files, event), expected) def test_producer_change_exercises_all_default_linux_consumers(self): self.assert_selection([".github/workflows/build_linux_native.yml"], DEFAULT) @@ -222,7 +230,15 @@ def test_producer_follows_policy_changes_without_workflow_edits(self): labels=("run-linux-tests",), label="run-linux-tests", ) - def test_producer_condition_matches_all_consumer_combinations(self): + def test_producer_output_matches_all_consumer_combinations(self): + """Exhaust all 512 consumer path masks across event and label combinations. + + Synthetic one-path filters exercise compute() without relying on real + paths overlapping particular consumers. Unrelated routes also match, + guarding against accidentally including them in the native union. Only + FILTERS is patched, and it is restored on success or assertion failure; + the actual event policy and native-output computation always execute. + """ keys = list(CONSUMERS.values()) label_sets = [ tuple(label for label, selected in zip(OPT_IN, mask) if selected) @@ -237,27 +253,21 @@ def test_producer_condition_matches_all_consumer_combinations(self): "name": "pull_request", "action": "labeled", "labels": labels, "label": label, }) - for mask in itertools.product((False, True), repeat=len(keys)): - raw_flags = dict(zip(keys, mask)) - for event in events: - # The flags consumed by ci.yml already include the policy. - # Exhaust the raw path combinations through that same policy, - # including the CLI's manual-dispatch override. - flags = { - key: event["name"] == "workflow_dispatch" or ( - raw_flags[key] and self.filters.event_allows(key, event) - ) - for key in keys - } - selected = self.evaluate(flags) - for consumer, key in CONSUMERS.items(): - self.assertEqual(selected[consumer], flags[key], (raw_flags, event, consumer)) - self.assertEqual( - selected["build_linux_native"], - any(flags.values()), - (raw_flags, event), - ) - self.assertEqual(selected[LINUX_CHECKS], flags["build_linux"], (raw_flags, event)) + filters = {key: [key] for key in self.filters.FILTERS} + unrelated = sorted(set(filters) - set(keys)) + with mock.patch.dict(self.filters.FILTERS, filters, clear=True): + for mask in itertools.product((False, True), repeat=len(keys)): + raw_flags = dict(zip(keys, mask)) + files = [key for key, selected in raw_flags.items() if selected] + unrelated + for event in events: + flags = self.filters.compute(files, event) + expected = { + job for job, key in CONSUMERS.items() + if event["name"] == "workflow_dispatch" or ( + raw_flags[key] and self.filters.event_allows(key, event) + ) + } + self.assert_selected(flags, expected) if __name__ == "__main__": From 0151f94dd893dac9cf5f65edc2c7180c999310ae Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Fri, 11 Sep 2026 16:33:23 +0000 Subject: [PATCH 6/8] ci: skip Rust setup when compiling Spark JVM artifacts --- .github/workflows/spark_sql_test_reusable.yml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/spark_sql_test_reusable.yml b/.github/workflows/spark_sql_test_reusable.yml index aa2c860e2f..5032e9c4c8 100644 --- a/.github/workflows/spark_sql_test_reusable.yml +++ b/.github/workflows/spark_sql_test_reusable.yml @@ -73,6 +73,9 @@ jobs: build: name: Build JVM Test Classes runs-on: ubuntu-24.04 + # Preserve the container workspace and Maven-cache paths used by the test + # jobs that reuse this Spark compilation and its Zinc incremental analysis. + # This job packages the downloaded native library and only needs Java setup. container: image: amd64/rust outputs: @@ -84,11 +87,11 @@ jobs: id: modules run: python3 dev/ci/spark-sql-modules.py --modules "${{ inputs.modules }}" --github-output "$GITHUB_OUTPUT" - - name: Setup Rust & Java toolchain - uses: ./.github/actions/setup-builder + - name: Setup Java toolchain + uses: actions/setup-java@v4 with: - rust-version: ${{ env.RUST_VERSION }} - jdk-version: ${{ inputs.java }} + distribution: zulu + java-version: ${{ inputs.java }} - name: Download native library uses: actions/download-artifact@v8 From 17f6f8f0d179814f68b2315999fd9f2c17a8d537 Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Fri, 11 Sep 2026 21:27:44 +0000 Subject: [PATCH 7/8] ci: align shared native builds with merge queue policy --- .github/workflows/README.md | 25 ++-- .github/workflows/ci.yml | 1 + .github/workflows/spark_sql_test_reusable.yml | 2 +- dev/ci/test-ci-config.py | 19 ++- dev/ci/test-native-build-selection.py | 117 +++++++++++++++--- 5 files changed, 129 insertions(+), 35 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 325fa8cf0a..a9d774a66f 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -22,14 +22,16 @@ ruleset in `.asf.yaml`. That splits CI into two tiers: against the merge result rather than against the PR head. Every queue-only job has a `run-*` label that opts a pull request into it -early, listed in the diagram below. +early, listed in the table below. -Heavy jobs have no `push` tier. The queue already tested the exact tree that +Most heavy jobs have no `push` tier. The queue already tested the exact tree that lands, so re-running them on push to main would double the cost of every -merge. Two jobs are still on `push`: `docs`, because it deploys to `asf-site` -and has to run after the commit is on main, and `pr_build_linux`, because of -`actions/cache` scoping. A pull request can only restore caches saved on its -own branch or on `main`, and the queue runs on a throwaway +merge. Two routes are still on `push`: `docs`, because it deploys to `asf-site` +and has to run after the commit is on main, and `build_linux`, because of +`actions/cache` scoping. The Linux route selects `pr_build_linux_checks` and +`pr_build_linux`, with the shared `build_linux_native` producer supplying the +latter. Spark SQL and Iceberg consumers stay off on push. A pull request can +only restore caches saved on its own branch or on `main`, and the queue runs on a throwaway `gh-readonly-queue/*` branch whose caches are deleted with it. Without a push run, a `Cargo.lock` or `pom.xml` change would leave the cargo-registry, Maven and TPC-H/TPC-DS caches on `main` stale until the next unrelated change. @@ -68,7 +70,9 @@ reads that single output. A Spark-patch-only change therefore gets a native build when its Spark caller is selected, even if the Linux build is not. Documentation-only changes, benchmark-only changes, and unrelated label events do not start an unused native build. The event-selection regression -test checks that the producer and its consumers stay in agreement. +test checks that the producer and its consumers stay in agreement across PR, +merge-group, push, and manual runs. A macOS-only or benchmark-only label run +also skips this producer because neither job consumes the Linux artifact. Linux lint, compile-only checks, Celeborn compatibility tests, and Rust debug tests run in `pr_build_linux_checks.yml` as soon as change selection completes. @@ -113,12 +117,13 @@ safe to make a required check. ### Label events `ci.yml` also fires on `pull_request.types: [labeled]`, so applying -`run-spark-3.4-tests`, `run-spark-4.0-tests` or `run-iceberg-tests` starts the -job that label gates without needing a new push. GitHub cannot filter a +`run-spark-3.4-tests`, `run-spark-3.5-tests`, `run-spark-4.0-tests`, +`run-iceberg-tests`, `run-macos-tests`, or `run-benchmark-check` starts the +jobs that label gates without needing a new push. GitHub cannot filter a `pull_request` trigger by label name, so **every** label added to a PR starts a run, including labels that gate nothing. -Two rules keep those runs from corrupting the PR's status: +Three rules keep those runs from corrupting the PR's status: - `preflight` and `changes` carry no event guard and run every time. A job held back by `if:` still publishes a check run under its own name with conclusion diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a155651b2b..52ab6d2a9d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -291,6 +291,7 @@ jobs: spark_3_5: name: Spark SQL Tests (Spark 3.5) needs: [changes, build_linux_native] + # Queue-only by default; PRs need the `run-spark-3.5-tests` label. if: needs.changes.outputs.spark_3_5 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: diff --git a/.github/workflows/spark_sql_test_reusable.yml b/.github/workflows/spark_sql_test_reusable.yml index 5032e9c4c8..2f4576db4b 100644 --- a/.github/workflows/spark_sql_test_reusable.yml +++ b/.github/workflows/spark_sql_test_reusable.yml @@ -94,7 +94,7 @@ jobs: java-version: ${{ inputs.java }} - name: Download native library - uses: actions/download-artifact@v8 + uses: ./.github/actions/download-artifact-retry with: name: ${{ inputs.native-library-artifact }} path: native/target/release/ diff --git a/dev/ci/test-ci-config.py b/dev/ci/test-ci-config.py index 7e67d32c7f..1ef51d6b4b 100644 --- a/dev/ci/test-ci-config.py +++ b/dev/ci/test-ci-config.py @@ -251,12 +251,21 @@ def test_independent_checks_cannot_require_native_artifact_input(self): self.assert_rejected("independent Linux checks must not consume the shared native artifact") def test_independent_checks_cannot_download_native_artifact(self): + """Reject direct and retried downloads of native artifacts by independent checks. + + Each action is appended to a fresh copy of the temporary workflow. The + original fixture is restored after successful assertions and its whole + temporary directory is cleaned up even if a check fails. + """ path = self.workflows / CHECK.LINUX_CHECKS_WORKFLOW - with path.open("a", encoding="utf-8") as stream: - stream.write("\n - uses: actions/download-artifact@v8\n" - " with:\n name: native-lib-linux\n" - " path: native/target/release/\n") - self.assert_rejected("independent Linux checks must not consume the shared native artifact") + original = path.read_text(encoding="utf-8") + for action in ("actions/download-artifact@v8", "./.github/actions/download-artifact-retry"): + with self.subTest(action=action): + path.write_text(original + f"\n - uses: {action}\n" + " with:\n name: native-lib-linux\n" + " path: native/target/release/\n", encoding="utf-8") + self.assert_rejected("independent Linux checks must not consume the shared native artifact") + path.write_text(original, encoding="utf-8") if __name__ == "__main__": diff --git a/dev/ci/test-native-build-selection.py b/dev/ci/test-native-build-selection.py index 21ce5f96db..22e76d16e6 100644 --- a/dev/ci/test-native-build-selection.py +++ b/dev/ci/test-native-build-selection.py @@ -45,8 +45,10 @@ "iceberg_1_10": "iceberg_1_10", "iceberg_1_11": "iceberg_1_11", } -DEFAULT = {"pr_build_linux", "spark_3_5", "spark_4_1", "iceberg_1_11"} -OPT_IN = ("run-spark-3.4-tests", "run-spark-4.0-tests", "run-iceberg-tests") +DEFAULT = {"pr_build_linux", "spark_4_1", "iceberg_1_11"} +OPT_IN = ( + "run-spark-3.4-tests", "run-spark-3.5-tests", "run-spark-4.0-tests", "run-iceberg-tests" +) class NativeBuildSelectionTest(unittest.TestCase): @@ -131,13 +133,25 @@ def test_docs_and_benchmarks_do_not_build_native(self): self.assert_selection([path], set()) def test_spark_patch_does_not_require_linux_test_workflow(self): - self.assert_selection(["dev/diffs/3.5.9.diff"], {"spark_3_5"}) + """Assert a default-tier Spark patch selects native without Linux tests.""" + self.assert_selection(["dev/diffs/4.1.3.diff"], {"spark_4_1"}) def test_legacy_patch_needs_opt_in(self): - self.assert_selection(["dev/diffs/3.4.3.diff"], set()) - self.assert_selection( - ["dev/diffs/3.4.3.diff"], {"spark_3_4"}, labels=("run-spark-3.4-tests",) - ) + """Assert queue-tier Spark patches need their label on ordinary PR runs. + + Each repository-relative patch path is tested with and without its + matching label. Inputs and routing tables stay unchanged; a mismatch + in either the consumer or producer selection fails the assertion. + """ + for version, job, label in ( + ("3.4.3", "spark_3_4", "run-spark-3.4-tests"), + ("3.5.9", "spark_3_5", "run-spark-3.5-tests"), + ("4.0.4", "spark_4_0", "run-spark-4.0-tests"), + ): + with self.subTest(version=version): + files = [f"dev/diffs/{version}.diff"] + self.assert_selection(files, set()) + self.assert_selection(files, {job}, labels=(label,)) def test_unrelated_label_does_not_duplicate_existing_runs(self): self.assert_selection( @@ -146,10 +160,42 @@ def test_unrelated_label_does_not_duplicate_existing_runs(self): ) def test_new_spark_label_runs_only_selected_version(self): - self.assert_selection( - ["native/core/src/lib.rs"], {"spark_3_4"}, action="labeled", - labels=OPT_IN, label="run-spark-3.4-tests", - ) + """Assert each new Spark label selects only its consumer and native build. + + Use a shared native source path with every opt-in label present, so + the event's newly added label must narrow the selection. No fixtures + are mutated; incorrect selection fails through assert_selection(). + """ + for job, label in ( + ("spark_3_4", "run-spark-3.4-tests"), + ("spark_3_5", "run-spark-3.5-tests"), + ("spark_4_0", "run-spark-4.0-tests"), + ): + with self.subTest(label=label): + self.assert_selection( + ["native/core/src/lib.rs"], {job}, action="labeled", + labels=OPT_IN, label=label, + ) + + def test_nonconsumer_labels_do_not_build_native(self): + """Assert macOS and benchmark label runs select no shared native build. + + Both real routes match the changed paths, and all consumer opt-ins are + present. Check that the newly labeled route runs while every native + consumer stays off. No input or routing configuration is changed. + """ + for label, key in ( + ("run-macos-tests", "build_macos"), + ("run-benchmark-check", "benchmark"), + ): + with self.subTest(label=label): + flags = self.filters.compute( + ["native/core/src/lib.rs", "native/core/benches/parquet_read.rs"], + {"name": "pull_request", "action": "labeled", + "labels": (*OPT_IN, label), "label": label}, + ) + self.assertTrue(flags[key]) + self.assert_selected(flags, set()) def test_new_iceberg_label_runs_only_opt_in_versions(self): self.assert_selection( @@ -157,9 +203,27 @@ def test_new_iceberg_label_runs_only_opt_in_versions(self): action="labeled", labels=OPT_IN, label="run-iceberg-tests", ) - def test_main_runs_include_legacy_consumers(self): - """Pin the current main-push policy until the merge-queue policy lands.""" - self.assert_selection(["native/core/src/lib.rs"], set(CONSUMERS), event="push") + def test_merge_queue_runs_include_legacy_consumers(self): + """Assert the queue selects all native consumers in compute() and CLI. + + A native source edit matches every consumer without opt-in labels. + Check both entry points; the CLI's temporary inputs are cleaned up + by cli_outputs(), and neither check changes the routing policy. + """ + files = ["native/core/src/lib.rs"] + self.assert_selection(files, set(CONSUMERS), event="merge_group") + self.assert_selected(self.cli_outputs(files, {"name": "merge_group"}), set(CONSUMERS)) + + def test_main_runs_only_linux_consumers_to_refresh_caches(self): + """Assert push selects the Linux consumer and native build in both APIs. + + Main's cache refresh needs the shared producer, while queue-only test + consumers stay off. The native source path and event are read-only; + cli_outputs() owns and cleans up its temporary changed-files input. + """ + files = ["native/core/src/lib.rs"] + self.assert_selection(files, {"pr_build_linux"}, event="push") + self.assert_selected(self.cli_outputs(files, {"name": "push"}), {"pr_build_linux"}) def test_manual_runs_include_legacy_consumers_without_changed_files(self): """Assert empty-input dispatch selects every consumer in compute and CLI.""" @@ -177,14 +241,17 @@ def test_nonconsumer_outputs_do_not_select_native(self): """Select each unrelated route alone and ensure it cannot start native CI. Temporarily give every filter a distinct synthetic path to separate - macOS from its normally overlapping Linux inputs. The patch restores - the real filters on exit, including when an assertion fails. + macOS from its normally overlapping Linux inputs. Use each route's + permitted event so the assertion checks an active unrelated job. The + patch restores the real filters on exit, including assertion failure. """ filters = {key: [key] for key in self.filters.FILTERS} with mock.patch.dict(self.filters.FILTERS, filters, clear=True): - for key in ("build_macos", "benchmark", "docs"): + for key, event in ( + ("build_macos", "merge_group"), ("benchmark", "merge_group"), ("docs", "push") + ): with self.subTest(key=key): - flags = self.filters.compute([key], {"name": "push"}) + flags = self.filters.compute([key], {"name": event}) self.assertEqual({name for name, selected in flags.items() if selected}, {key}) def test_cli_emits_native_output_for_selected_and_skipped_runs(self): @@ -203,7 +270,14 @@ def test_independent_checks_change_selects_linux_checks_and_tests(self): ) def test_label_event_cli_uses_only_the_new_gating_label(self): + """Assert the CLI derives native selection from the new label only. + + Exercise Spark 3.5's queue opt-in, Iceberg's grouped opt-in, and an + unrelated label. cli_outputs() isolates and cleans up the child + environment and temporary file; routing tables remain unchanged. + """ for label, expected in ( + ("run-spark-3.5-tests", {"spark_3_5"}), ("run-iceberg-tests", {"iceberg_1_8", "iceberg_1_9", "iceberg_1_10"}), ("dependencies", set()), ): @@ -238,13 +312,18 @@ def test_producer_output_matches_all_consumer_combinations(self): guarding against accidentally including them in the native union. Only FILTERS is patched, and it is restored on success or assertion failure; the actual event policy and native-output computation always execute. + Include every subset of native opt-in labels and the merge queue, + cache-refresh push, manual dispatch, and unsupported schedule events. """ keys = list(CONSUMERS.values()) label_sets = [ tuple(label for label, selected in zip(OPT_IN, mask) if selected) for mask in itertools.product((False, True), repeat=len(OPT_IN)) ] - events = [{"name": "push"}, {"name": "workflow_dispatch"}, {"name": "schedule"}] + events = [ + {"name": "merge_group"}, {"name": "push"}, + {"name": "workflow_dispatch"}, {"name": "schedule"}, + ] for labels in label_sets: for action in ("opened", "synchronize", "reopened"): events.append({"name": "pull_request", "action": action, "labels": labels}) From 79c161b293f2147b1d25233a47a15222d7289cf9 Mon Sep 17 00:00:00 2001 From: Chao Sun Date: Sun, 13 Sep 2026 20:36:46 +0000 Subject: [PATCH 8/8] ci: preserve Maven bootstrap and Hive native routing --- .github/workflows/README.md | 66 +++++--- dev/ci/check-ci-config.py | 89 +++++++++-- dev/ci/compute-changes.py | 37 +++-- dev/ci/test-ci-config.py | 212 +++++++++++++++++++++++--- dev/ci/test-native-build-selection.py | 85 ++++++++--- 5 files changed, 401 insertions(+), 88 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index a9d774a66f..163adf54c4 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -89,25 +89,25 @@ of the Linux CI-profile Cargo cache, and only writes on `main`. ## What runs when -| Job in `ci.yml` | Triggered by | Routing rule | -| -------------------- | ---------------------------------------------------------------------------------------------------------------------- | ----------------------------------- | -| `preflight` | every PR / merge group / push / dispatch / label | none (always runs) | -| `changes` | every PR / merge group / push / dispatch / label | runs `dev/ci/compute-changes.py` | -| `build_linux_native` | any selected Linux/Spark/Iceberg consumer | `dev/ci/compute-changes.py` | -| `pr_build_linux_checks` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | -| `pr_build_linux` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | -| `pr_build_macos` | merge group, **or** PR with `run-macos-tests` | `dev/ci/compute-changes.py` | -| `pr_benchmark_check` | merge group, **or** PR with `run-benchmark-check` | benchmark sources only | -| `docs` | push to main, paths matched | `.asf.yaml`, `docs/**`, `docs.yaml` | -| `spark_3_5` | merge group, **or** PR with `run-spark-3.5-tests` | Spark 3.5 sources | -| `spark_4_1` | PR or merge group, paths matched; the `sql_hive` shards only in the merge group **or** with `run-spark-4.1-hive-tests` | Spark 4.1 sources | -| `spark_3_4` | merge group, **or** PR with `run-spark-3.4-tests` | Spark 3.4 sources | -| `spark_4_0` | merge group, **or** PR with `run-spark-4.0-tests` | Spark 4.0 sources | -| `iceberg_1_11` | PR or merge group, paths matched | Iceberg sources | -| `iceberg_1_8` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | -| `iceberg_1_9` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | -| `iceberg_1_10` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | -| `required_checks` | always, after every job above except `docs` | none (always runs) | +| Job in `ci.yml` | Triggered by | Routing rule | +| ----------------------- | ---------------------------------------------------------------------------------------------------------------------- | ----------------------------------- | +| `preflight` | every PR / merge group / push / dispatch / label | none (always runs) | +| `changes` | every PR / merge group / push / dispatch / label | runs `dev/ci/compute-changes.py` | +| `build_linux_native` | any selected Linux/Spark/Iceberg consumer | `dev/ci/compute-changes.py` | +| `pr_build_linux_checks` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | +| `pr_build_linux` | PR, merge group or push to main, paths matched | `dev/ci/compute-changes.py` | +| `pr_build_macos` | merge group, **or** PR with `run-macos-tests` | `dev/ci/compute-changes.py` | +| `pr_benchmark_check` | merge group, **or** PR with `run-benchmark-check` | benchmark sources only | +| `docs` | push to main, paths matched | `.asf.yaml`, `docs/**`, `docs.yaml` | +| `spark_3_5` | merge group, **or** PR with `run-spark-3.5-tests` | Spark 3.5 sources | +| `spark_4_1` | PR or merge group, paths matched; the `sql_hive` shards only in the merge group **or** with `run-spark-4.1-hive-tests` | Spark 4.1 sources | +| `spark_3_4` | merge group, **or** PR with `run-spark-3.4-tests` | Spark 3.4 sources | +| `spark_4_0` | merge group, **or** PR with `run-spark-4.0-tests` | Spark 4.0 sources | +| `iceberg_1_11` | PR or merge group, paths matched | Iceberg sources | +| `iceberg_1_8` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | +| `iceberg_1_9` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | +| `iceberg_1_10` | merge group, **or** PR with `run-iceberg-tests` | Iceberg sources | +| `required_checks` | always, after every job above except `docs` | none (always runs) | A heavy job appears in the PR's checks list as a `skipped` entry whenever its path filter or event criteria don't match. Skipped checks count as @@ -118,8 +118,9 @@ safe to make a required check. `ci.yml` also fires on `pull_request.types: [labeled]`, so applying `run-spark-3.4-tests`, `run-spark-3.5-tests`, `run-spark-4.0-tests`, -`run-iceberg-tests`, `run-macos-tests`, or `run-benchmark-check` starts the -jobs that label gates without needing a new push. GitHub cannot filter a +`run-spark-4.1-hive-tests`, `run-iceberg-tests`, `run-macos-tests`, or +`run-benchmark-check` starts the jobs that label gates without needing a new +push. GitHub cannot filter a `pull_request` trigger by label name, so **every** label added to a PR starts a run, including labels that gate nothing. @@ -180,15 +181,26 @@ umbrella doesn't watch, or operate independently of the rest of CI: ## Changing what runs when -Consumer jobs in `ci.yml` use a single routing output, for example: +Consumer jobs in `ci.yml` use their routing outputs, for example: ```yaml if: needs.changes.outputs.spark_3_5 == 'true' ``` -The shared native producer runs when any of its consumers' outputs is true. +Spark 4.1 has separate core and Hive outputs selecting the same caller: -That single boolean folds together two separate decisions, both of which live +```yaml +if: needs.changes.outputs.spark_4_1 == 'true' || needs.changes.outputs.spark_4_1_hive == 'true' +``` + +`NATIVE_CONSUMERS` in `dev/ci/compute-changes.py` maps each native consumer +job to all outputs that can select it. The shared native producer runs when +any of those outputs is true. A Hive-only label event has `spark_4_1=false` +and `spark_4_1_hive=true`, so it still starts the native producer. The +configuration guard checks the exact callers, output exports, dependencies, +and gates against this mapping. + +Each routing output folds together two separate decisions, both of which live in `dev/ci/compute-changes.py`: - **`FILTERS`** — which files the job covers. Pattern semantics match @@ -306,6 +318,12 @@ Any job whose first Maven use is a bare `./mvnw` needs this step before it. the composite, because a local action invoking another local action is deliberately avoided here (see the artifact-upload note above). +The independent Java lint, Spark 4.1 compile, and Celeborn compatibility jobs +in `pr_build_linux_checks.yml` each bootstrap Maven, as do the two TPC jobs +in `pr_build_linux.yml`. The configuration guard scans both workflows and +requires an earlier unconditional bootstrap step for every direct `./mvnw` +command; a bootstrap that ignores failure does not satisfy the guard. + ## Merge queue `.asf.yaml` declares a `Merge Queue` ruleset for the default branch, so `main` diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index e3c3c6d053..ad60145f72 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -51,6 +51,9 @@ # 6. Independent Linux checks. Lint, compile-only checks and debug Rust # tests must remain runnable without waiting for the native CI build. # +# 7. Direct Maven wrapper invocations in the two Linux workflows must run +# after the retrying bootstrap, including checks moved between them. +# # Run from the repository root: python3 dev/ci/check-ci-config.py import importlib.util @@ -105,7 +108,8 @@ # to nothing at all and merges having been exercised by no consumer. ([".github/actions/upload-artifact-retry/action.yaml"], BUILD_JOBS), ([".github/actions/download-artifact-retry/action.yaml"], BUILD_JOBS), - # The Maven bootstrap composite is called only from pr_build_linux.yml. + # Both Linux workflows call the Maven bootstrap composite. They share the + # build_linux route, so moving a caller between them keeps this route. ([".github/actions/maven-bootstrap/action.yaml"], {"build_linux"}), # Editing the shared Linux producer must exercise every Linux consumer. ([".github/workflows/build_linux_native.yml"], BUILD_JOBS - {"build_macos"}), @@ -259,6 +263,8 @@ "lint", "scalafix-syntactic", "lint-java", "build-spark-4-1", "celeborn-reflection-compatibility", "linux-test-rust", } +LINUX_MAVEN_WORKFLOWS = (LINUX_CHECKS_WORKFLOW, "pr_build_linux.yml") +MAVEN_BOOTSTRAP_ACTION = "./.github/actions/maven-bootstrap" def load_filters(): @@ -545,13 +551,14 @@ def native_selection_failures(jobs): """Return errors when workflow gates diverge from the Python selector. `jobs` is the ci.yml job mapping returned by block_mapping, with each value - holding an inline scalar and indented body. Read the selector's consumer - keys and require a matching caller, direct output export, and simple gate - for each one, plus the producer. This checks wiring without evaluating YAML - expressions. Inputs and files are not mutated; import/read errors propagate. + holding an inline scalar and indented body. The selector maps actual caller + IDs to tuples of selection outputs; each caller must use exactly their OR + (a single comparison for one output), and export every output directly. + For example, spark_4_1 accepts either its core or Hive output, while the + producer has one derived output. Expressions are compared, not evaluated. + Inputs and files are not mutated; import/read errors propagate. """ - consumers = {"pr_build_linux" if key == "build_linux" else key: key - for key in load_filters().NATIVE_CONSUMERS} + consumers = load_filters().NATIVE_CONSUMERS actual_consumers = { job_id for job_id, (_, body) in jobs.items() if scalar(block_mapping(body, 4).get("uses", ("", ""))[0]) @@ -563,17 +570,72 @@ def native_selection_failures(jobs): "in compute-changes.py") changes = block_mapping(jobs.get("changes", ("", ""))[1], 4) outputs = block_mapping(changes.get("outputs", ("", ""))[1], 6) - for job_id, output in {SHARED_NATIVE_JOB: SHARED_NATIVE_JOB, **consumers}.items(): + selections = {SHARED_NATIVE_JOB: (SHARED_NATIVE_JOB,), **consumers} + for job_id, routes in selections.items(): fields = block_mapping(jobs.get(job_id, ("", ""))[1], 4) - if fields.get("if", ("", ""))[0] != f"needs.changes.outputs.{output} == 'true'": - failures.append(f"ci.yml: {job_id} must select only changes.outputs.{output}") - if outputs.get(output, ("", ""))[0] != f"${{{{ steps.compute.outputs.{output} }}}}": - failures.append(f"ci.yml: changes must export steps.compute.outputs.{output}") + expected = " || ".join(f"needs.changes.outputs.{output} == 'true'" for output in routes) + if fields.get("if", ("", ""))[0] != expected: + failures.append(f"ci.yml: {job_id} must select exactly {expected}") + for output in routes: + if outputs.get(output, ("", ""))[0] != f"${{{{ steps.compute.outputs.{output} }}}}": + failures.append(f"ci.yml: changes must export steps.compute.outputs.{output}") + return failures + + +def job_steps(job): + """Return ordered step mappings from a conventional workflow job body. + + `job` is the indented text from block_mapping. Only the direct `steps:` + list at six spaces is read; each result uses block_mapping's (scalar, + body) values. Replacing each list marker with spaces lets that existing + parser read step keys at eight spaces without inspecting nested actions. + This reads a string, mutates nothing, and returns an empty list if absent; + actionlint remains responsible for other YAML layouts and syntax errors. + """ + body = block_mapping(job, 4).get("steps", ("", ""))[1] + starts = list(re.finditer(r"^ - ", body, re.MULTILINE)) + return [block_mapping(" " + body[start.end(): + starts[index + 1].start() if index + 1 < len(starts) else len(body)], 8) + for index, start in enumerate(starts)] + + +def linux_maven_bootstrap_failures(workflows): + """Return errors for Linux Maven runs without a prior reliable bootstrap. + + `workflows` is a directory Path. Read only the two Linux reusable workflows + and their direct job steps; composite actions own their internal bootstrap. + Each direct ./mvnw run needs an earlier maven-bootstrap step in the same job + with no `if` and with failures propagated. Comments and non-run fields do + not count as commands. No files or mappings are mutated; missing workflows + produce errors, other file-read errors propagate, and success returns []. + """ + failures = [] + for filename in LINUX_MAVEN_WORKFLOWS: + path = workflows / filename + if not path.exists(): + failures.append(f"{path}: Linux Maven workflow is missing") + continue + jobs = block_mapping(block_mapping(path.read_text(encoding="utf-8"), 0) + .get("jobs", ("", ""))[1], 2) + for job_id, (_, body) in jobs.items(): + bootstrapped = False + for step in job_steps(body): + if (scalar(step.get("uses", ("", ""))[0]) == MAVEN_BOOTSTRAP_ACTION + and "if" not in step + and scalar(step.get("continue-on-error", ("false", ""))[0]) == "false"): + bootstrapped = True + value, script = step.get("run", ("", "")) + script = script if value in {"|", ">", "|-", ">-", "|+", ">+"} else scalar(value) + commands = "\n".join(line for line in script.splitlines() + if not line.lstrip().startswith("#")) + if re.search(r"(?