viirya commented on code in PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4039014919
########## .github/actions/build-native-ci/action.yaml: ########## @@ -0,0 +1,81 @@ +# 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: Build or restore the Linux CI native library +description: 'Reuse an exact-input native library, otherwise build it with the CI profile' +runs: + using: composite + steps: + - name: Pin native build flags + shell: bash + run: echo 'RUSTFLAGS=-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd' >> "$GITHUB_ENV" + + # Call after checkout and setup-builder. Compute once, before Cargo writes + # generated Rust files, and use the same keys for both restore and save. + - name: Fingerprint native build inputs + id: key + shell: bash + run: python3 dev/ci/native-cache-key.py --profile ci --github-output "$GITHUB_OUTPUT" + + - name: Restore native library cache + id: binary-cache + uses: actions/cache/restore@v6 + with: + path: native/target/ci/libcomet.so + key: ${{ steps.key.outputs.binary-key }} + # Main still builds to keep its incremental Cargo cache warm. Lookup + # only avoids downloading a library that this run will not execute. + lookup-only: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }} + + - name: Restore incremental Cargo cache + id: cargo-cache + if: steps.binary-cache.outputs.cache-hit != 'true' || (github.event_name == 'push' && github.ref == 'refs/heads/main') + uses: actions/cache/restore@v6 + with: + path: | + ${{ steps.key.outputs.cargo-home }}/registry + ${{ steps.key.outputs.cargo-home }}/git + native/target + key: ${{ steps.key.outputs.source-key }} + restore-keys: ${{ steps.key.outputs.restore-prefix }} + + - name: Build native library (CI profile) + if: steps.binary-cache.outputs.cache-hit != 'true' || (github.event_name == 'push' && github.ref == 'refs/heads/main') Review Comment: Non-blocking: could we add regression coverage for the action’s hit/miss control flow? The new tests cover fingerprinting and routing well, but do not exercise the conditions that decide whether compilation runs. The cases worth protecting are: a PR library hit skips Cargo; a library miss still runs Cargo even with an exact incremental-cache hit; main still runs Cargo after a lookup-only library hit; and PR/merge-queue runs never save either cache. The conditions look correct today, but this is the boundary where a future regression could make downstream tests use a library without the intended validation. ########## .github/workflows/README.md: ########## @@ -404,6 +404,64 @@ entry through `restore-keys` and downloads whatever else it needs, which is what a cold pull request already did. See the push-tier discussion above for which jobs do run on main and therefore do write. +## Reusing Linux native builds + +The Linux, Spark SQL, Iceberg and manual writer workflows call +`.github/actions/build-native-ci` after checkout and `setup-builder`. An exact +cache hit restores `native/target/ci/libcomet.so` and skips Cargo. A miss restores +an incremental cache and runs `cargo build --locked --profile ci`. Artifacts and +downstream tests use the same paths in either case. +`--locked` deliberately fails when a manifest change requires updating +`native/Cargo.lock`; contributors must commit that lockfile update with the change. + +`dev/ci/native-cache-key.py` snapshots tracked native/protobuf/dependency files, +Cargo configuration and the native build recipes before Cargo generates source +files. The key also includes Rust versions, installed system package +versions, architecture, JDK release/path and the build environment: Cargo/Rust +settings, C/C++ compiler and flag overrides (including target-specific variants), +and the HDFS library overrides used by the default dependencies. The helper targets +our official Rust container and `setup-builder`. Adding external tools or files +requires updating this contract; recording an override's path does not identify +arbitrary contents stored there. + +The shared build and setup actions are fingerprinted; the four caller workflows +are not. Their selected Rust/JDK versions and build environment are observed +directly, so editing a test matrix or shard does not force a native rebuild. +Spark-only edits, documentation and generated files also preserve the key; +native/protobuf changes invalidate it. Optional contrib crates contribute +their manifests, which Cargo resolves even with their features disabled, but not +their Rust sources or standalone lockfiles. Benchmarks enter the debug cache key +but not the library key. The input lists and glob matcher are shared with main's +cache routing in `compute-changes.py`. The shared action uses portable `x86-64-v3` +code generation. + +The incremental cache contains the effective `CARGO_HOME` registry/git directories +and `native/target`. In the Rust container, correcting `~/.cargo` to +`/usr/local/cargo` adds the registry and Git checkouts that the old entry did not +contain. The incremental entry therefore grows alongside the addition of the +separate finished-library entry. +Its dependency prefix permits reuse after source changes +within the same build environment, but every restore still invokes Cargo. +Environment changes also invalidate this fallback: native dependencies compile C +against JNI headers and cache build-script outputs that Cargo does not fully +invalidate after external compiler or JDK changes. This can miss after unrelated +package updates, but prevents reusing those objects under a new library key. +The Rust test job uses a separate debug key and continues to run all checks and tests. + +Only pushes to `main` save either cache. Main always compiles to keep the +incremental cache warm. Other runs consume matching entries; a cold or evicted +cache builds normally. Changes to shared native inputs owned by other workflows +also trigger main's cache warmer. GitHub Actions handles cache storage and +restoration. + +Preflight tests key invalidation, generated-file stability, container checkout +ownership, and that every binary-key input triggers main's cache warmer. On the +first main push that populates these namespaces, report the compressed cache +sizes in bytes for both the finished library and the incremental Cargo entry, Review Comment: Non-blocking: could the first-main measurements include the debug Cargo entry as well as the finished library and CI Cargo entry? `linux-test-rust` also switches to the effective `CARGO_HOME`, so its cache gains registry/git contents too. At review time, main’s existing debug entry was approximately 4.33 GB, compared with 1.51 GB for the CI entry. Measuring all three new entries would give us a more complete view of retention pressure when validating the cross-run library hit described here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
