sunchao commented on code in PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#discussion_r4127439687
########## 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: Done in de25d50f. Removed the duplicate java_home/cargo_home fields; JAVA_HOME and CARGO_HOME remain covered by the captured environment. The ownership-test environment stub now returns {}. ########## 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: Renamed in de25d50f: library-key/library-cache for the finished library and cargo-key/cargo-cache for incremental outputs, including the debug workflow and tests. No binary-key/source-key references remain under .github or dev/ci. ########## .github/actions/build-native-ci/action.yaml: ########## @@ -0,0 +1,75 @@ +# 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 restores the incremental cache below, including libcomet.so. + # A lookup avoids downloading the same library separately. + 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: 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' && steps.cargo-cache.outputs.cache-hit != 'true') + shell: bash + run: | + cd native + # A library miss always builds, even on an exact incremental hit. + # Main also builds to populate a missing incremental entry; two exact + # hits already supply the library and leave neither cache to publish. + cargo build --locked --profile ci Review Comment: Added in de25d50f. After cargo build succeeds, the same fail-fast step runs --check-depinfo target/ci/libcomet.d before either save step. Every declared local file must be in the tracked fingerprint, with explicit exceptions for the five generated protobuf modules and JAVA_HOME. Directory declarations are checked recursively so untracked/excluded descendants cannot slip through. The parser follows real Cargo escaping and rejects unsupported relative paths instead of guessing their base. Regression tests cover excluded/untracked files, generated files, directories, JDK inputs, escaped filenames and symlink escapes. Independent tiny Cargo builds verified real dep-info acceptance and rejection after adding an excluded tracked include or an untracked file in a watched directory. The action integration test confirms an uncovered dependency fails the actual build script. This guards Cargo's declared inputs; it is not a full audit of undeclared build-script file reads. Full Comet hosted validation is pending. -- 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]
