You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/29 03:17:03 UTC
[PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
andygrove opened a new pull request, #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377
## Which issue does this PR close?
Closes #3292.
## Rationale for this change
`dev/release/build-release-comet.sh` builds the native libraries in Docker containers that clone the repository and check out the branch passed with `-b`. It then builds and installs the jars with `./mvnw install` in the local checkout the script is run from (`$COMET_HOME_DIR`), at whatever branch and commit that checkout is on, including any uncommitted changes. The jars also record that local commit through `git-commit-id-maven-plugin` (`comet-git-info.properties`, surfaced as `COMET_REVISION`). The script's `GIT_HASH` also came from the local checkout, though nothing used it. So a release built with `-b branch-0.13` from a checkout of `main` bundles native code from one branch with JVM code from another.
There is a second problem: each builder container resolves `-b` on its own. If a commit lands on the release branch while the amd64 build is running, the arm64 library is built from a different commit.
## What changes are included in this PR?
- `build-release-comet.sh` clones `-r` into a temporary directory and checks out `-b` there, before building the Docker images. It uses the same `git checkout` the containers use, so branches, tags and commits are all still accepted. This resolves `-b` to a single commit. A bad `-b` now fails right away instead of after the image builds.
- Both builder containers are given that commit instead of the branch name, so both native libraries come from the same commit even if the branch moves.
- The native libraries are copied into the temporary clone, and the jars are built and installed from there. The script no longer builds anything in the local checkout, and no longer cleans it. That makes the `./mvnw clean` and `cargo clean` of the local checkout unnecessary, so they are removed. They were added to keep stale dylibs out of the jars (#2232), and a fresh clone has no `native/target` for stale dylibs to come from. The clone is deleted on exit. The staging Maven repository is kept, as before.
- The script prints the resolved commit at the start, and again at the end next to the staging repository path. The unused `GIT_HASH` is removed.
- In `build-comet-native-libs.sh` (the container entrypoint), the second argument is renamed from `BRANCH` to `COMMIT`.
- The usage text and `release_process.md` now describe the new behavior. The example command now passes `-b branch-0.13`, because the default branch, `release`, does not exist in the repository.
## How are these changes tested?
I did not run a real release build: it needs Docker and a full Maven build for six profiles. Instead I ran:
- `bash -n` on both scripts.
- `shellcheck` 0.11.0 on both scripts. It reports 23 findings, down from 27 on `main`, and none are new. The four that went away are `SC2034` (`GIT_HASH appears unused`) and three unquoted `$COMET_HOME_DIR` uses. The rest are existing `SC2086`, `SC2155` and `SC2181` findings.
- An end-to-end harness, kept outside the repo. It runs the real `build-release-comet.sh`, with the real container entrypoint, against a throwaway upstream repo. The external tools are stand-ins:
- `docker` runs the entrypoint from the image's build context, in a separate directory per container.
- `make` writes the checked-out commit into `libcomet.so`.
- `mvnw` logs its directory, the commit, the number of local changes, and the contents of the libraries it would bundle.
- `java` and `cargo` are trivial stubs.
In the scenario, `main` and `branch-9.9` have diverged, and the local checkout is on `main` with an uncommitted edit and an untracked file. The script is run with `-b branch-9.9`, and a commit lands on `branch-9.9` just before the arm64 container clones the repo.
- On `main`, without this change, the release mixes three commits. The amd64 library came from the branch tip at the start, and the aarch64 library came from the new tip. All six `mvnw install` runs happened in the local checkout, at `main`'s commit, with 2 local changes. `mvnw clean` and `cargo clean` also ran in the local checkout.
- With this change, both libraries and all six `mvnw install` runs used the branch tip at the start, and the jar builds ran in a fresh clone with no local changes. The local checkout's HEAD and local changes were untouched, and the temporary clone was removed on exit.
- With `-b release` (the default, which does not exist), the script stopped with `error: pathspec 'release' did not match any file(s) known to git` before building any Docker image, and removed the temporary clone.
- `prettier --write` on `release_process.md`, and a check that the usage text copied into the doc matches the script's.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377#discussion_r4146842364
##########
docs/source/contributor-guide/release_process.md:
##########
@@ -323,14 +324,15 @@ Options are:
Example:
```shell
-cd dev/release && ./build-release-comet.sh && cd ../..
+cd dev/release && ./build-release-comet.sh -b branch-0.13 && cd ../..
```
#### Build output
The build output is installed to a temporary local maven repository. The build script will print the name of the
repository location at the end. This location will be required at the time of deploying the artifacts to a staging
-repository
+repository. The script also prints the commit that the artifacts were built from, which should be the commit that
+you tag in the next step.
Review Comment:
Good catch, fixed in 89259b0735. The tag step now tags the commit that the script prints (`git tag 0.13.0-rc1 <commit>`) instead of resetting to the branch tip, and says why. I also pointed "If the Vote Fails" at the change log step instead of the tag step, because repeating from the tag step would tag a new commit without rebuilding the jars.
##########
dev/release/build-release-comet.sh:
##########
@@ -87,6 +96,17 @@ if [ "$JAVA_VERSION" -lt 17 ]; then
fi
echo "Java version check passed: $JAVA_VERSION"
+# Resolve the branch to a single commit, and build both the native binaries (in the docker
+# containers) and the jars (in a fresh clone) from that commit, so that they match even if
+# the branch moves during the build. Nothing is built from the local checkout that this
+# script is run from, so its current branch, local changes and stale build output (see
+# https://github.com/apache/datafusion-comet/issues/2232) cannot leak into the release.
Review Comment:
Done in 89259b0735. The Docker build context is now the clone's `dev/release/comet-rm`, and `cargo.config` is copied from the clone's `dev/cargo.config`, which also stops the script from writing `workdir` into the local checkout. That leaves the script itself as the only file taken from the local checkout, so the release doc now says to run it from a checkout of the release branch.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377#issuecomment-5919290670
The change closes the real gap - jars and native libraries now come from one resolved commit. One latent bug I didn't see raised elsewhere:
- **`dev/release/build-release-comet.sh:111`** — in a fresh clone `workdir` doesn't exist (it's gitignored), so `cp "$BUILD_DIR/dev/cargo.config" "$WORKING_DIR"` writes a file called `workdir` rather than placing `cargo.config` inside a directory. It's harmless for the normal Linux-only release because the Dockerfile only does `COPY workdir/cargo.config` in the dormant macOS-SDK stage, but it silently does the wrong thing and would break the macOS build once that path turns on, which is exactly the latent breakage this PR is trying to remove. Please add `mkdir -p "$WORKING_DIR"` right before the copy.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377#discussion_r4129951370
##########
docs/source/contributor-guide/release_process.md:
##########
@@ -323,14 +324,15 @@ Options are:
Example:
```shell
-cd dev/release && ./build-release-comet.sh && cd ../..
+cd dev/release && ./build-release-comet.sh -b branch-0.13 && cd ../..
```
#### Build output
The build output is installed to a temporary local maven repository. The build script will print the name of the
repository location at the end. This location will be required at the time of deploying the artifacts to a staging
-repository
+repository. The script also prints the commit that the artifacts were built from, which should be the commit that
+you tag in the next step.
Review Comment:
Could we update the tagging commands below to tag the printed commit directly? They still fetch the release branch and reset to its latest tip. If the branch advances from A to B during the build, this script correctly builds everything from A, but following those commands tags B. I reproduced that mismatch with a local Git fixture. Using `git tag <rc-tag> <built-commit>` would preserve the pinned revision through this last step.
##########
dev/release/build-release-comet.sh:
##########
@@ -87,6 +96,17 @@ if [ "$JAVA_VERSION" -lt 17 ]; then
fi
echo "Java version check passed: $JAVA_VERSION"
+# Resolve the branch to a single commit, and build both the native binaries (in the docker
+# containers) and the jars (in a fresh clone) from that commit, so that they match even if
+# the branch moves during the build. Nothing is built from the local checkout that this
+# script is run from, so its current branch, local changes and stale build output (see
+# https://github.com/apache/datafusion-comet/issues/2232) cannot leak into the release.
Review Comment:
Would it be simpler to derive the Docker build context and `cargo.config` from `BUILD_DIR` as well? Both image builds still use `$SCRIPT_DIR/comet-rm`, so the Dockerfile and entrypoint come from the caller's checkout. An uncommitted entrypoint edit was still executed in both builders in my fixture. Using the pinned clone for those paths removed that dependency without another clone. If caller-side build tooling is intentional, could we narrow the new documentation's guarantee about local changes to make that distinction clear?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377#discussion_r4129951370
##########
docs/source/contributor-guide/release_process.md:
##########
@@ -323,14 +324,15 @@ Options are:
Example:
```shell
-cd dev/release && ./build-release-comet.sh && cd ../..
+cd dev/release && ./build-release-comet.sh -b branch-0.13 && cd ../..
```
#### Build output
The build output is installed to a temporary local maven repository. The build script will print the name of the
repository location at the end. This location will be required at the time of deploying the artifacts to a staging
-repository
+repository. The script also prints the commit that the artifacts were built from, which should be the commit that
+you tag in the next step.
Review Comment:
Please tag the printed commit directly. Resetting to the latest branch tip can tag a different revision if the branch advances during the build.
##########
dev/release/build-release-comet.sh:
##########
@@ -87,6 +96,17 @@ if [ "$JAVA_VERSION" -lt 17 ]; then
fi
echo "Java version check passed: $JAVA_VERSION"
+# Resolve the branch to a single commit, and build both the native binaries (in the docker
+# containers) and the jars (in a fresh clone) from that commit, so that they match even if
+# the branch moves during the build. Nothing is built from the local checkout that this
+# script is run from, so its current branch, local changes and stale build output (see
+# https://github.com/apache/datafusion-comet/issues/2232) cannot leak into the release.
Review Comment:
Can we use `BUILD_DIR` for the Docker context and `cargo.config` too? These paths still use the caller’s checkout, so local build-script edits can affect the release.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: build release jars from the same commit as the native libraries [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #6377:
URL: https://github.com/apache/datafusion-comet/pull/6377#discussion_r4146858951
##########
dev/release/build-release-comet.sh:
##########
@@ -202,9 +212,8 @@ fi
# Build final jar
echo "Building uber jar and publishing it locally"
-pushd $COMET_HOME_DIR
+pushd "$BUILD_DIR"
-GIT_HASH=$(git rev-parse --short HEAD)
LOCAL_REPO=$(mktemp -d /tmp/comet-staging-repo-XXXXX)
./mvnw "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-3.4 -P scala-2.12 -DskipTests install
Review Comment:
From reading `cleanup`, I expect it to delete `BUILD_DIR` (and the builder containers) even when one of these `mvnw install` runs fails. The copied `libcomet.so` files would then be gone, so a failure in a late profile means redoing both native builds. Before this change they stayed in the local checkout. Would it make sense to keep the clone on failure and print its path?
##########
dev/release/build-release-comet.sh:
##########
@@ -53,6 +58,10 @@ function cleanup()
then
docker rm comet-amd64-builder-container
fi
+ if [ -n "$BUILD_DIR" ]
+ then
+ rm -rf "$BUILD_DIR"
Review Comment:
`BUILD_DIR` is not initialized before the `trap` is installed, so on the early exits (`-h`, the Java version check) this `rm -rf` would run on whatever `BUILD_DIR` the caller has exported in their shell. I haven't run it, but I'd expect that to delete the caller's directory. Would it make sense to add `BUILD_DIR=` next to `CLEANUP=1`, or to use a more specific name?
##########
dev/release/build-release-comet.sh:
##########
@@ -217,5 +226,6 @@ LOCAL_REPO=$(mktemp -d /tmp/comet-staging-repo-XXXXX)
./mvnw "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-4.1 -DskipTests install
echo "Installed to local repo: ${LOCAL_REPO}"
+echo "Built from commit: ${COMMIT}"
Review Comment:
`publish-to-maven.sh` still sets `GIT_HASH` from `git rev-parse --short HEAD` in whatever directory it runs from and puts it in the Nexus staging description. Could it take the commit printed here instead, for example from a file written next to the staging repo? Otherwise the description can name a different commit than the jars were built from. Fine to do this as a follow-up.
##########
docs/source/contributor-guide/release_process.md:
##########
@@ -323,14 +324,15 @@ Options are:
Example:
```shell
-cd dev/release && ./build-release-comet.sh && cd ../..
+cd dev/release && ./build-release-comet.sh -b branch-0.13 && cd ../..
Review Comment:
The CI step above prints the `head_sha` that was tested. Would it make sense to pass that commit as `-b` here and tag the same commit? I expect `git checkout` to accept a SHA, but I haven't run it. Then CI, the build, and the tag agree even if the branch moves in between. The `-b` help text says `git branch`, so it may also need a word about commits.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org