Copilot commented on code in PR #4738: URL: https://github.com/apache/bookkeeper/pull/4738#discussion_r3056893595
########## native-io/src/main/java/org/apache/bookkeeper/common/util/nativeio/NativeIOLibraryPath.java: ########## @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ +package org.apache.bookkeeper.common.util.nativeio; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.regex.Pattern; +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.SystemUtils; + +/** + * Resolves the path of the native-io shared library inside the JAR. + * + * <p>The {@code cargo-zigbuild} Maven profile embeds two variants: + * <pre> + * lib/linux-x86_64-gnu/libnative-io.so (glibc, amd64) + * lib/linux-aarch64-gnu/libnative-io.so (glibc, arm64) + * </pre> + * + * <p>An explicit override path can be set via the system property + * {@code bookkeeper.native.io.library.path} or the environment variable + * {@code BOOKKEEPER_NATIVE_IO_LIBRARY_PATH}. + */ +public class NativeIOLibraryPath { + + private static final String LIBRARY_PATH_ENV = "BOOKKEEPER_NATIVE_IO_LIBRARY_PATH"; + private static final String LIBRARY_PATH_PROPERTY = "bookkeeper.native.io.library.path"; + + private NativeIOLibraryPath() { + } + + /** + * Returns an explicit path from system property / env, or {@code null}. + */ + public static String configuredLibraryPath() { + return configuredLibraryPath( + System.getProperty(LIBRARY_PATH_PROPERTY), + System.getenv(LIBRARY_PATH_ENV)); + } + + protected static String configuredLibraryPath(String propertyValue, String envValue) { + return StringUtils.isNotBlank(propertyValue) ? propertyValue : StringUtils.stripToNull(envValue); + } + + /** + * Returns the ordered list of JAR-resource paths to try for the current + * platform and architecture. The first path that successfully loads wins. + */ + public static List<String> currentPlatformLibraryCandidates() { + if (SystemUtils.IS_OS_LINUX) { + return libraryCandidates(SystemUtils.OS_NAME, SystemUtils.OS_ARCH); + } else { + return Collections.emptyList(); + } + } + + protected static List<String> libraryCandidates(String osName, String osArch) { + List<String> paths = new ArrayList<>(); + String osNameTag = osNameTag(osName); + String archTag = archTag(osArch); + paths.add("/lib/" + osNameTag + "-" + archTag + "-gnu/libnative-io.so"); + return paths; + } Review Comment: `osNameTag()` supports macOS/Windows, but `currentPlatformLibraryCandidates()` only ever returns candidates for Linux, and `libraryCandidates()` hard-codes Linux-centric conventions (`-gnu` + `.so`). This inconsistency makes it easy to accidentally “enable” macOS/Windows later while still generating incorrect resource names. Consider (mandatory) either constraining `osNameTag()`/`libraryCandidates()` to Linux-only, or (mandatory) refactoring `libraryCandidates()` to be OS-aware (suffix/extension and tag format per OS) so the method’s output matches what the build actually packages. ########## native-io/src/main/java/org/apache/bookkeeper/common/util/nativeio/NativeIOJni.java: ########## @@ -50,16 +50,32 @@ class NativeIOJni { static native int close(int fd) throws NativeIOException; static { - try { - if (SystemUtils.IS_OS_MAC_OSX) { - NativeUtils.loadLibraryFromJar("/lib/libnative-io.jnilib"); - } else if (SystemUtils.IS_OS_LINUX) { - NativeUtils.loadLibraryFromJar("/lib/libnative-io.so"); - } else { - throw new RuntimeException("OS not supported by Native-IO utils"); + String explicitPath = NativeIOLibraryPath.configuredLibraryPath(); + if (explicitPath != null) { + System.load(explicitPath); + } else { + List<String> candidates = NativeIOLibraryPath.currentPlatformLibraryCandidates(); + if (candidates.isEmpty()) { + throw new IllegalStateException("No native-io JNI library candidates found for platform " + + System.getProperty("os.name") + "/" + System.getProperty("os.arch")); + } Review Comment: The new loading path will always fail on non-Linux platforms because `currentPlatformLibraryCandidates()` returns an empty list unless `IS_OS_LINUX` is true. This is a functional behavior change from the prior implementation (which loaded on macOS and Linux) and will now throw at class-load time on macOS/Windows even if a compatible native library exists. Either (mandatory) reintroduce candidate generation + packaging for macOS/Windows (with correct extensions and resource paths), or (mandatory) make the intent explicit by limiting OS handling to Linux throughout (e.g., reject non-Linux earlier with a clear message and remove macOS/Windows tagging/tests so the API doesn’t suggest support that isn’t present). ########## native-io/src/test/java/org/apache/bookkeeper/common/util/nativeio/NativeIOLibraryPathTest.java: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.bookkeeper.common.util.nativeio; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; + +import java.util.Collections; +import java.util.List; +import org.junit.Test; + +public class NativeIOLibraryPathTest { + + private void assertLibraryCandidates(String osName, String osArch, String... expectedCandidates) { + List<String> expected = expectedCandidates.length == 0 + ? Collections.emptyList() + : List.of(expectedCandidates); + assertEquals(expected, NativeIOLibraryPath.libraryCandidates(osName, osArch)); + } + + @Test + public void testLinuxAmd64() { + assertLibraryCandidates("Linux", "amd64", "/lib/linux-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxX8664() { + assertLibraryCandidates("Linux", "x86_64", "/lib/linux-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxAarch64() { + assertLibraryCandidates("Linux", "aarch64", "/lib/linux-aarch64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxUnknownArch() { + assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("Linux", "riscv64")); + } + + @Test + public void testMacOsAmd64() { + assertLibraryCandidates("MacOS", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testMacAmd64() { + assertLibraryCandidates("Mac", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testMacOsWithSpaceAmd64() { + assertLibraryCandidates("Mac OS", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testWindowsAmd64() { + assertLibraryCandidates("Windows", "amd64", "/lib/windows-x86_64-gnu/libnative-io.so"); Review Comment: These tests codify macOS/Windows candidate paths that use Linux-specific naming (`-gnu` and `.so`). Given the current Maven profile description and copy step only package Linux `.so` variants, these expectations appear inconsistent with what can actually be built/loaded. Recommend (mandatory) either removing macOS/Windows cases (if Linux-only is the intended scope) or updating both build packaging and path generation to use OS-appropriate file extensions/naming and then updating the tests accordingly. ```suggestion assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("MacOS", "amd64")); } @Test public void testMacAmd64() { assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("Mac", "amd64")); } @Test public void testMacOsWithSpaceAmd64() { assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("Mac OS", "amd64")); } @Test public void testWindowsAmd64() { assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("Windows", "amd64")); ``` ########## native-io/pom.xml: ########## @@ -51,175 +62,93 @@ <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> </plugin> - <plugin> - <groupId>com.github.maven-nar</groupId> - <artifactId>nar-maven-plugin</artifactId> - <extensions>true</extensions> - </plugin> - <plugin> - <groupId>org.apache.maven.plugins</groupId> - <artifactId>maven-assembly-plugin</artifactId> - <configuration> - <descriptors> - <descriptor>src/main/assembly/assembly.xml</descriptor> - </descriptors> - <appendAssemblyId>false</appendAssemblyId> - <tarLongFileMode>posix</tarLongFileMode> - </configuration> - <executions> - <execution> - <id>make-assembly</id> - <phase>package</phase> - <goals> - <goal>single</goal> - </goals> - </execution> - </executions> - </plugin> <plugin> <groupId>org.apache.rat</groupId> <artifactId>apache-rat-plugin</artifactId> + <configuration> + <excludes> + <exclude>**/rust/target/**</exclude> + </excludes> + </configuration> </plugin> </plugins> </build> <profiles> <profile> - <!-- from JDK10 javah command is not available - see http://openjdk.java.net/jeps/313 + <!-- + Cross-compile for Linux amd64 + arm64 (glibc) via cargo-zigbuild. + Produces a single JAR containing two variants: + lib/linux-x86_64-gnu/libnative-io.so (glibc, amd64) + lib/linux-aarch64-gnu/libnative-io.so (glibc, arm64) + + Prerequisites: zig, cargo-zigbuild, and both Rust targets + added via `rustup target add`. --> - <id>jdk-without-javah</id> + <id>cargo-zigbuild</id> <activation> - <jdk>[10,)</jdk> + <activeByDefault>true</activeByDefault> </activation> Review Comment: Activating the `cargo-zigbuild` profile by default makes Rust + Zig + cargo-zigbuild mandatory for all builds of this module (including local dev builds and downstream builds that may not need native-io). Since the profile runs during `compile`, this will hard-fail builds on machines without those toolchains. Recommend (mandatory) removing `activeByDefault` and activating via an explicit profile/property, or adding a controlled fallback/skip mechanism (e.g., a property to skip native build steps) so Java-only builds remain possible. ########## .github/workflows/bk-ci.yml: ########## @@ -82,6 +82,22 @@ jobs: distribution: 'temurin' java-version: 17 + - name: Set up Rust + uses: dtolnay/rust-toolchain@stable + with: + # Cross-compilation targets: Linux amd64 and arm64 (glibc) + targets: >- + x86_64-unknown-linux-gnu, + aarch64-unknown-linux-gnu + + - name: Set up Zig + if: steps.check_changes.outputs.docs_only != 'true' + uses: mlugg/setup-zig@v2 + + - name: Install cargo-zigbuild + if: steps.check_changes.outputs.docs_only != 'true' + run: cargo install --locked cargo-zigbuild + Review Comment: Installing `cargo-zigbuild` from source in many CI jobs can add significant time and variability to builds. Consider (optional) caching the Cargo registry/target directories and/or using a prebuilt installer action (or a pinned binary install approach) to reduce repeated compilation across jobs/workflows. ```suggestion - name: Cache cargo-zigbuild if: steps.check_changes.outputs.docs_only != 'true' id: cargo_zigbuild_cache uses: actions/cache@v4 with: path: | ~/.cargo/bin ~/.cargo/registry ~/.cargo/git key: ${{ runner.os }}-cargo-zigbuild-0.20.1 - name: Install cargo-zigbuild if: steps.check_changes.outputs.docs_only != 'true' && steps.cargo_zigbuild_cache.outputs.cache-hit != 'true' run: cargo install --locked cargo-zigbuild --version 0.20.1 ``` ########## native-io/src/main/java/org/apache/bookkeeper/common/util/nativeio/NativeIOJni.java: ########## @@ -50,16 +50,32 @@ class NativeIOJni { static native int close(int fd) throws NativeIOException; static { - try { - if (SystemUtils.IS_OS_MAC_OSX) { - NativeUtils.loadLibraryFromJar("/lib/libnative-io.jnilib"); - } else if (SystemUtils.IS_OS_LINUX) { - NativeUtils.loadLibraryFromJar("/lib/libnative-io.so"); - } else { - throw new RuntimeException("OS not supported by Native-IO utils"); + String explicitPath = NativeIOLibraryPath.configuredLibraryPath(); + if (explicitPath != null) { + System.load(explicitPath); + } else { + List<String> candidates = NativeIOLibraryPath.currentPlatformLibraryCandidates(); + if (candidates.isEmpty()) { + throw new IllegalStateException("No native-io JNI library candidates found for platform " + + System.getProperty("os.name") + "/" + System.getProperty("os.arch")); + } + + boolean loaded = false; + Throwable lastFailure = null; + for (String candidate : candidates) { + try { + NativeUtils.loadLibraryFromJar(candidate); + loaded = true; + break; + } catch (Exception | UnsatisfiedLinkError e) { + lastFailure = e; + } + } + + if (!loaded) { + throw new IllegalStateException("Failed to load any native-io JNI library candidate for platform " + + System.getProperty("os.name") + "/" + System.getProperty("os.arch"), lastFailure); Review Comment: The failure message doesn’t include the set of candidate resource paths that were attempted, which makes diagnosing packaging vs. platform-tag mismatches harder (especially now that multiple variants are embedded). Consider (optional) including the `candidates` list (and possibly the explicit override keys/values if present but blank) in the exception message to speed up debugging. ```suggestion + System.getProperty("os.name") + "/" + System.getProperty("os.arch") + "; attempted candidates: " + candidates, lastFailure); ``` ########## native-io/src/test/java/org/apache/bookkeeper/common/util/nativeio/NativeIOLibraryPathTest.java: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.bookkeeper.common.util.nativeio; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; + +import java.util.Collections; +import java.util.List; +import org.junit.Test; + +public class NativeIOLibraryPathTest { + + private void assertLibraryCandidates(String osName, String osArch, String... expectedCandidates) { + List<String> expected = expectedCandidates.length == 0 + ? Collections.emptyList() + : List.of(expectedCandidates); + assertEquals(expected, NativeIOLibraryPath.libraryCandidates(osName, osArch)); + } + + @Test + public void testLinuxAmd64() { + assertLibraryCandidates("Linux", "amd64", "/lib/linux-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxX8664() { + assertLibraryCandidates("Linux", "x86_64", "/lib/linux-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxAarch64() { + assertLibraryCandidates("Linux", "aarch64", "/lib/linux-aarch64-gnu/libnative-io.so"); + } + + @Test + public void testLinuxUnknownArch() { + assertThrows(IllegalArgumentException.class, () -> assertLibraryCandidates("Linux", "riscv64")); + } + + @Test + public void testMacOsAmd64() { + assertLibraryCandidates("MacOS", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testMacAmd64() { + assertLibraryCandidates("Mac", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testMacOsWithSpaceAmd64() { + assertLibraryCandidates("Mac OS", "amd64", "/lib/macos-x86_64-gnu/libnative-io.so"); + } + + @Test + public void testWindowsAmd64() { + assertLibraryCandidates("Windows", "amd64", "/lib/windows-x86_64-gnu/libnative-io.so"); + } + + @Test Review Comment: These tests codify macOS/Windows candidate paths that use Linux-specific naming (`-gnu` and `.so`). Given the current Maven profile description and copy step only package Linux `.so` variants, these expectations appear inconsistent with what can actually be built/loaded. Recommend (mandatory) either removing macOS/Windows cases (if Linux-only is the intended scope) or updating both build packaging and path generation to use OS-appropriate file extensions/naming and then updating the tests accordingly. ```suggestion ``` -- 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]
