Repository navigation
Add multi-arch workflow - #426
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR updates build packaging scripts with platform-aware output naming, introduces an OSUtils utility class for centralized platform detection, refactors LibraryLoader to simplify the library loading API, and bumps version numbers to 1.6.0 with VS2022 exporter support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
3c74cb5 to
bab6097
Compare
637cdb2 to
034128e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
owlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.java (1)
47-47: Logger initialized with wrong class.The logger is using
LibraryLoader.classinstead ofEmbeddedScannerPluginLoader.class. This will cause log messages to be attributed to the wrong class, making debugging more difficult.Proposed fix
- private static final Logger log = LoggerFactory.getLogger(LibraryLoader.class); + private static final Logger log = LoggerFactory.getLogger(EmbeddedScannerPluginLoader.class);
🤖 Fix all issues with AI agents
In @build/package-appimage.sh:
- Around line 3-4: The script defines owlplug_version and target_platform but
never uses target_platform when producing the AppImage; update the packaging
step (the part invoking appimagetool / producing the .AppImage) to rename the
generated artifact to include the platform tag (e.g., append or insert
${target_platform} into the filename) or pass target_platform into the
appimagetool output name so the final .AppImage filename matches the DEB/MSI
pattern; locate the variable usages (owlplug_version and target_platform) in
build/package-appimage.sh and add a post-build mv/rename or adjust the
appimagetool output argument to produce an artifact named with
${owlplug_version}-${target_platform}.AppImage.
🧹 Nitpick comments (6)
build/package-msi.cmd (1)
23-24: Consider adding error handling for the rename operation.If
jpackagefails or produces an unexpected filename, themovecommand will fail silently. Consider adding validation orif existchecks.💡 Optional improvement
-move ./output/OwlPlug-%owlplug-version%.msi ./output/OwlPlug-%owlplug-version%-%target-platform%.msi +if exist "./output/OwlPlug-%owlplug-version%.msi" ( + move ./output/OwlPlug-%owlplug-version%.msi ./output/OwlPlug-%owlplug-version%-%target-platform%.msi +) else ( + echo ERROR: MSI file not found + exit /b 1 +)owlplug-host/src/main/java/com/owlplug/host/utils/OSUtils.java (1)
24-34: Consider making this a proper utility class.Utility classes with only static methods should typically be
finalwith a private constructor to prevent instantiation and subclassing.💡 Suggested improvement
-public class OSUtils { +public final class OSUtils { + + private OSUtils() { + // Utility class + } public static String getPlatformTagName() {owlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.java (1)
233-242: Clarify or remove dead code.
getPlatformExecutableExtension()now unconditionally returns an empty string for all platforms. If platform-specific extensions are no longer needed, consider removing this method entirely and using an empty string literal directly, or document why it's kept as a placeholder.owlplug-host/src/main/java/com/owlplug/host/loaders/jni/JNIPluginMapper.java (1)
41-46: Singleton pattern is not thread-safe.The
getInstance()method lacks synchronization, which could result in multiple instances being created if called concurrently from different threads. While this may be acceptable ifinit()is always called from a single thread, consider using double-checked locking or an enum-based singleton for safety.Example thread-safe singleton
+ private static final Object LOCK = new Object(); + public static JNIPluginMapper getInstance() { if (INSTANCE == null) { - INSTANCE = new JNIPluginMapper(); + synchronized (LOCK) { + if (INSTANCE == null) { + INSTANCE = new JNIPluginMapper(); + } + } } return INSTANCE; }build/package-dmg.sh (2)
3-4: Add input validation for required parameters.The script will fail or produce unexpected results if
owlplug_versionortarget_platformare not provided. Consider adding validation at the start of the script.Proposed validation
echo "Preparing package" owlplug_version=$1 target_platform=$2 + +if [ -z "$owlplug_version" ] || [ -z "$target_platform" ]; then + echo "Usage: $0 <version> <target_platform>" + exit 1 +fi
16-21: Quote variables to handle edge cases.While unlikely in this context, unquoted variables can cause issues with special characters or spaces. Quoting
${owlplug_version}and${target_platform}in the mv command and jpackage arguments would be safer.Proposed fix
-jpackage --input ./input/ --name OwlPlug --main-class org.springframework.boot.loader.launch.JarLauncher \ ---main-jar owlplug.jar --license-file ./input/LICENSE --dest ./output \ ---app-version ${owlplug_version} --icon ./resources/owlplug.icns --vendor OwlPlug \ ---mac-package-identifier owlplug --mac-package-name OwlPlug +jpackage --input ./input/ --name OwlPlug --main-class org.springframework.boot.loader.launch.JarLauncher \ +--main-jar owlplug.jar --license-file ./input/LICENSE --dest ./output \ +--app-version "${owlplug_version}" --icon ./resources/owlplug.icns --vendor OwlPlug \ +--mac-package-identifier owlplug --mac-package-name OwlPlug -mv ./output/OwlPlug-${owlplug_version}.dmg ./output/OwlPlug-${owlplug_version}-${target_platform}.dmg +mv "./output/OwlPlug-${owlplug_version}.dmg" "./output/OwlPlug-${owlplug_version}-${target_platform}.dmg"
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/main.ymlis excluded by!**/*.yml
📒 Files selected for processing (13)
build/package-appimage.shbuild/package-deb.shbuild/package-dmg.shbuild/package-msi.cmdowlplug-host/src/main/java/com/owlplug/host/io/LibraryLoader.javaowlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.javaowlplug-host/src/main/java/com/owlplug/host/loaders/jni/JNIPluginMapper.javaowlplug-host/src/main/java/com/owlplug/host/utils/FileSystemUtils.javaowlplug-host/src/main/java/com/owlplug/host/utils/OSUtils.javaowlplug-host/src/main/juce/JuceLibraryCode/JuceHeader.howlplug-host/src/main/juce/OwlPlugHost.jucerowlplug-host/src/main/resources/owlplug-host.versionowlplug-host/src/main/resources/owlplug-scanner.version
💤 Files with no reviewable changes (1)
- owlplug-host/src/main/java/com/owlplug/host/utils/FileSystemUtils.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-07-26T20:55:41.458Z
Learnt from: DropSnorz
Repo: DropSnorz/OwlPlug PR: 225
File: build/OwlPlug.AppDir/AppRun:1-6
Timestamp: 2024-07-26T20:55:41.458Z
Learning: The `AppRun` script used in the OwlPlug AppImage is the default script from the AppImages repository, and despite not being POSIX compliant, it is used for consistency and compatibility reasons.
Applied to files:
build/package-appimage.sh
📚 Learning: 2025-11-03T18:35:06.941Z
Learnt from: DropSnorz
Repo: DropSnorz/OwlPlug PR: 359
File: owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java:104-113
Timestamp: 2025-11-03T18:35:06.941Z
Learning: In OwlPlug's PluginScanTask (owlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.java), all plugin paths are absolute paths, never relative, as they are normalized via FileUtils.convertPath before being stored or used as directory scopes.
Applied to files:
owlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.javaowlplug-host/src/main/java/com/owlplug/host/loaders/jni/JNIPluginMapper.javaowlplug-host/src/main/java/com/owlplug/host/io/LibraryLoader.java
🧬 Code graph analysis (2)
owlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.java (1)
owlplug-host/src/main/java/com/owlplug/host/utils/OSUtils.java (1)
OSUtils(24-72)
owlplug-host/src/main/java/com/owlplug/host/loaders/jni/JNIPluginMapper.java (3)
owlplug-host/src/main/java/com/owlplug/host/utils/OSUtils.java (1)
OSUtils(24-72)owlplug-host/src/main/java/com/owlplug/host/io/ClassPathVersionUtils.java (1)
ClassPathVersionUtils(27-59)owlplug-host/src/main/java/com/owlplug/host/io/LibraryLoader.java (1)
LibraryLoader(38-111)
🪛 Shellcheck (0.11.0)
build/package-appimage.sh
[warning] 4-4: target_platform appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-host-linux (linux-x64)
- GitHub Check: build-host-win (win-x64)
- GitHub Check: build-host-osx (osx-arm64)
- GitHub Check: build-host-osx (osx-x64)
🔇 Additional comments (18)
owlplug-host/src/main/resources/owlplug-scanner.version (1)
1-1: LGTM!Version bump to 0.4.0 aligns with the project-wide version updates for multi-arch support.
build/package-msi.cmd (1)
3-5: LGTM!The
target-platformparameter is properly captured and used to tag the MSI output filename, aligning with the multi-arch naming convention across other packaging scripts.build/package-deb.sh (1)
3-4: LGTM!Variables are properly defined and the syntax is consistent with shell best practices.
owlplug-host/src/main/java/com/owlplug/host/utils/OSUtils.java (2)
47-65: LGTM!The architecture detection logic handles common
os.archvalues well, including:
- x86_64/amd64 → x64
- i386-i686/x86 → x32
- aarch64/arm64 → arm64
- arm/arm32 → arm32
The null check and lowercase normalization are good defensive practices.
67-72: LGTM on the POSIX detection.Using
FileSystems.getDefault().supportedFileAttributeViews()is a reliable way to detect POSIX filesystem support.Minor: There's an extra blank line before the closing brace (line 71).
owlplug-host/src/main/java/com/owlplug/host/loaders/EmbeddedScannerPluginLoader.java (1)
95-102: LGTM!The switch from
FileSystemUtils.isPosix()toOSUtils.isPosixFileSystem()is consistent with the centralization effort in this PR. The POSIX permission handling logic remains correct.owlplug-host/src/main/resources/owlplug-host.version (1)
1-1: LGTM!Version bump to 1.6.0 is consistent with the JuceHeader.h changes and aligns with the multi-arch workflow release.
owlplug-host/src/main/java/com/owlplug/host/loaders/jni/JNIPluginMapper.java (2)
31-32: Same empty platform tag concern as EmbeddedScannerPluginLoader.If
OSUtils.getPlatformTagName()returns an empty string,LIB_IDwill be malformed with a trailing hyphen (e.g.,owlplug-host-1.6.0-). This mirrors the issue inEmbeddedScannerPluginLoaderand should be handled consistently across both classes.
48-52: LGTM!The updated
LibraryLoader.load()call aligns with the simplified API. The guard against re-loading an already loaded library is correct.owlplug-host/src/main/juce/JuceLibraryCode/JuceHeader.h (1)
49-50: LGTM!Version bump to 1.6.0 is correctly reflected in both
versionStringandversionNumber(0x10600). The hex encoding follows JUCE's convention of major/minor/patch in byte pairs.owlplug-host/src/main/java/com/owlplug/host/io/LibraryLoader.java (5)
38-46: LGTM - Good utility class pattern.Making the class
finalwith a private constructor correctly enforces non-instantiability. The fail-fast behavior fromplatformExtension()during static initialization is appropriate for early detection of unsupported platforms.
48-55: Clean loading flow with sensible fallback chain.The three-step approach (system library path → cached temp file → extract from classpath) is well-structured and provides good resilience. The API simplification from
throwOnFailureto a boolean return is cleaner.
57-81: LGTM - Well-implemented loading helpers.Proper distinction between
System.loadLibrary(name-based) andSystem.load(path-based), with appropriate existence checks and logging levels.
83-101: Good use of NIO for resource extraction.Clean implementation with proper try-with-resources and null checks. The
REPLACE_EXISTINGoption appropriately handles stale temp files from previous runs.One minor consideration: if multiple JVMs or threads simultaneously extract and load the same library, there's a small race window between
Files.copycompleting andtryLoadByPathexecuting where another process could overwrite the file. In practice, this is unlikely during typical single-threaded startup scenarios.
103-110: LGTM - Correct platform extension mapping.The extension mappings are accurate for each platform, and throwing
UnsupportedOperationExceptionprovides clear fail-fast behavior for unsupported operating systems.owlplug-host/src/main/juce/OwlPlugHost.jucer (3)
3-5: LGTM!Version bump to 1.6.0 aligns with the broader version updates across the project.
57-58: Good deprecation practice.Retaining VS2019 for backward compatibility while clearly signaling the migration path to VS2022 is a sensible approach. The deprecation note is clear and actionable.
78-97: VS2022 exporter configuration is complete and correct.The exporter is properly configured with the appropriate target folder, header paths, and module paths, mirroring the VS2019 structure. All 10 JUCE modules are included consistently with other exporters. VS2022 exporter support has been available in JUCE since version 6.1.3.
034128e to
ebd58fa
Compare
|
Issues:
|
ebd58fa to
f6fa164
Compare
Relates to #409