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: [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]
