viirya commented on code in PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4041695105
########## .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: Thanks, this addresses my coverage request. Reading the conditions from the action itself protects the actual build/save decisions, and the twelve scenarios pass locally. The revised main double-hit behavior is also sound: the target restore supplies the library, so Cargo does not need to run when neither entry needs replenishing. My earlier suggestion to assert that main always builds is superseded by this optimization; the new expectation is the right one. This thread can be resolved. ########## .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: Addressed, thanks. The rollout plan now explicitly includes the finished library and both CI/debug target caches, and moving these one-time measurements into the PR description keeps the README focused on the lasting contract. With the target-only update, my original concern about adding registry/git contents to the debug entry no longer applies. Measuring all three entries remains useful for verifying retention during the namespace migration. This thread can be resolved; the measurements themselves remain part of the rollout. -- 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]
