comphead commented on code in PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4039086522
########## .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: **Major.** On a main push whose native inputs are unchanged, both keys hit exactly, both save steps are skipped, and Cargo still runs over a fully restored `native/target`. That is the common case, not the edge one: `FILTERS["build_linux"]` routes on `spark/**`, `common/**`, `pom.xml` and `dev/ci/**`, so most merges reach this job without touching a single native input. The library the job would rebuild is already published under the same key, so the run restores a multi-GB entry and re-verifies a workspace to produce nothing. Narrowing the condition keeps main compiling only when it has something to write: ```yaml if: steps.binary-cache.outputs.cache-hit != 'true' || (github.event_name == 'push' && github.ref == 'refs/heads/main' && steps.cargo-cache.outputs.cache-hit != 'true') ``` `native/target/ci/libcomet.so` still comes from the restored incremental entry in that case, so the artifact upload is unaffected. Leaving the restore step's condition as it is keeps the incremental entry available whenever a build can still happen. ########## 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: Three things collapse here. 1. `git ls-files --stage` already emits the content hash. `metadata.split()` is `[mode, oid, stage]`, and this keeps `[0]` and throws away `[1]`, then re-reads and SHA-256s every file. Keying on `mode + oid` removes all the file I/O and the `FileNotFoundError` that an index entry with no worktree file would raise. The docstring already scopes this to a clean checkout, and untracked generated files never appear in `ls-files`, so hashing the working tree buys nothing the index does not already give. 2. `dependencies` on L68 is a second pass over the dict just built. Both maps fit in the one loop. 3. `CHANGES.matches()` is a whole-changeset predicate being invoked once per file, so it rebuilds the compiled include/exclude lists on every call. Sharing the semantics with `compute-changes.py` is the right instinct, but the reusable unit is the matcher, not the any-file wrapper. A `compile_matcher(patterns)` there that returns a predicate, with `matches()` calling it too, gives the same guarantee without the per-file rebuild. For calibration, I measured this on the current tree: 309 files and 5.2 MB, 0.15s. So this is about single-traversal clarity, not runtime. ########## .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: This swaps a step-level `env:` for a job-scoped one. The value is right: all four callers declare a workflow-level `RUSTFLAGS: "-Clink-arg=-fuse-ld=bfd"`, and `GITHUB_ENV` does win over it, because the runner evaluates workflow and job `env` once into the same dictionary that `GITHUB_ENV` later writes to (`JobExtension.InitializeJob` populating `Global.EnvironmentVariables`, then `StepsRunner` seeding each step's env context from it). What changes is scope. `-Ctarget-cpu=x86-64-v3` now applies to every later step in the caller's job, where the old step-level `env:` applied to the one `cargo build`. Nothing downstream invokes Cargo today, so this is not a bug, but it is an unflagged widening. Dropping this step and putting `env: RUSTFLAGS: ...` on the fingerprint and build steps restores the old scope and removes a step. ########## 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: Two of the three recipes are already routed here. `FILTERS["build_linux"]` contains `dev/ci/**`, which covers both `dev/ci/native-cache-key.py` and `dev/ci/compute-changes.py` (I confirmed the same for `build_macos`). Only `.github/actions/build-native-ci/**` is a new route for `build_linux`. Dropping `build_linux` from the loop and adding the action pattern to its filter directly makes it visible which route the PR actually adds, and keeps the loop meaning "the consumers that did not already have these". ########## 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: **Major.** The library key takes the entire dpkg database. `amd64/rust` is an unpinned `latest` tag that the runner re-pulls per job, and `setup-builder` runs `apt-get update` before installing, so any archive refresh discards a finished library over packages that cannot affect `libcomet.so`: `tzdata`, `ca-certificates`, `git`, `imagemagick` and the rest of the `buildpack-deps` base. The stated rationale is the C/JNI boundary in `hdfs-sys` and `core/build.rs`, and that needs only the toolchain: `clang*`, `gcc*`, `binutils`, `libc6-dev`, `libstdc++*`, `protobuf-compiler`. Restricting `dpkg-query -W` to those keeps the invariant you actually depend on and materially extends how long an entry stays usable, which is the thing this PR is buying. Worth folding into the hit-rate measurement you already planned rather than deferring it, since it decides whether the reuse pays off at all. Same question, smaller, for `java_release` on L93: the full `release` file rotates on every Zulu 17 patch, while the JNI headers it stands in for essentially never change. ########## dev/ci/compute-changes.py: ########## @@ -546,11 +571,16 @@ def event_allows(job, event): def compute(files, event): - """Return {job: bool}, folding the path filter and the event policy.""" - return { + """Return job flags, including main's warmer for shared native cache inputs.""" + selected = { name: event_allows(name, event) and matches(patterns, files) for name, patterns in FILTERS.items() } + # Use the fingerprint's exact patterns and matcher for main's producer, + # including inputs owned by other workflows, without broadening PR jobs. + if event.get("name") == "push" and matches(NATIVE_LIBRARY_INPUTS, files): Review Comment: This encodes the push tier outside `POLICY`. `POLICY["build_linux"]` already lists `"push"`, so the two agree today, but if `"push"` is ever dropped there this line silently puts it back, which is exactly the drift `check-ci-config.py` exists to prevent. Threading it through the same gate keeps `POLICY` authoritative without widening anything: ```python if (event.get("name") == "push" and event_allows("build_linux", event) and matches(NATIVE_LIBRARY_INPUTS, files)): selected["build_linux"] = True ``` ########## 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: `binary-key=""` is written to `$GITHUB_OUTPUT` for the debug profile and no caller reads it. Omitting the entry when `profile != "ci"` makes a caller that passes the wrong profile fail on a missing output instead of silently keying a cache on an empty string. ########## .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 Review Comment: This paragraph is a one-off verification checklist for this change rather than a description of how the caches work. "Report the compressed cache sizes in bytes" and "verify a hosted library hit by checking that `Restore native library cache` reports an exact hit" are PR-body or follow-up-issue items, and they will read as stale instructions once the namespaces are populated. More broadly the section runs 58 lines and restates much of the PR description. The durable contract is short: what is fingerprinted, who writes and who consumes, what invalidates, and that `--locked` means a manifest change must ship with its `native/Cargo.lock` update. L451-455 in particular ("GitHub Actions handles cache storage and restoration") restates the obvious, and L438-442 narrate the size change rather than stating the resulting constraint on the shared cache budget. ########## dev/ci/test-native-cache-key.py: ########## @@ -0,0 +1,206 @@ +#!/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. + +"""Check native cache boundaries and container checkout ownership with real Git.""" + +import importlib.util +import io +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + + +SPEC = importlib.util.spec_from_file_location("native_cache_key", Path(__file__).with_name("native-cache-key.py")) +CACHE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CACHE) + + +class NativeCacheKeyTests(unittest.TestCase): + """Use disposable Git repositories and mock only installed tool versions.""" + + def setUp(self): + """Create tracked native/JVM fixtures and JDK metadata; clean up after each test.""" + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + self.root = Path(temporary.name) + subprocess.run(["git", "init", "--quiet", str(self.root)], check=True) + self.inputs = {"native/Cargo.toml": "[workspace]\n", "native/Cargo.lock": "version = 4\n", + "native/lib.rs": "fn native() {}\n", "native/proto/expr.proto": "message Expr {}\n", + "spark/Plan.scala": "object Plan {}\n", "README.md": "Comet\n", + ".github/workflows/README.md": "CI documentation\n", + ".github/workflows/pr_build_linux.yml": "jobs: {}\n", + ".github/workflows/spark_sql_test_reusable.yml": "jobs: {}\n", + ".github/workflows/iceberg_spark_test_reusable.yml": "jobs: {}\n", + ".github/workflows/spark_sql_writer_tests.yml": "jobs: {}\n", + ".github/workflows/check_pr_title.yml": "jobs: {}\n", + ".github/actions/build-native-ci/action.yaml": "runs: {}\n", + ".github/actions/setup-builder/action.yaml": "runs: {}\n", + "dev/ci/compute-changes.py": "# shared native input rules\n", + "contrib/delta/native/Cargo.toml": '[package]\nname = "delta"\n', + "contrib/delta/native/src/lib.rs": "fn delta() {}\n", + "contrib/delta/native/Cargo.lock": "version = 4\n", + "contrib/a/b/native/Cargo.toml": '[package]\nname = "nested"\n', + "native/core/benches/perf.rs": "fn benchmark() {}\n"} + for name, content in self.inputs.items(): + self.write(name, content) + subprocess.run(["git", "add", "."], cwd=self.root, check=True) + self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n') + self.env = {"JAVA_HOME": str(self.root / "jdk"), "CARGO_HOME": str(self.root / "cargo"), + "RUSTFLAGS": "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd"} + self.versions = {"rustc": "rustc 1.90\nhost: x86_64-unknown-linux-gnu\n", + "cargo": "cargo 1.90\n", "rustfmt": "rustfmt 1.8\n", + "dpkg-query": "libc6\t2.40\tamd64\n", "uname": "x86_64\n"} + + def write(self, name, content): + """Write fixture text under the temporary repository, creating its parents.""" + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + + def keys(self, profile="ci"): + """Return keys from real tracked files and deterministic tool version responses.""" + dependencies, sources = CACHE.source_inputs(self.root, profile) + with patch.object(CACHE, "command", side_effect=lambda args, cwd: self.versions[args[0]]): + environment = CACHE.environment_inputs(self.root, self.env) + return CACHE.cache_keys(profile, dependencies, sources, environment) + + def test_source_and_dependency_changes_invalidate_the_right_keys(self): + """Native/protobuf edits retain the dependency prefix; dependency edits replace it.""" + before = self.keys() + for name in ("native/lib.rs", "native/proto/expr.proto", "native/Cargo.toml", "native/Cargo.lock", + "contrib/delta/native/Cargo.toml", "dev/ci/compute-changes.py", + ".github/actions/build-native-ci/action.yaml", ".github/actions/setup-builder/action.yaml"): + with self.subTest(name=name): + self.write(name, self.inputs[name] + "changed\n") + after = self.keys() + self.assertNotEqual(before["source-key"], after["source-key"]) + self.assertNotEqual(before["binary-key"], after["binary-key"]) + if name.endswith(("Cargo.toml", "Cargo.lock")): + self.assertNotEqual(before["restore-prefix"], after["restore-prefix"]) + else: + self.assertEqual(before["restore-prefix"], after["restore-prefix"]) + self.write(name, self.inputs[name]) + + def test_generated_files_and_unrelated_jvm_edits_preserve_keys(self): + """Generated files and non-build edits preserve reuse; debug still tracks benchmarks.""" + before = self.keys() + debug = self.keys("debug") + for name in self.inputs: + if name.startswith(".github/workflows/"): + self.write(name, "unrelated test configuration\n") + self.env["GITHUB_RUN_ID"] = "12345" + self.assertEqual(before, self.keys()) + self.assertEqual(debug, self.keys("debug")) + self.write("native/proto/src/generated/expr.rs", "generated Rust") + self.write("native/target/ci/libcomet.so", "compiled library") + self.write("spark/Plan.scala", "object NewPlan {}") + self.write("README.md", "updated docs") + self.write("contrib/delta/native/src/lib.rs", "fn changed_delta() {}") + self.write("contrib/delta/native/Cargo.lock", "version = 3\n") + self.write("contrib/a/b/native/Cargo.toml", '[package]\nname = "changed_nested"\n') + self.write("native/core/benches/perf.rs", "fn changed_benchmark() {}") + self.assertEqual(before, self.keys()) + self.assertNotEqual(debug["source-key"], self.keys("debug")["source-key"]) + + def test_native_input_routing(self): + """Library inputs warm main; helper tests retain Linux coverage without extra consumers.""" + project = Path(__file__).resolve().parents[2] + route = CACHE.CHANGES.compute + _, sources = CACHE.source_inputs(project) Review Comment: This re-derives the real repository inventory to assert the same routing once per file: 309 files and 5.2 MB hashed on the current tree, all proving one thing. The invariant is "every pattern in `NATIVE_LIBRARY_INPUTS` warms main". Iterating the patterns with one representative path each proves it directly, needs no repo read, and does not silently lose coverage when a pattern stops matching any file (the loop would just get shorter and stay green). The three literals already appended on L129 are exactly that style. ########## dev/ci/test-native-cache-key.py: ########## @@ -0,0 +1,206 @@ +#!/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. + +"""Check native cache boundaries and container checkout ownership with real Git.""" + +import importlib.util +import io +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + + +SPEC = importlib.util.spec_from_file_location("native_cache_key", Path(__file__).with_name("native-cache-key.py")) +CACHE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(CACHE) + + +class NativeCacheKeyTests(unittest.TestCase): + """Use disposable Git repositories and mock only installed tool versions.""" + + def setUp(self): + """Create tracked native/JVM fixtures and JDK metadata; clean up after each test.""" + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + self.root = Path(temporary.name) + subprocess.run(["git", "init", "--quiet", str(self.root)], check=True) + self.inputs = {"native/Cargo.toml": "[workspace]\n", "native/Cargo.lock": "version = 4\n", + "native/lib.rs": "fn native() {}\n", "native/proto/expr.proto": "message Expr {}\n", + "spark/Plan.scala": "object Plan {}\n", "README.md": "Comet\n", + ".github/workflows/README.md": "CI documentation\n", + ".github/workflows/pr_build_linux.yml": "jobs: {}\n", + ".github/workflows/spark_sql_test_reusable.yml": "jobs: {}\n", + ".github/workflows/iceberg_spark_test_reusable.yml": "jobs: {}\n", + ".github/workflows/spark_sql_writer_tests.yml": "jobs: {}\n", + ".github/workflows/check_pr_title.yml": "jobs: {}\n", + ".github/actions/build-native-ci/action.yaml": "runs: {}\n", + ".github/actions/setup-builder/action.yaml": "runs: {}\n", + "dev/ci/compute-changes.py": "# shared native input rules\n", + "contrib/delta/native/Cargo.toml": '[package]\nname = "delta"\n', + "contrib/delta/native/src/lib.rs": "fn delta() {}\n", + "contrib/delta/native/Cargo.lock": "version = 4\n", + "contrib/a/b/native/Cargo.toml": '[package]\nname = "nested"\n', + "native/core/benches/perf.rs": "fn benchmark() {}\n"} + for name, content in self.inputs.items(): + self.write(name, content) + subprocess.run(["git", "add", "."], cwd=self.root, check=True) + self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n') + self.env = {"JAVA_HOME": str(self.root / "jdk"), "CARGO_HOME": str(self.root / "cargo"), + "RUSTFLAGS": "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd"} + self.versions = {"rustc": "rustc 1.90\nhost: x86_64-unknown-linux-gnu\n", + "cargo": "cargo 1.90\n", "rustfmt": "rustfmt 1.8\n", + "dpkg-query": "libc6\t2.40\tamd64\n", "uname": "x86_64\n"} + + def write(self, name, content): + """Write fixture text under the temporary repository, creating its parents.""" + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + + def keys(self, profile="ci"): + """Return keys from real tracked files and deterministic tool version responses.""" + dependencies, sources = CACHE.source_inputs(self.root, profile) + with patch.object(CACHE, "command", side_effect=lambda args, cwd: self.versions[args[0]]): + environment = CACHE.environment_inputs(self.root, self.env) + return CACHE.cache_keys(profile, dependencies, sources, environment) + + def test_source_and_dependency_changes_invalidate_the_right_keys(self): + """Native/protobuf edits retain the dependency prefix; dependency edits replace it.""" + before = self.keys() + for name in ("native/lib.rs", "native/proto/expr.proto", "native/Cargo.toml", "native/Cargo.lock", + "contrib/delta/native/Cargo.toml", "dev/ci/compute-changes.py", + ".github/actions/build-native-ci/action.yaml", ".github/actions/setup-builder/action.yaml"): + with self.subTest(name=name): + self.write(name, self.inputs[name] + "changed\n") + after = self.keys() + self.assertNotEqual(before["source-key"], after["source-key"]) + self.assertNotEqual(before["binary-key"], after["binary-key"]) + if name.endswith(("Cargo.toml", "Cargo.lock")): + self.assertNotEqual(before["restore-prefix"], after["restore-prefix"]) + else: + self.assertEqual(before["restore-prefix"], after["restore-prefix"]) + self.write(name, self.inputs[name]) + + def test_generated_files_and_unrelated_jvm_edits_preserve_keys(self): + """Generated files and non-build edits preserve reuse; debug still tracks benchmarks.""" + before = self.keys() + debug = self.keys("debug") + for name in self.inputs: + if name.startswith(".github/workflows/"): + self.write(name, "unrelated test configuration\n") + self.env["GITHUB_RUN_ID"] = "12345" + self.assertEqual(before, self.keys()) + self.assertEqual(debug, self.keys("debug")) + self.write("native/proto/src/generated/expr.rs", "generated Rust") + self.write("native/target/ci/libcomet.so", "compiled library") + self.write("spark/Plan.scala", "object NewPlan {}") + self.write("README.md", "updated docs") + self.write("contrib/delta/native/src/lib.rs", "fn changed_delta() {}") + self.write("contrib/delta/native/Cargo.lock", "version = 3\n") + self.write("contrib/a/b/native/Cargo.toml", '[package]\nname = "changed_nested"\n') + self.write("native/core/benches/perf.rs", "fn changed_benchmark() {}") + self.assertEqual(before, self.keys()) + self.assertNotEqual(debug["source-key"], self.keys("debug")["source-key"]) + + def test_native_input_routing(self): + """Library inputs warm main; helper tests retain Linux coverage without extra consumers.""" + project = Path(__file__).resolve().parents[2] + route = CACHE.CHANGES.compute + _, sources = CACHE.source_inputs(project) + for name in [*sources, ".cargo/config.toml", "rust-toolchain", "contrib/new/native/Cargo.toml"]: + self.assertTrue(route([name], {"name": "push"})["build_linux"], name) + for name in ("contrib/delta/native/src/lib.rs", "contrib/delta/native/Cargo.lock", + "contrib/a/b/native/Cargo.toml", "contrib/a/b/native/x.rs"): + self.assertFalse(route([name], {"name": "push"})["build_linux"], name) + self.assertFalse(route(["contrib/new/native/Cargo.toml"], {"name": "pull_request"})["build_linux"]) + for event in ("merge_group", "schedule"): + routed = route(["dev/ci/test-native-cache-key.py"], {"name": event}) + self.assertTrue(routed["build_linux" if event == "merge_group" else "build_linux_all_profiles"]) + self.assertFalse(any(selected for name, selected in routed.items() + if name.startswith(("spark_", "iceberg_")))) + + def test_tools_jdk_flags_and_tracked_build_configuration_invalidate(self): + """Observed tool/package versions, Java metadata, flags and tracked configs enter keys.""" + before = self.keys() + for tool in self.versions: + with self.subTest(tool=tool): + old = self.versions[tool] + self.versions[tool] += "changed\n" + self.assertNotEqual(before["source-key"], self.keys()["source-key"]) + self.assertNotEqual(before["binary-key"], self.keys()["binary-key"]) + self.assertNotEqual(before["restore-prefix"], self.keys()["restore-prefix"]) + self.versions[tool] = old + self.write("jdk/release", 'JAVA_VERSION="17.0.2"\n') + self.assertNotEqual(before["binary-key"], self.keys()["binary-key"]) + self.write("jdk/release", 'JAVA_VERSION="17.0.1"\n') + self.env["RUSTFLAGS"] += " -Copt-level=1" + self.assertNotEqual(before["binary-key"], self.keys()["binary-key"]) + self.env["RUSTFLAGS"] = "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" + for name in ("CC", "CXX", "CFLAGS", "LDFLAGS", "AR", "PROTOC", "PROTOC_INCLUDE", Review Comment: All three keys digest `environment` wholesale, so this loop proves one property 57 times (19 names times 3 keys). The invariant genuinely at risk is *which* names the `startswith`/`split` predicate in `environment_inputs` selects, and nothing asserts that: a future edit that drops `TARGET_` or narrows the `CC`/`CXX` set would keep every one of these assertions green as long as the name happens to still match. One `assertEqual` on `sorted(environment_inputs(...)["env"])` against the expected set, plus a single representative invalidation check, tests the predicate itself and shrinks the test. Same shape at L96: `binary-key` differing follows from `source-key` differing, since both digest `sources`. The real distinction in that test is prefix-stable vs prefix-changing, which L97-100 already make. -- 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]
