sunchao commented on code in PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4039633644
########## .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: Added in 1dc4bf318. Preflight now runs a small table-driven test of the conditions read from the action itself. It covers PR/queue library hits, library misses with an exact incremental hit, main replenishing missing or prefix-matched entries, and the save restrictions. It also covers the new optimization from the other thread: main skips Cargo when both entries hit exactly. Restoring the old build condition makes that case fail. The focused tests and local CI checks pass; hosted CI for this commit is pending. ########## .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: Updated the PR description to measure all three entries on the first main push: the finished library, CI Cargo cache, and debug Cargo cache. The effective-CARGO_HOME correction affects both Cargo profiles, so including debug is necessary to assess retention pressure. The rollout measurements now live in the PR description rather than the permanent README. Actual new-entry sizes remain pending main publication. ########## .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: Implemented in 1dc4bf318. The incremental restore condition stays unchanged; the build condition now skips Cargo on main only when both entries hit exactly. The restored CI target entry supplies libcomet.so in that case. A library miss still builds even with an exact target hit, and a target miss/prefix match still builds on main to replenish it. Updated the comments and documentation and added regression coverage for these cases. This avoids the Cargo work; it still downloads the incremental archive. ########## .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" Review Comment: The scope observation is correct. I kept the single definition because Andy explicitly requested removing the duplicated fingerprint/build literals in the previous review. These are native-producer jobs, and their remaining steps upload or package the existing library without invoking Cargo. There is no affected downstream build today. Reintroducing the two copies would reverse that agreed simplification, so I have left this unchanged and am keeping the thread open for discussion. ########## dev/ci/native-cache-key.py: ########## @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +# 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. + +"""Fingerprint the clean Linux checkout and toolchain used by Comet CI. + +Run after setup-builder and before Cargo generates source files. This helper +supports the official Rust container, setup-builder's JDK/packages, and the +build commands in our workflows; it is not a general local-build cache. +""" + +import argparse +import hashlib +import importlib.util +import json +import os +from pathlib import Path +import subprocess + + +# Share both the input patterns and their glob semantics with main's warmer. +SPEC = importlib.util.spec_from_file_location("compute_changes", Path(__file__).with_name("compute-changes.py")) +CHANGES = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CHANGES) + + +def digest(value): + return hashlib.sha256(json.dumps(value, sort_keys=True).encode()).hexdigest() + + +def command(args, cwd): + return subprocess.check_output(args, cwd=cwd, text=True).strip() + + +def source_inputs(root, profile="ci"): + """Return dependency and source maps for the selected native build profile. + + Each map contains relative names, Git modes and content digests. Untracked + generated Rust, target files and documentation are excluded. CI library + builds omit benchmarks; debug checks compile them. Trust only this checkout + for the Git read: container steps can run as a different owner than checkout. + """ + patterns = CHANGES.NATIVE_LIBRARY_INPUTS if profile == "ci" else CHANGES.NATIVE_BUILD_INPUTS + inventory = command(["git", "-c", f"safe.directory={root}", + "ls-files", "--stage", "-z"], root) + sources = {} + for record in inventory.split("\0"): + if not record: + continue + metadata, name = record.split("\t", 1) + if CHANGES.matches(patterns, [name]): + sources[name] = [metadata.split()[0], Review Comment: The index-OID approach is reasonable under the clean-checkout contract. I have kept this part unchanged in the focused revision: hashing working-tree bytes directly describes the files Cargo sees, and the measured 0.15s does not justify changing that behavior or adding a matcher API here. The dependency comprehension is also small and readable. I did simplify the separate routing test so it no longer scans and hashes the repository inventory. Leaving this thread open since the helper refactor is deferred. ########## dev/ci/native-cache-key.py: ########## @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +# 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. + +"""Fingerprint the clean Linux checkout and toolchain used by Comet CI. + +Run after setup-builder and before Cargo generates source files. This helper +supports the official Rust container, setup-builder's JDK/packages, and the +build commands in our workflows; it is not a general local-build cache. +""" + +import argparse +import hashlib +import importlib.util +import json +import os +from pathlib import Path +import subprocess + + +# Share both the input patterns and their glob semantics with main's warmer. +SPEC = importlib.util.spec_from_file_location("compute_changes", Path(__file__).with_name("compute-changes.py")) +CHANGES = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CHANGES) + + +def digest(value): + return hashlib.sha256(json.dumps(value, sort_keys=True).encode()).hexdigest() + + +def command(args, cwd): + return subprocess.check_output(args, cwd=cwd, text=True).strip() + + +def source_inputs(root, profile="ci"): + """Return dependency and source maps for the selected native build profile. + + Each map contains relative names, Git modes and content digests. Untracked + generated Rust, target files and documentation are excluded. CI library + builds omit benchmarks; debug checks compile them. Trust only this checkout + for the Git read: container steps can run as a different owner than checkout. + """ + patterns = CHANGES.NATIVE_LIBRARY_INPUTS if profile == "ci" else CHANGES.NATIVE_BUILD_INPUTS + inventory = command(["git", "-c", f"safe.directory={root}", + "ls-files", "--stage", "-z"], root) + sources = {} + for record in inventory.split("\0"): + if not record: + continue + metadata, name = record.split("\t", 1) + if CHANGES.matches(patterns, [name]): + sources[name] = [metadata.split()[0], + hashlib.sha256((root / name).read_bytes()).hexdigest()] + dependencies = {name: value for name, value in sources.items() + if Path(name).name in {"Cargo.toml", "Cargo.lock"}} + return dependencies, sources + + +def environment_inputs(root, env): + """Identify the official tools installed by setup-builder without modifying them. + + Rust's versions include the compiler commit; dpkg identifies the installed + C/C++/protobuf tools and system libraries. The JDK release file identifies + the vendor/build supplying JNI headers and libjvm. Record build overrides, + including target-qualified cc variables and HDFS linking options, without + including unrelated per-run GitHub variables. The shared setup/build actions + are hashed separately; caller test configuration does not affect the library. + """ + java_home = Path(env["JAVA_HOME"]) + return { + "workspace": str(root), + "architecture": command(["uname", "-m"], root), + "rust": {tool: command([tool, flag], root / "native") + for tool, flag in (("rustc", "-vV"), ("cargo", "--version"), + ("rustfmt", "--version"))}, + "packages": sorted(command(["dpkg-query", "-W", Review Comment: Unrelated installed-package changes can indeed cause conservative misses. I retained the current boundary because the suggested package list does not cover the full compiler dependency set. In the official Rust image I inspected, GCC's cc1 also links libisl23, libmpc3, libmpfr6, libgmp10, zlib1g and libzstd1. GCC constrains these with minimum versions, so they can change while the proposed whitelisted package versions remain unchanged. Narrowing this safely needs the complete set of relevant toolchain dependencies. Also, apt-get update alone changes repository indexes, not the installed versions this helper hashes. I retained the JDK identity too: default HDFS builds against its JNI headers and links libjvm. Removing java_release alone would still leave patch-specific JAVA_HOME and PATH values in the key. The PR's rollout plan now explicitly calls for tracking package/JDK changes behind misses, alongside sizes, retention and hit behavior. Leaving this open rather than claiming the proposed narrowing is implemented. ########## dev/ci/native-cache-key.py: ########## @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +# 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. + +"""Fingerprint the clean Linux checkout and toolchain used by Comet CI. + +Run after setup-builder and before Cargo generates source files. This helper +supports the official Rust container, setup-builder's JDK/packages, and the +build commands in our workflows; it is not a general local-build cache. +""" + +import argparse +import hashlib +import importlib.util +import json +import os +from pathlib import Path +import subprocess + + +# Share both the input patterns and their glob semantics with main's warmer. +SPEC = importlib.util.spec_from_file_location("compute_changes", Path(__file__).with_name("compute-changes.py")) +CHANGES = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CHANGES) + + +def digest(value): + return hashlib.sha256(json.dumps(value, sort_keys=True).encode()).hexdigest() + + +def command(args, cwd): + return subprocess.check_output(args, cwd=cwd, text=True).strip() + + +def source_inputs(root, profile="ci"): + """Return dependency and source maps for the selected native build profile. + + Each map contains relative names, Git modes and content digests. Untracked + generated Rust, target files and documentation are excluded. CI library + builds omit benchmarks; debug checks compile them. Trust only this checkout + for the Git read: container steps can run as a different owner than checkout. + """ + patterns = CHANGES.NATIVE_LIBRARY_INPUTS if profile == "ci" else CHANGES.NATIVE_BUILD_INPUTS + inventory = command(["git", "-c", f"safe.directory={root}", + "ls-files", "--stage", "-z"], root) + sources = {} + for record in inventory.split("\0"): + if not record: + continue + metadata, name = record.split("\t", 1) + if CHANGES.matches(patterns, [name]): + sources[name] = [metadata.split()[0], + hashlib.sha256((root / name).read_bytes()).hexdigest()] + dependencies = {name: value for name, value in sources.items() + if Path(name).name in {"Cargo.toml", "Cargo.lock"}} + return dependencies, sources + + +def environment_inputs(root, env): + """Identify the official tools installed by setup-builder without modifying them. + + Rust's versions include the compiler commit; dpkg identifies the installed + C/C++/protobuf tools and system libraries. The JDK release file identifies + the vendor/build supplying JNI headers and libjvm. Record build overrides, + including target-qualified cc variables and HDFS linking options, without + including unrelated per-run GitHub variables. The shared setup/build actions + are hashed separately; caller test configuration does not affect the library. + """ + java_home = Path(env["JAVA_HOME"]) + return { + "workspace": str(root), + "architecture": command(["uname", "-m"], root), + "rust": {tool: command([tool, flag], root / "native") + for tool, flag in (("rustc", "-vV"), ("cargo", "--version"), + ("rustfmt", "--version"))}, + "packages": sorted(command(["dpkg-query", "-W", + "-f=${binary:Package}\t${Version}\t${Architecture}\n"], root).splitlines()), + "java_home": str(java_home), + "java_release": (java_home / "release").read_text(), + "cargo_home": env.get("CARGO_HOME", str(Path.home() / ".cargo")), + "env": {name: value for name, value in env.items() + if name.startswith(("CARGO_", "RUST", "HOST_", "TARGET_", "HDFS_")) + or name.split("_", 1)[0] in {"CC", "CXX", "CFLAGS", "CXXFLAGS", "CXXSTDLIB", + "LDFLAGS", "AR", "ARFLAGS", "RANLIB", "RANLIBFLAGS", "PROTOC"} + or name in {"JAVA_HOME", "PATH", "HADOOP_HOME", "DOCS_RS", + "CRATE_CC_NO_DEFAULTS", "CROSS_COMPILE"}}, + } + + +def cache_keys(profile, dependencies, sources, environment): + """Return output keys for one pre-build snapshot. + + Only the incremental Cargo cache has a source-independent restore prefix. + The library key includes all tracked build inputs and never uses fallback. + Both retain the environment: native build scripts can reuse C objects + without detecting changes to external compiler binaries or JNI headers. + """ + prefix = f"Linux-cargo-{profile}-v3-{digest([environment, dependencies])}-" + return { + "cargo-home": environment["cargo_home"], + "source-key": prefix + digest(sources), + "restore-prefix": prefix, + "binary-key": f"Linux-native-ci-v2-{digest([environment, sources])}" if profile == "ci" else "", Review Comment: Omitting the unused output is possible cleanup, but it would not add the proposed failure behavior: GitHub evaluates a nonexistent context property to an empty string, so steps.key.outputs.binary-key behaves the same either way. See the [context-property documentation](https://docs.github.com/en/actions/reference/workflows-and-actions/contexts). I kept the uniform output mapping without adding a profile-specific branch; the action explicitly requests the CI profile. Leaving this unchanged. ########## dev/ci/compute-changes.py: ########## @@ -388,6 +405,14 @@ "mvnw", ], } +# These inputs are shared by the Linux native producers. Keep the routes in +# one place so an action-only cache change exercises each applicable consumer. +for _native_consumer in ( + "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", +): + FILTERS[_native_consumer].extend(NATIVE_CACHE_RECIPES) Review Comment: Done in 1dc4bf318. Linux lists the build-native-ci action directly and relies on its existing dev/ci/** route for the helpers. The shared recipe loop now covers the Spark/Iceberg consumers. CI routing checks pass with the same selected jobs. -- 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]
