Make library compatible to Windows ARM64 as well #106
Conversation
WalkthroughThe changes introduce architecture-specific handling for building and loading native DLLs within the project. The Makefile is updated to parameterize build outputs by architecture, with new variables and logic to ensure correct output paths and directory creation. The Possibly related issues
Possibly related PRs
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
infeo
left a comment
infeo
left a comment
There was a problem hiding this comment.
I adjusted the pom to contain two executions.
When I inspect the generated jar, there are 3 dll files:
- integrations-x64.dll
- integrations-arm64.dll
- integrations.dll
Where is the third one coming from?
src/main/java/org/cryptomator/windows/common/NativeLibLoader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
19-21: Ensure output directory exists before compilation
Using Make'sifeqwithwildcardto check and create$(OUT_DIR)is correct. Just confirm that themkdirline is indented with a tab (required by Make) and not spaces to avoid execution errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)pom.xml(1 hunks)
🔇 Additional comments (4)
Makefile (2)
2-2: Parameterize build outputs for multiple architectures
The newARCHvariable and relatedOUT_DIR,OUT_DLL, andOUT_LIBdefinitions effectively allow targeting different architectures without duplicating targets. The default ofx64is sensible, and overriding viamake ARCH=arm64aligns with common patterns.Also applies to: 10-12
23-24: Emit artifacts to the correct architecture-specific paths
The updated-Fe,-Fo, and-implibflags direct the DLL and import library into the new$(OUT_DLL)and$(OUT_LIB)locations. This aligns perfectly with the parameterized outputs.Also applies to: 27-27
pom.xml (2)
339-345: Centralizeexec-maven-pluginenvironment configuration
Adding a plugin-level<configuration>withcmdas the executable, the project root asworkingDirectory, and propagatingJAVA_HOMEis a clean way to avoid duplication across executions. This will apply consistently to all<execution>blocks below.
348-348: Use clear execution IDs for architecture targets
Naming the executionscompile-x64andcompile-arm64makes the build steps explicit and easy to track in the Maven logs.
Hi @infeo, I found the time to look into it. 
|
Thanks for checking. I didn't cleaned my environment properly, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pom.xml (1)
367-379: Add ARM64 compilation execution
The newcompile-arm64execution mirrors the x64 flow usingvcvarsamd64_arm64.batandmake install ARCH=arm64, enabling seamless cross‐compilation on the GitHub runner.You may optionally simplify maintenance by switching to
vcvarsall.bat amd64_arm64instead of the dedicated script, e.g.:<argument>/c</argument> <argument>"%VSINSTALLDIR%VC\Auxiliary\Build\vcvarsall.bat" amd64_arm64 && make install ARCH=arm64</argument>This approach reduces the number of hard-coded batch filenames and leverages the generic
vcvarsall.batwith explicit architecture arguments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(2 hunks)src/main/java/org/cryptomator/windows/common/NativeLibLoader.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/windows/common/NativeLibLoader.java
🔇 Additional comments (2)
pom.xml (2)
124-130: Include DLLs in clean phase
Adding a<fileset>to remove.dllresources duringmaven cleanensures no stale native artifacts linger between builds. This aligns with the new ARM64 outputs and avoids accidentally bundling outdated binaries.
346-352: Centralize Exec Plugin configuration
Movingexecutable,workingDirectory, andenvironmentVariablesto the plugin level DRYs up the executions and guarantees consistency across architectures. Confirm that${java.home}correctly points to your JDK rather than just the JRE (if native tools need a full JDK).
3 participants
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.