Make library compatible to Windows ARM64 as well #106
Conversation
WalkthroughThis set of changes introduces architecture-specific handling for native library builds and runtime loading within the project. The Makefile now uses an Possibly related issues
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)README.md(1 hunks)pom.xml(1 hunks)src/main/java/org/cryptomator/windows/common/NativeLibLoader.java(2 hunks)
🔇 Additional comments (10)
src/main/java/org/cryptomator/windows/common/NativeLibLoader.java (3)
15-17: Good addition of architecture-specific constants.The constants for x64 and ARM64 libraries are well-defined, allowing for architecture-specific loading.
34-34: LGTM! Resource loading with dynamic library path.The code correctly uses the dynamically determined library path.
41-46: LGTM! Updated error logging.Error logging now includes the dynamically selected library path.
README.md (1)
19-19: Clear documentation of ARM64 build requirements.The updated README clearly specifies that the MSVC v143 VS 2022 C++ ARM64/ARM64EC-Buildtools component is required for ARM64 support.
pom.xml (1)
353-353: Efficient dual-architecture build implementation.The command efficiently builds for both x64 and ARM64 architectures in a single execution. The approach of initializing the appropriate environment for each architecture before building is correct.
Note: If the x64 build fails, the ARM64 build won't execute since they're chained with
&&. This might be intentional as a fail-fast approach, but you may want to consider separating these into two separate executions if independent builds are desired.Makefile (5)
2-2: Good parameterization with default architecture.Adding the
ARCHvariable with a default value ofx64is a clean approach for enabling multi-architecture builds.
10-12: Well-structured architecture-specific output paths.The output directory and filenames now incorporate the architecture parameter, allowing for clean separation of build artifacts.
19-19: Directory creation using parameterized path.The updated directory creation command correctly uses the architecture-specific output directory.
21-23: Architecture-specific DLL and object file output paths.The compiler output paths correctly use the architecture parameter, ensuring that build artifacts for different architectures don't conflict.
25-25: Architecture-specific import library path.The import library path is correctly parameterized with the architecture.
| var arch = System.getProperty("os.arch"); | ||
| if (arch.contains("amd64")) { | ||
| LOG.debug("Loading library for x86_64 architecture"); | ||
| LIBNAME = X64_LIB; | ||
| } else if (arch.contains("aarch64")) { | ||
| LOG.debug("Loading library for aarch64 architecture"); | ||
| LIBNAME = ARM64_LIB; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a fallback for unrecognized architectures.
The current implementation only handles "amd64" and "aarch64" architectures but doesn't provide a fallback for unrecognized architectures. This could lead to a NullPointerException when using LIBNAME on line 34.
var arch = System.getProperty("os.arch");
if (arch.contains("amd64")) {
LOG.debug("Loading library for x86_64 architecture");
LIBNAME = X64_LIB;
} else if (arch.contains("aarch64")) {
LOG.debug("Loading library for aarch64 architecture");
LIBNAME = ARM64_LIB;
+} else {
+ LOG.warn("Unrecognized architecture: {}. Defaulting to x86_64 architecture", arch);
+ LIBNAME = X64_LIB;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var arch = System.getProperty("os.arch"); | |
| if (arch.contains("amd64")) { | |
| LOG.debug("Loading library for x86_64 architecture"); | |
| LIBNAME = X64_LIB; | |
| } else if (arch.contains("aarch64")) { | |
| LOG.debug("Loading library for aarch64 architecture"); | |
| LIBNAME = ARM64_LIB; | |
| } | |
| var arch = System.getProperty("os.arch"); | |
| if (arch.contains("amd64")) { | |
| LOG.debug("Loading library for x86_64 architecture"); | |
| LIBNAME = X64_LIB; | |
| } else if (arch.contains("aarch64")) { | |
| LOG.debug("Loading library for aarch64 architecture"); | |
| LIBNAME = ARM64_LIB; | |
| } else { | |
| LOG.warn("Unrecognized architecture: {}. Defaulting to x86_64 architecture", arch); | |
| LIBNAME = X64_LIB; | |
| } |
This requires the component "MSVC v143 VS 2022 C++ ARM64/ARM64EC-Buildtools" for compiling. These are included in the GitHub windows runner-image, so no adjustment is needed for the GitHub workflow of this lib.