Add support for Minecraft 26.2-snapshot-1+ to unofficial supported platforms#5928
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates LWJGL native dependencies in natives.json, adding support for ARM64, RISCV64, and FreeBSD platforms. In DefaultLauncher.java, the logic for determining the native library path was refined to support subfolders introduced in recent Minecraft snapshots, and the internal Command class was refactored into a record. Feedback was provided regarding the robustness of the java.library.path extraction logic, specifically concerning multiple property occurrences and empty path handling.
| Optional<String> javaLibraryPathArgument = parsedArguments.stream().filter(it -> it.startsWith(libraryPathPrefix)).findFirst(); | ||
| if (javaLibraryPathArgument.isPresent()) { | ||
| // Since Minecraft 26.2-snapshot-1, the java.library.path will be located in a subfolder of ${natives_directory}. | ||
| try { | ||
| javaNativeFolder = Path.of(javaLibraryPathArgument.get().substring(libraryPathPrefix.length())); | ||
| } catch (IllegalArgumentException ignored) { | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for extracting java.library.path is fragile. findFirst() may not correctly handle multiple occurrences of the property, and Path.of() can throw an InvalidPathException if the path string is malformed. Additionally, if the property value is empty, Path.of("") resolves to the current working directory, which could lead to unintended directory operations. Consider finding the last occurrence of the property and validating the path before use.
| Optional<String> javaLibraryPathArgument = parsedArguments.stream().filter(it -> it.startsWith(libraryPathPrefix)).findFirst(); | |
| if (javaLibraryPathArgument.isPresent()) { | |
| // Since Minecraft 26.2-snapshot-1, the java.library.path will be located in a subfolder of ${natives_directory}. | |
| try { | |
| javaNativeFolder = Path.of(javaLibraryPathArgument.get().substring(libraryPathPrefix.length())); | |
| } catch (IllegalArgumentException ignored) { | |
| } | |
| } | |
| Optional<String> javaLibraryPathArgument = parsedArguments.stream() | |
| .filter(it -> it.startsWith(libraryPathPrefix)) | |
| .reduce((first, second) -> second); | |
| if (javaLibraryPathArgument.isPresent()) { | |
| String pathStr = javaLibraryPathArgument.get().substring(libraryPathPrefix.length()); | |
| if (!pathStr.isEmpty()) { | |
| try { | |
| javaNativeFolder = Path.of(pathStr); | |
| } catch (Exception ignored) { | |
| } | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates natives.json to include native library definitions for shaderc, spvc, vma, and vulkan on Linux ARM64, RISC-V 64, and FreeBSD, while removing redundant LWJGL entries. It also modifies DefaultLauncher.java to handle native library path subdirectories in JVM arguments and refactors the Command class into a record. Feedback indicates that the path resolution logic should catch InvalidPathException instead of IllegalArgumentException to avoid runtime crashes.
No description provided.