Skip to content

Commit 23bba35

Browse files
andygroveandygrove
committed
fix: build release jars from the same commit as the native libraries
build-release-comet.sh built the native libraries in Docker from the -b branch but built the jars in the local checkout the script was run from, so a release could combine code from two branches (plus any local changes), and each container resolved the branch on its own. Resolve -b to one commit in a fresh clone, pass that commit to both builder containers, and build the jars in the clone. The local checkout is no longer built or cleaned.
1 parent 61810e2 commit 23bba35

3 files changed

Lines changed: 50 additions & 38 deletions

File tree

‎dev/release/build-release-comet.sh‎

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,20 @@
2121
set -e
2222

2323
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null && pwd)"
24-
COMET_HOME_DIR=$SCRIPT_DIR/../..
2524

2625
function usage {
2726
local NAME=$(basename $0)
2827
cat <<EOF
2928
Usage: $NAME [options]
3029
3130
This script builds comet native binaries inside a docker image. The image is named
32-
"comet-rm" and will be generated by this script
31+
"comet-rm" and will be generated by this script. It then builds the jars and installs
32+
them to a temporary local Maven repository.
33+
34+
The branch is resolved to a single commit when the script starts, and the native
35+
binaries and the jars are all built from fresh clones of the repo at that commit.
36+
The local checkout that this script is run from is not built, so its current branch
37+
and any local changes do not end up in the release.
3338
3439
Options are:
3540
@@ -53,6 +58,10 @@ function cleanup()
5358
then
5459
docker rm comet-amd64-builder-container
5560
fi
61+
if [ -n "$BUILD_DIR" ]
62+
then
63+
rm -rf "$BUILD_DIR"
64+
fi
5665
CLEANUP=0
5766
fi
5867
}
@@ -87,6 +96,17 @@ if [ "$JAVA_VERSION" -lt 17 ]; then
8796
fi
8897
echo "Java version check passed: $JAVA_VERSION"
8998

99+
# Resolve the branch to a single commit, and build both the native binaries (in the docker
100+
# containers) and the jars (in a fresh clone) from that commit, so that they match even if
101+
# the branch moves during the build. Nothing is built from the local checkout that this
102+
# script is run from, so its current branch, local changes and stale build output (see
103+
# https://github.com/apache/datafusion-comet/issues/2232) cannot leak into the release.
104+
BUILD_DIR=$(mktemp -d /tmp/comet-release-build-XXXXX)
105+
git clone "$REPO" "$BUILD_DIR"
106+
git -C "$BUILD_DIR" checkout "$BRANCH"
107+
COMMIT=$(git -C "$BUILD_DIR" rev-parse HEAD)
108+
echo "Resolved $BRANCH to commit $COMMIT"
109+
90110
WORKING_DIR="$SCRIPT_DIR/comet-rm/workdir"
91111
cp $SCRIPT_DIR/../cargo.config $WORKING_DIR
92112

@@ -122,16 +142,6 @@ docker build --no-cache \
122142
--build-arg MACOS_SDK=${MACOS_SDK} \
123143
"$SCRIPT_DIR/comet-rm"
124144

125-
# Clean previous Java build
126-
pushd $COMET_HOME_DIR && ./mvnw clean && popd
127-
128-
# Clean previous native build. This is required because spark/pom.xml has
129-
# unconditional resource entries that bundle libcomet.dylib from
130-
# native/target/{x86_64,aarch64}-apple-darwin/release. If a release manager
131-
# previously cross-compiled those targets locally, stale dylibs would leak
132-
# into the release jars. See https://github.com/apache/datafusion-comet/issues/2232
133-
pushd $COMET_HOME_DIR/native && cargo clean && popd
134-
135145
# Run the builder container for each architecture. The entrypoint script will build the binaries
136146

137147
# AMD64
@@ -143,7 +153,7 @@ docker run \
143153
--env "CARGO_BUILD_JOBS=${CARGO_BUILD_JOBS:-2}" \
144154
-it \
145155
--platform linux/amd64 \
146-
$BUILDER_IMAGE_AMD64 "${REPO}" "${BRANCH}" amd64
156+
$BUILDER_IMAGE_AMD64 "${REPO}" "${COMMIT}" amd64
147157

148158
if [ $? != 0 ]
149159
then
@@ -160,7 +170,7 @@ docker run \
160170
--env "CARGO_BUILD_JOBS=${CARGO_BUILD_JOBS:-2}" \
161171
-it \
162172
--platform linux/arm64 \
163-
$BUILDER_IMAGE_ARM64 "${REPO}" "${BRANCH}" arm64
173+
$BUILDER_IMAGE_ARM64 "${REPO}" "${COMMIT}" arm64
164174

165175
if [ $? != 0 ]
166176
then
@@ -171,7 +181,7 @@ fi
171181
echo "Building binaries completed"
172182
echo "Copying to java build directories"
173183

174-
JVM_TARGET_DIR=$COMET_HOME_DIR/spark/target/classes/org/apache/comet
184+
JVM_TARGET_DIR=$BUILD_DIR/spark/target/classes/org/apache/comet
175185
mkdir -p $JVM_TARGET_DIR
176186

177187
mkdir -p $JVM_TARGET_DIR/linux/amd64
@@ -202,9 +212,8 @@ fi
202212

203213
# Build final jar
204214
echo "Building uber jar and publishing it locally"
205-
pushd $COMET_HOME_DIR
215+
pushd "$BUILD_DIR"
206216

207-
GIT_HASH=$(git rev-parse --short HEAD)
208217
LOCAL_REPO=$(mktemp -d /tmp/comet-staging-repo-XXXXX)
209218

210219
./mvnw "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-3.4 -P scala-2.12 -DskipTests install
@@ -217,5 +226,6 @@ LOCAL_REPO=$(mktemp -d /tmp/comet-staging-repo-XXXXX)
217226
./mvnw "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-4.1 -DskipTests install
218227

219228
echo "Installed to local repo: ${LOCAL_REPO}"
229+
echo "Built from commit: ${COMMIT}"
220230

221231
popd

‎dev/release/comet-rm/build-comet-native-libs.sh‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@
2323
set -e
2424

2525
REPO=$1
26-
BRANCH=$2
26+
COMMIT=$2
2727
ARCH=$3
2828

2929
function usage {
3030
local NAME=$(basename $0)
31-
echo "Usage: ${NAME} [git repo] [branch] [arm64 | amd64]"
31+
echo "Usage: ${NAME} [git repo] [commit] [arm64 | amd64]"
3232
exit 1
3333
}
3434

@@ -44,11 +44,11 @@ fi
4444

4545
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null && pwd)"
4646

47-
echo "Building architecture: ${ARCH} for ${REPO}/${BRANCH}"
47+
echo "Building architecture: ${ARCH} for ${REPO} at commit ${COMMIT}"
4848
rm -fr comet
4949
git clone "$REPO" comet
5050
cd comet
51-
git checkout "$BRANCH"
51+
git checkout "$COMMIT"
5252

5353
# build comet binaries
5454

‎docs/source/contributor-guide/release_process.md‎

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -272,18 +272,12 @@ runs start, run them again. Repeat this for every release candidate.
272272

273273
#### A note on workspace cleanliness
274274

275-
The `common/pom.xml` resource configuration unconditionally bundles
276-
`native/target/{x86_64,aarch64}-apple-darwin/release/libcomet.dylib` into the
277-
`common` jar when those files exist on disk. Maven's `clean` removes
278-
`common/target` but does not touch Cargo's `native/target` directory, so a
279-
stale dylib left over from a prior local `make release` or `make release-linux`
280-
on the release manager's workstation can silently end up in a release jar
281-
(see [#2232](https://github.com/apache/datafusion-comet/issues/2232) for the
282-
incident in 0.9.1).
283-
284-
The `build-release-comet.sh` script now runs `cargo clean` for you, but as a
285-
defensive measure, prefer running the release build from a fresh clone of the
286-
repository rather than your day-to-day working tree.
275+
The `build-release-comet.sh` script does not build Comet from your local checkout. It resolves the requested
276+
branch to a single commit when it starts, and builds both the native binaries (inside the Docker containers)
277+
and the jars from fresh clones of the repository at that commit. The branch you have checked out, uncommitted
278+
changes, and stale build output in your working tree therefore cannot end up in the release artifacts (see
279+
[#2232](https://github.com/apache/datafusion-comet/issues/2232) for how a stale `libcomet.dylib` from a local
280+
build ended up in the 0.9.1 release jars).
287281

288282
#### Setup to do the build
289283

@@ -304,14 +298,21 @@ $ docker context use desktop-linux
304298

305299
The `build-release-comet.sh` script will create a docker image for each architecture and use the image
306300
to build the platform specific binaries. These builder images are created every time this script is run.
307-
The script optionally allows overriding of the repository and branch to build the binaries from (Note that
308-
the local git repo is not used in the building of the binaries, but it is used to build the final uber jar).
301+
The script optionally allows overriding of the repository and branch to build from. The branch is resolved to a
302+
single commit when the script starts, and both the binaries and the final uber jar are built from that commit, so
303+
the branch that your local git repo has checked out does not matter.
309304

310305
```shell
311306
Usage: build-release-comet.sh [options]
312307

313308
This script builds comet native binaries inside a docker image. The image is named
314-
"comet-rm" and will be generated by this script
309+
"comet-rm" and will be generated by this script. It then builds the jars and installs
310+
them to a temporary local Maven repository.
311+
312+
The branch is resolved to a single commit when the script starts, and the native
313+
binaries and the jars are all built from fresh clones of the repo at that commit.
314+
The local checkout that this script is run from is not built, so its current branch
315+
and any local changes do not end up in the release.
315316

316317
Options are:
317318

@@ -323,14 +324,15 @@ Options are:
323324
Example:
324325

325326
```shell
326-
cd dev/release && ./build-release-comet.sh && cd ../..
327+
cd dev/release && ./build-release-comet.sh -b branch-0.13 && cd ../..
327328
```
328329

329330
#### Build output
330331

331332
The build output is installed to a temporary local maven repository. The build script will print the name of the
332333
repository location at the end. This location will be required at the time of deploying the artifacts to a staging
333-
repository
334+
repository. The script also prints the commit that the artifacts were built from, which should be the commit that
335+
you tag in the next step.
334336

335337
### Tag the Release Candidate
336338

0 commit comments

Comments
 (0)