fix obi java agent build on macos#1247
Conversation
213e04c to
1d38064
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1247 +/- ##
==========================================
- Coverage 43.41% 43.37% -0.05%
==========================================
Files 301 301
Lines 32409 32409
==========================================
- Hits 14071 14056 -15
- Misses 17432 17444 +12
- Partials 906 909 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b71d098 to
d399305
Compare
Signed-off-by: Endre Sara <endresara@gmail.com>
d399305 to
ea6f69d
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses macOS build failures for the OBI Java agent’s JNI native library by shifting the native build to Linux/Docker and producing architecture-specific agent artifacts.
Changes:
- Builds JNI native libraries via Docker and packages them into Linux x86_64 and arm64 agent JARs.
- Adds loader tasks to assemble
obi-java-agent-<arch>.jarartifacts that embed the corresponding agent payload. - Updates Java agent build documentation to describe architecture-specific outputs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/internal/java/README.md | Documents the new arch-specific agent JAR outputs. |
| pkg/internal/java/loader/build.gradle.kts | Adds per-arch copy + ShadowJar tasks to create obi-java-agent-<arch>.jar. |
| pkg/internal/java/build.gradle.kts | Copies per-arch assembled JARs into the root build/ output directory. |
| pkg/internal/java/agent/Makefile.jni | Adds Docker-based build flow on macOS and arch-scoped output directories. |
| pkg/internal/java/agent/build.gradle.kts | Reworks JNI build to produce amd64/arm64 Linux .so via Docker and creates per-arch agent ShadowJars. |
| // Copy obi-java-agent JARs for each arch into root build/ (obi-java-agent-x86_64.jar, obi-java-agent-arm64.jar) | ||
| val copyObiJavaAgentJars by tasks.registering(Copy::class) { | ||
| dependsOn(":loader:obiJavaAgentX86_64", ":loader:obiJavaAgentArm64") | ||
| from(project(":loader").tasks.named("obiJavaAgentX86_64").get().outputs.files) | ||
| from(project(":loader").tasks.named("obiJavaAgentArm64").get().outputs.files) | ||
| into(layout.buildDirectory) | ||
| rename { name -> | ||
| when { | ||
| name.contains("x86_64") -> "obi-java-agent-x86_64.jar" | ||
| name.contains("arm64") -> "obi-java-agent-arm64.jar" | ||
| else -> name | ||
| } | ||
| } |
There was a problem hiding this comment.
copyObiJavaAgentJars no longer produces the previously expected build/obi-java-agent.jar (it only outputs obi-java-agent-x86_64.jar and obi-java-agent-arm64.jar). This is a breaking change: .github/workflows/java-agent.yml uploads pkg/internal/java/build/obi-java-agent.jar, and pkg/internal/java/java_inject.go hard-codes ObiJavaAgentFileName = "obi-java-agent.jar". Either keep generating obi-java-agent.jar for backward compatibility (e.g., as the default x86_64 build), or update all in-repo consumers to select the correct arch-specific JAR at runtime/build time.
| // Copy obi-java-agent JARs for each arch into root build/ (obi-java-agent-x86_64.jar, obi-java-agent-arm64.jar) | |
| val copyObiJavaAgentJars by tasks.registering(Copy::class) { | |
| dependsOn(":loader:obiJavaAgentX86_64", ":loader:obiJavaAgentArm64") | |
| from(project(":loader").tasks.named("obiJavaAgentX86_64").get().outputs.files) | |
| from(project(":loader").tasks.named("obiJavaAgentArm64").get().outputs.files) | |
| into(layout.buildDirectory) | |
| rename { name -> | |
| when { | |
| name.contains("x86_64") -> "obi-java-agent-x86_64.jar" | |
| name.contains("arm64") -> "obi-java-agent-arm64.jar" | |
| else -> name | |
| } | |
| } | |
| // Copy obi-java-agent JARs for each arch into root build/ and keep the legacy obi-java-agent.jar | |
| val copyObiJavaAgentJars by tasks.registering(Copy::class) { | |
| dependsOn(":loader:obiJavaAgentX86_64", ":loader:obiJavaAgentArm64") | |
| into(layout.buildDirectory) | |
| from(project(":loader").tasks.named("obiJavaAgentX86_64").get().outputs.files) { | |
| rename { "obi-java-agent-x86_64.jar" } | |
| } | |
| from(project(":loader").tasks.named("obiJavaAgentArm64").get().outputs.files) { | |
| rename { "obi-java-agent-arm64.jar" } | |
| } | |
| from(project(":loader").tasks.named("obiJavaAgentX86_64").get().outputs.files) { | |
| rename { "obi-java-agent.jar" } | |
| } |
| **Architecture-specific JARs:** The native library is built for Linux only (x86_64 and arm64). `./gradlew build` produces both in `build/`: | ||
|
|
||
| ``` | ||
| build/obi-java-agent.jar | ||
| ``` | ||
| - `build/obi-java-agent-x86_64.jar` — for x86_64 Linux | ||
| - `build/obi-java-agent-arm64.jar` — for arm64 Linux | ||
|
|
||
| Use the JAR that matches your Linux host. Building requires Docker to cross-compile both architectures. |
There was a problem hiding this comment.
This section introduces architecture-specific JAR names, but the usage examples below still reference obi-java-agent.jar. To avoid confusing users (and to match the new artifact names), update the usage snippets to use the selected arch-specific JAR (or explicitly document that obi-java-agent.jar is still produced as a compatibility alias).
|
|
||
| // Build the native JNI library | ||
| tasks.register<Exec>("buildNativeLib") { | ||
| val isMac = System.getProperty("os.name").lowercase().contains("mac") |
There was a problem hiding this comment.
isMac is declared but never used. Either use it to choose between a native build vs Docker (as the Makefile suggests), or remove it to avoid dead code.
| val isMac = System.getProperty("os.name").lowercase().contains("mac") |
| fun execDocker(arch: String, platform: String) = | ||
| listOf( | ||
| "docker", "run", "--rm", | ||
| "--platform", platform, | ||
| "-v", "$agentDir:/work", "-w", "/work", | ||
| dockerImage, | ||
| "bash", "-c", | ||
| "apt-get update -qq && apt-get install -y -qq make clang && make -f Makefile.jni native ARCH=$arch" | ||
| ) |
There was a problem hiding this comment.
The Docker invocation mounts the project directory but runs as root in the container. On Linux hosts this commonly leaves build/ and target/ artifacts owned by root, breaking subsequent builds/cleans for the normal user. Consider running the container with the current UID/GID (or otherwise ensuring host-writable outputs), and/or writing artifacts to a container-only volume and copying them out with correct ownership.
| // Build Linux libobijni.so for x86_64 (always via Docker so it works on macOS and Linux) | ||
| tasks.register<Exec>("buildNativeLibAmd64") { | ||
| group = "build" | ||
| description = "Build the JNI native library (libobijni.so)" | ||
|
|
||
| description = "Build Linux libobijni.so for x86_64 (amd64)" | ||
| dependsOn("compileJava") | ||
|
|
||
| workingDir = projectDir | ||
| commandLine("make", "-f", "Makefile.jni") | ||
|
|
||
| doLast { | ||
| println("OBI JNI library built successfully") | ||
| } | ||
| commandLine(execDocker("amd64", "linux/amd64")) | ||
| doLast { println("OBI JNI library (amd64) built: target/classes/linux-amd64/libobijni.so") } | ||
| } | ||
|
|
||
| // Build Linux libobijni.so for arm64 (always via Docker) | ||
| tasks.register<Exec>("buildNativeLibArm64") { | ||
| group = "build" | ||
| description = "Build Linux libobijni.so for arm64" | ||
| dependsOn("compileJava") | ||
| workingDir = projectDir | ||
| commandLine(execDocker("arm64", "linux/arm64")) | ||
| doLast { println("OBI JNI library (arm64) built: target/classes/linux-arm64/libobijni.so") } |
There was a problem hiding this comment.
These tasks always build via Docker (including --platform linux/arm64), even on Linux where Makefile.jni supports a native build path. This makes ./gradlew build depend on Docker availability and (for arm64) binfmt/qemu support, which can break CI/dev setups that previously built natively. Consider using native make on Linux and Docker only on macOS (or gating the arm64 build behind an explicit opt-in task/flag).
| # ARCH=amd64 or arm64; output goes to target/classes/linux-$(ARCH). Default only for direct 'make' use; Gradle always passes ARCH. | ||
| ARCH ?= amd64 | ||
|
|
||
| SRC_DIR = src/main/c | ||
| BUILD_DIR = build | ||
| TARGET_DIR = target/classes | ||
| BUILD_DIR = build/linux-$(ARCH) | ||
| TARGET_DIR = target/classes/linux-$(ARCH) |
There was a problem hiding this comment.
On macOS, docker run defaults to the host architecture (arm64 on Apple Silicon). With ARCH ?= amd64 as the default, invoking make without explicitly setting ARCH/DOCKER_PLATFORM can produce an arm64 .so but place it under target/classes/linux-amd64, effectively mislabeling the output. Consider defaulting ARCH based on uname -m on Darwin, or enforce that DOCKER_PLATFORM/ARCH are consistent when building via Docker.
| linux-in-docker: | ||
| @command -v docker >/dev/null 2>&1 || (echo "Docker is required to build on macOS." && exit 1) | ||
| docker run --rm $(PLATFORM_OPT) -v "$(CURDIR):/work" -w /work $(DOCKER_IMAGE) bash -c 'apt-get update -qq && apt-get install -y -qq make clang && make -f Makefile.jni native ARCH=$(ARCH)' | ||
| @echo "Built Linux JNI library: $(TARGET)" |
There was a problem hiding this comment.
linux-in-docker mounts the working directory but does not set a user for the container process. On Linux hosts this can leave generated outputs owned by root (e.g., under build/ and target/), which can break subsequent local builds/cleans. Consider running the container with the current UID/GID (or otherwise ensuring host-writable outputs).
|
Confirmed that I can reproduce the original issue on ./gradlew build
...
BUILD SUCCESSFUL in 47s
24 actionable tasks: 15 executed, 9 up-to-date
Consider enabling configuration cache to speed up this build: https://docs.gradle.org/9.3.1/userguide/configuration_cache_enabling.html
...
ls -1 build/*.jar
build/obi-java-agent-arm64.jar
build/obi-java-agent-x86_64.jarRegarding the CI failures:
Suggest further iteration on this PR to address the above. |
fixes #1246