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