feat: implement scripts for binary release build #932

parthchandra · 2024-09-10T17:48:41Z

Which issue does this PR close?

Closes #721

Rationale for this change

Allows us to publish artifacts to maven

What changes are included in this PR?

Scripts, and Dockerfile to do the binary build in a docker container and include them in an uber jar

How are these changes tested?

Locally.

parthchandra · 2024-09-10T17:52:43Z

@andygrove FYI. This will build an uber jar but does not have the script to deploy. That script can be a different PR.
Note: This includes support for MacOS binaries but that part does not actually work correctly because the build breaks on compiling Blake3. The MacOS build is skipped if the XCode library is not provided.

parthchandra · 2024-09-10T22:32:30Z

MacOS build hits this - BLAKE3-team/BLAKE3#180. Will try the suggested solutions.

dev/release/build-release-comet.sh

andygrove · 2024-09-11T21:13:02Z

dev/release/build-release-comet.sh

+  -t "comet-rm:$IMGTAG" \
+  --build-arg HAS_MACOS_SDK=${HAS_MACOS_SDK} \
+  --build-arg MACOS_SDK=${MACOS_SDK} \
+  --load \


I had to make this change to get this to work (or at least get further along) on linux, based on the comment at docker/buildx#59 (comment).

Suggested change

--load \

--push \

The next issue I hit was:

------ > exporting to image: ------ ERROR: failed to solve: failed to push comet-rm:latest: push access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed Cleaning up ... Error response from daemon: No such container: comet-arm64-builder-container

--push will try to push to docker hub and we don't want to do that since this is a temporary image. --load will load into your local containerd store but it that does not work for you then let me look for a workaround for this.

a multi-arch image

parthchandra · 2024-09-13T16:50:05Z

@andygrove I changed the script to build a different image for each architecture instead of a single multi-arch image. It makes things much simpler at the cost of having multiple images (and a small increase in building time). It also removes the need to have the local container store and will also work with a custom docker backend as long as the backend supports docker build.
I'm hoping this addresses some of the authentication issues you are seeing.
Also, I have noticed I get some network errors doing the build inside a container when on a VPN. Maybe we could try when not on a VPN?
Anyway, could you take this for a spin?

parthchandra · 2024-09-13T17:17:29Z

@andygrove @viirya For the binary builder I chose to use Ubuntu 20.04 as the base image because that is the image we currently use for our published docker images.
Ubuntu 20.04 has glibc 2.31 which means that many redhat based releases will be incompatible because they have an older glibc version. Centos 7 for instance has glibc 2.17
(See: https://gist.github.com/wagenet/35adca1a032cec2999d47b6c40aa45b1)

Should we consider using an older version of Ubuntu?
(BTW I tried to build with an older version of glibc but the build kept failing for one reason or the other so I abandoned that effort).

viirya · 2024-09-13T17:39:21Z

Hmm, I think for OSS Comet we don't have the restriction on supported glibc for platform compatibility. Glibc 2.31 seems to be released on 2020. I think it is old enough for the compatibility of our binary release. For example, Centos 7 is already EOL (https://blog.centos.org/2023/04/end-dates-are-coming-for-centos-stream-8-and-centos-linux-7/)

Ubuntu 20.04 looks like a reasonable choice.

I personally wouldn't want to spend too much efforts on resolving issues on building on older versions of Ubuntu.

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

andygrove · 2024-09-16T19:42:30Z

I ran the scripts locally and they seem to have worked.

I ran this command:

./dev/release/build-release-comet.sh -r https://github.com/parthchandra/datafusion-comet.git -b binary-build

The resulting jar file contains the following native libs:

% jar tvf  spark/target/comet-spark-spark3.4_2.12-0.3.0-SNAPSHOT.jar | grep libcomet
149504624 Wed Jan 22 15:10:16 MST 2020 org/apache/comet/darwin/aarch64/libcomet.dylib
52964152 Wed Jan 22 15:10:16 MST 2020 org/apache/comet/linux/aarch64/libcomet.so
56773320 Wed Jan 22 15:10:16 MST 2020 org/apache/comet/linux/amd64/libcomet.so

parthchandra · 2024-09-16T20:40:14Z

The artifact

149504624 Wed Jan 22 15:10:16 MST 2020 org/apache/comet/darwin/aarch64/libcomet.dylib

seems to be a leftover from a manual run. The script will not prepare macos binaries at the moment.

parthchandra · 2024-09-16T20:41:08Z

@andygrove thank you for testing! This is ready for review.

andygrove

LGTM. Thanks @parthchandra!

viirya · 2024-09-17T19:46:28Z

Makefile

+endif
+
+# build native libs for arm64 architecture Linux/MacOS on a Linux/arm64 machine/container
+core-arm64-libs:


There are two core-arm64-libs targets?

viirya · 2024-09-17T19:51:20Z

Makefile

+ifdef $(HAS_OSXCROSS)
+	cd native && cargo zigbuild -j 1 --target aarch64-apple-darwin --release
+endif
+	cd native && cargo build -j 2 --release


So for MacOSX build, we need to run both cargo zigbuild and cargo build?

Oops. This was a mistake. I experimented with zigbuild for macos. Removed

viirya · 2024-09-17T20:32:57Z

Makefile

+# build native libs for arm64 architecture Linux/MacOS on a Linux/arm64 machine/container
+core-arm64-libs:
+	# if the environment variable HAS_OSXCROSS is defined
+ifdef $(HAS_OSXCROSS)


Do we need MacOS X SDK installed for HAS_OSXCROSS case?

This part is a placeholder for future work to enable MacOS. MacOS Sdk has to be provided to the Docker file as input and the build-release-comet script will copy it into the release builder's Docker image.
I removed the option because the build did not succeed but left the work so we can fix this later. I can remove it if it makes things clearer.

viirya · 2024-09-17T20:36:48Z

dev/release/comet-rm/Dockerfile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+ARG HAS_MACOS_SDK="false"


https://hub.docker.com/r/messense/cargo-zigbuild claims they have MacOS X SDK pre-installed in their docker image. Can we reuse it to use MacOS X SDK for Comet build?

I did not try the docker image from zigbuild (yet). I will try it and if it works, then we can remove the HAS_OSXCROSS portions entirely.
Follow up issue: #947

I tried the zigbuild docker image and the build failed. I'll investigate the failure in the followup.

parthchandra · 2024-09-19T16:21:57Z

@viirya Any further comments?

viirya · 2024-09-19T16:36:26Z

Makefile

@@ -46,6 +46,22 @@ format:
 	./mvnw compile test-compile scalafix:scalafix -Psemanticdb $(PROFILES)
 	./mvnw spotless:apply $(PROFILES)

+# build native libs for amd64 architecture Linux/MacOS on a Linux/amd64 machine/container
+core-amd64-libs:
+	cd native && cargo build -j 2 --release


Don't we need to specify target for this?

This will build the binary for the same architecture as the machine. So no need to specify target.

viirya · 2024-09-19T16:37:57Z

Makefile

+ifdef HAS_OSXCROSS
+	rustup target add x86_64-apple-darwin
+	cd native && cargo build -j 2 --target x86_64-apple-darwin --release
+endif


So as the L51 is not in an else block, if HAS_OSXCROSS is true, we will build the library for x86_64-apple-darwin additionally? I.e., two libraries for core-amd64-libs?

Yes. For the amd64 architecture, one for linux and one for MacOS

viirya · 2024-09-19T16:42:08Z

dev/release/build-release-comet.sh

+./mvnw  "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-3.5 -P scala-2.13  -DskipTests install
+
+echo "Installed to local repo: ${LOCAL_REPO}"
+


Do we need to remove the created docker image/container after installation?

The container is removed in the cleanup part of the script which is invoked on exit or error.

viirya · 2024-09-19T16:43:03Z

Looks good to me, with a few minor questions.

feat: implement scripts for binary release build

f3f293c

parthchandra marked this pull request as draft September 10, 2024 17:48

andygrove reviewed Sep 11, 2024

View reviewed changes

dev/release/build-release-comet.sh Show resolved Hide resolved

andygrove reviewed Sep 11, 2024

View reviewed changes

dev/release/build-release-comet.sh Show resolved Hide resolved

andygrove reviewed Sep 11, 2024

View reviewed changes

parthchandra added 4 commits September 11, 2024 20:23

Install to temp local maven repo and updates for MacOS

c98f15e

newline

1bb4196

Use independent docker images for different architectures instead of

87e61be

a multi-arch image

update docs and cleanup

77c8df2

remove unused code

fb9f361

parthchandra mentioned this pull request Sep 16, 2024

feat: Publish artifacts to maven #946

Merged

andygrove reviewed Sep 16, 2024

View reviewed changes

dev/release/comet-rm/build-comet-native-libs.sh Show resolved Hide resolved

fail build script on error

6973a86

Build all profiles

9ae5083

parthchandra marked this pull request as ready for review September 16, 2024 20:40

andygrove approved these changes Sep 16, 2024

View reviewed changes

andygrove requested a review from viirya September 16, 2024 22:18

Merge branch 'main' into binary-build

c4c2873

viirya reviewed Sep 17, 2024

View reviewed changes

remove duplicate target from makefile

5103f73

viirya reviewed Sep 19, 2024

View reviewed changes

viirya approved these changes Sep 19, 2024

View reviewed changes

andygrove merged commit fa275f1 into apache:main Sep 19, 2024
74 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement scripts for binary release build #932

feat: implement scripts for binary release build #932

parthchandra commented Sep 10, 2024

parthchandra commented Sep 10, 2024

parthchandra commented Sep 10, 2024

andygrove Sep 11, 2024

andygrove Sep 11, 2024

parthchandra Sep 12, 2024

parthchandra commented Sep 13, 2024

parthchandra commented Sep 13, 2024

viirya commented Sep 13, 2024

andygrove commented Sep 16, 2024

parthchandra commented Sep 16, 2024

parthchandra commented Sep 16, 2024

andygrove left a comment

viirya Sep 17, 2024

parthchandra Sep 17, 2024

viirya Sep 17, 2024

parthchandra Sep 17, 2024

viirya Sep 17, 2024

parthchandra Sep 17, 2024

viirya Sep 17, 2024

parthchandra Sep 17, 2024

parthchandra Sep 17, 2024

parthchandra commented Sep 19, 2024

viirya Sep 19, 2024

parthchandra Sep 19, 2024

viirya Sep 19, 2024

parthchandra Sep 19, 2024

viirya Sep 19, 2024

parthchandra Sep 19, 2024

viirya commented Sep 19, 2024

		./mvnw "-Dmaven.repo.local=${LOCAL_REPO}" -P spark-3.5 -P scala-2.13 -DskipTests install

		echo "Installed to local repo: ${LOCAL_REPO}"

feat: implement scripts for binary release build #932

feat: implement scripts for binary release build #932

Conversation

parthchandra commented Sep 10, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

parthchandra commented Sep 10, 2024

parthchandra commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthchandra commented Sep 13, 2024

parthchandra commented Sep 13, 2024

viirya commented Sep 13, 2024

andygrove commented Sep 16, 2024

parthchandra commented Sep 16, 2024

parthchandra commented Sep 16, 2024

andygrove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthchandra commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Sep 19, 2024