comphead commented on code in PR #5976:
URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4087916461
##########
dev/ci/compute-changes.py:
##########
@@ -546,11 +572,17 @@ 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 event_allows("build_linux", event)
+ and matches(NATIVE_LIBRARY_INPUTS, files)):
+ selected["build_linux"] = True
+ return selected
Review Comment:
This override can go, and dropping it also closes viirya's `--locked` gap
above. Extend the Linux filter next to the consumer loop instead:
```python
FILTERS["build_linux"].extend(NATIVE_BUILD_INPUTS)
```
and let `compute()` return the plain comprehension again. I tried it on this
head. Across all 2,271 tracked files, push routing is unchanged. The only
difference is that the two `contrib/*/native/Cargo.toml` files (4 commits on
main in six months) now also select `build_linux` on the PR and queue tiers,
and `build_linux_all_profiles` nightly.
It has to be the build list rather than the library list. `matches()`
applies excludes globally, so `!**/benches/**` would stop routing the three
`native/shuffle/benches/*` files, which no other PR-tier filter covers.
This also puts the route back under `check_change_filters`, which calls
`matches()` on `FILTERS` directly and so never sees this override. Without the
override, `test_native_input_routing` has nothing left of its own:
- Its routing asserts become two `ROUTING_CASES` rows,
`(["contrib/lance/native/Cargo.toml"], {"build_linux", "build_linux_full",
"build_linux_all_profiles"})` and `(["dev/ci/test-native-cache-key.py"],
{"build_linux", "build_linux_full", "build_linux_all_profiles",
"build_macos"})`. `check-ci-config.py` passes with both.
- Its `matches(NATIVE_LIBRARY_INPUTS, ...)` asserts repeat what the two
real-Git key tests prove. Only `dev/ci/native-cache-key.py`, `rust-toolchain`,
`rust-toolchain.toml` and `native/core/README.md` are missing from those tests,
and they can join their path lists.
- The `POLICY` patch case only tests the override's own gate.
##########
dev/ci/native-cache-key.py:
##########
@@ -0,0 +1,144 @@
+#!/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")),
Review Comment:
`java_home` repeats `env["JAVA_HOME"]`, which the explicit name set below
captures. `cargo_home` repeats `env["CARGO_HOME"]`, which the `CARGO_` prefix
captures and `amd64/rust` always sets. The test's
`assertEqual(environment["env"], build_env)` already shows both land in `env`.
Dropping the two entries leaves the key's coverage on the supported builder
unchanged, and the `environment_inputs` stub in the ownership test can then
return `{}`.
##########
dev/ci/native-cache-key.py:
##########
@@ -0,0 +1,144 @@
+#!/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 {
+ "source-key": prefix + digest(sources),
+ "restore-prefix": prefix,
+ "binary-key": f"Linux-native-ci-v2-{digest([environment, sources])}"
if profile == "ci" else "",
Review Comment:
Naming nit: these outputs and the steps that use them mix three
vocabularies. `binary-key` feeds the `binary-cache` step, named "Restore native
library cache". `source-key` feeds `cargo-cache`, named "Restore incremental
Cargo cache", and the README says "library" and "incremental". `source-key`
also reads as the key over the sources, but both keys digest the sources and
the environment. What differs is what they cache. `library-key` (with a
`library-cache` step id) and `cargo-key` would match the "native library" and
`cargo-ci`/`cargo-debug` terms the workflows and `CACHE_REFRESH_JOBS` already
use.
--
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]
