viirya commented on code in PR #5976:
URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4074949292
##########
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)
Review Comment:
The PR's rationale for keeping `contrib/*/native/Cargo.toml` in the key is
that Cargo validates those manifests against `native/Cargo.lock` under
`--locked`. But this only selects `build_linux` for them on `push`. I ran
`compute()` for a change to only `contrib/lance/native/Cargo.toml`. It selects
nothing on `pull_request`, `merge_group` or `schedule`, and `build_linux` on
`push`. For delta the queue runs `delta_gate`, but that uses `cargo tree`
without `--locked`.
I checked the failure mode on this branch. Adding a dependency that is not
in `native/Cargo.lock` to `contrib/lance/native/Cargo.toml` makes `cargo
metadata --locked` in `native/` fail with "cannot update the lock file ...
because --locked was passed". So such a PR goes green through the PR tier and
the queue, and the first failure is main's cache-refresh push. From then on
every PR that selects `build_linux` fails `--locked` until someone fixes the
lockfile. Before this PR, the CI build would have quietly updated the lock
instead.
Could `contrib/*/native/Cargo.toml` (and `.cargo/**` and `rust-toolchain`,
for the same reason) route `build_linux` on the PR and queue tiers too? The
assertion in `test_native_input_routing` that a contrib manifest does not
select `build_linux` on `pull_request` would flip. A cheap `cargo metadata
--locked` check in `lint` for those paths would also work if the full build
feels too heavy.
##########
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()
Review Comment:
Now that the caller workflows are out of the key, this sweep is the only
thing that notices a new `env:` entry. A few build scripts in the default
`cargo build --profile ci` graph read variables it does not capture.
`aws-lc-sys` comes in through `aws-config` and `reqwest` -> `rustls` ->
`aws-lc-rs`, and reads `AWS_LC_SYS_NO_ASM`, `AWS_LC_SYS_CFLAGS`,
`AWS_LC_SYS_STATIC`, `AWS_LC_SYS_CMAKE_BUILDER` and `CMAKE_*`. `liblzma-sys`
reads `LZMA_API_STATIC`. `zstd-sys` reads `ZSTD_SYS_USE_PKG_CONFIG`, and the
`pkg-config` crate reads `PKG_CONFIG_*`. None of these match the current
prefixes.
Nothing sets them today, so this is not a live bug. But if someone adds
`AWS_LC_SYS_NO_ASM: 1` to a caller's `env:`, PRs would restore main's library
and never test that change. Would a denylist be safer here? For example,
capture everything except `GITHUB_*`, `RUNNER_*`, `ACTIONS_*`, `HOSTNAME` and
the other per-run variables, so an unknown variable causes a miss instead of a
stale hit. If you would rather keep the allowlist, a `check-ci-config.py`
invariant on the set of `env:` keys in the four callers would catch the drift
instead.
##########
.github/workflows/README.md:
##########
@@ -404,6 +404,42 @@ 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`. PR,
queue,
+scheduled and manual runs restore `native/target/ci/libcomet.so` and skip Cargo
+on an exact library match. Only pushes to `main` save caches. Main skips Cargo
+when both the library and incremental cache match exactly, and builds when
either
+lacks an exact match to replenish it.
+An incremental cache hit alone never replaces compilation. Builds use
+`cargo build --locked --profile ci`; manifest changes requiring a lockfile
update
+must include that update to `native/Cargo.lock`. Artifact paths remain
unchanged.
+
+`dev/ci/native-cache-key.py` snapshots native sources, protobufs, dependencies,
+Cargo configuration and shared build/setup actions before source generation.
+It includes Rust versions, installed package versions, architecture, JDK
+release/path, and Cargo/Rust, C/C++ compiler/flag and HDFS environment
overrides.
+Caller workflows are excluded because their selected tools and environment are
+observed directly. Spark edits, documentation, generated files and disabled
+contrib sources preserve the key; contrib manifests remain inputs for
`--locked`.
+Benchmarks enter only the debug key. The input lists and glob matcher are
shared
+with main's routing in `compute-changes.py`; code generation uses `x86-64-v3`.
+
+The helper supports the official Rust container and `setup-builder`.
Introducing
Review Comment:
Only `pr_build_linux`'s `build-native` publishes on push, and the restore
prefix includes the environment. So a producer whose JDK, image or env differs
never gets an exact hit and never gets a prefix fallback either. It builds cold
every time. Today every caller resolves to JDK 17 in `amd64/rust`, so this
holds. But `spark_sql_test_reusable.yml` takes `jdk-version: ${{ inputs.java
}}`, and a future `java: 21` row would quietly cold-build on every run, where
the old JDK-agnostic key still restored.
Could this section state that constraint? It might also be worth having
`check-ci-config.py` assert that every job using `build-native-ci` matches
`build-native`'s container image and JDK.
--
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]
