Refactor Java runtime to async modular system with Adoptium support #82
Conversation
Reviewed-by: Claude-Sonnet-4.5
- Split monolithic java.rs (1089 lines) into focused modules - detection: Java installation discovery - validation: Version validation and requirements checking - priority: Installation selection priority logic - provider: Java download provider trait - providers: Provider implementations (Adoptium) - persistence: Cache and catalog management - Add java_path_override field to Instance struct for per-instance Java configuration - Export JavaInstallation at core module level This refactoring improves maintainability and prepares for multi-vendor Java provider support. Reviewed-by: Claude Sonnet 4.5
|
@BegoniaHe is attempting to deploy a commit to the retrofor Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideRefactors Java handling into a new async, provider-based core::java module, adds richer Java detection/installation capabilities (via Adoptium), introduces per-instance Java overrides, and updates Tauri commands and game-start/install paths to use the new async APIs. Sequence diagram for Java resolution on game startsequenceDiagram
actor User
participant UI as TauriFrontend
participant Cmd as start_game_command
participant JP as JavaPriority
participant JV as JavaValidation
participant JPers as JavaPersistence
participant JCore as CoreJava
User->>UI: Click Play
UI->>Cmd: Invoke start_game(instance_id)
Cmd->>JP: resolve_java_for_launch(app_handle, instance_java_override, global_java_path, required_major_version, max_major_version)
alt instance_java_override set and nonempty
JP->>JV: check_java_installation(instance_java_path)
JV-->>JP: Option_JavaInstallation
alt valid and version compatible
JP-->>Cmd: Some(JavaInstallation)
else invalid or incompatible
note over JP: Fallback to next priority
end
end
alt no valid instance override
alt global_java_path set and nonempty
JP->>JV: check_java_installation(global_java_path)
JV-->>JP: Option_JavaInstallation
alt valid and version compatible
JP-->>Cmd: Some(JavaInstallation)
else invalid or incompatible
note over JP: Fallback to preferred path
end
end
end
alt no valid global path
JP->>JPers: get_preferred_java_path(app_handle)
JPers-->>JP: Option_String
alt preferred path exists
JP->>JV: check_java_installation(preferred_path)
JV-->>JP: Option_JavaInstallation
alt valid and version compatible
JP-->>Cmd: Some(JavaInstallation)
else invalid or incompatible
note over JP: Fallback to detection
end
end
end
alt no valid preferred path
JP->>JCore: detect_all_java_installations(app_handle)
JCore-->>JP: Vec_JavaInstallation
JP-->>Cmd: Option_JavaInstallation (first compatible)
end
Cmd-->>UI: Success or error starting game
Class diagram for new core_java module and related typesclassDiagram
class JavaInstallation {
+String path
+String version
+String arch
+String vendor
+String source
+bool is_64bit
}
class ImageType {
<<enum>>
+Jre
+Jdk
+toString() String
}
class JavaReleaseInfo {
+u32 major_version
+String image_type
+String version
+String release_name
+Option_String release_date
+u64 file_size
+Option_String checksum
+String download_url
+bool is_lts
+bool is_available
+String architecture
}
class JavaCatalog {
+Vec_JavaReleaseInfo releases
+Vec_u32 available_major_versions
+Vec_u32 lts_versions
+u64 cached_at
}
class JavaDownloadInfo {
+String version
+String release_name
+String download_url
+String file_name
+u64 file_size
+Option_String checksum
+String image_type
}
class JavaConfig {
+Vec_String user_defined_paths
+Option_String preferred_java_path
+u64 last_detection_time
}
class JavaProvider {
<<interface>>
+fetch_catalog(app_handle, force_refresh) Result_JavaCatalog_String
+fetch_release(major_version, image_type) Result_JavaDownloadInfo_String
+available_versions() Result_Vec_u32_String
+provider_name() str
+os_name() str
+arch_name() str
+install_prefix() str
}
class AdoptiumProvider {
+new() AdoptiumProvider
+fetch_catalog(app_handle, force_refresh) Result_JavaCatalog_String
+fetch_release(major_version, image_type) Result_JavaDownloadInfo_String
+available_versions() Result_Vec_u32_String
+provider_name() str
+os_name() str
+arch_name() str
+install_prefix() str
}
class DownloadQueue {
+Vec_PendingJavaDownload pending_downloads
+load(app_handle) DownloadQueue
+save(app_handle) Result_void_String
+add(pending) void
+remove(major_version, image_type) void
}
class PendingJavaDownload {
+u32 major_version
+String image_type
+String download_url
+String file_name
+u64 file_size
+Option_String checksum
+String install_path
+u64 created_at
}
class JavaDetection {
<<module>>
+get_java_candidates() Vec_PathBuf
}
class JavaValidation {
<<module>>
+check_java_installation(path) Option_JavaInstallation
+parse_version_string(output) Option_String
+parse_java_version(version) u32
+extract_architecture(output) String
+extract_vendor(output) String
+strip_unc_prefix(path) PathBuf
}
class JavaPersistence {
<<module>>
+load_java_config(app_handle) JavaConfig
+save_java_config(app_handle, config) Result_void_String
+add_user_defined_path(app_handle, path) Result_void_String
+remove_user_defined_path(app_handle, path) Result_void_String
+set_preferred_java_path(app_handle, path) Result_void_String
+get_preferred_java_path(app_handle) Option_String
+update_last_detection_time(app_handle) Result_void_String
}
class JavaPriority {
<<module>>
+resolve_java_for_launch(app_handle, instance_java_override, global_java_path, required_major_version, max_major_version) Option_JavaInstallation
}
class CoreJavaModule {
<<module>>
+get_java_install_dir(app_handle) PathBuf
+load_cached_catalog(app_handle) Option_JavaCatalog
+save_catalog_cache(app_handle, catalog) Result_void_String
+clear_catalog_cache(app_handle) Result_void_String
+fetch_java_catalog(app_handle, force_refresh) Result_JavaCatalog_String
+fetch_java_release(major_version, image_type) Result_JavaDownloadInfo_String
+fetch_available_versions() Result_Vec_u32_String
+download_and_install_java(app_handle, major_version, image_type, custom_path) Result_JavaInstallation_String
+detect_java_installations() Vec_JavaInstallation
+get_recommended_java(required_major_version) Option_JavaInstallation
+get_compatible_java(app_handle, required_major_version, max_major_version) Option_JavaInstallation
+is_java_compatible(java_path, required_major_version, max_major_version) bool
+detect_all_java_installations(app_handle) Vec_JavaInstallation
+resume_pending_downloads(app_handle) Result_Vec_JavaInstallation_String
+cancel_current_download() void
+get_pending_downloads(app_handle) Vec_PendingJavaDownload
+clear_pending_download(app_handle, major_version, image_type) Result_void_String
}
class Instance {
+String id
+String name
+Option_String notes
+Option_String mod_loader
+Option_String mod_loader_version
+Option_String java_path_override
}
JavaProvider <|.. AdoptiumProvider
CoreJavaModule "1" o-- "many" JavaInstallation
CoreJavaModule ..> JavaProvider
CoreJavaModule ..> DownloadQueue
CoreJavaModule ..> PendingJavaDownload
CoreJavaModule ..> JavaDetection
CoreJavaModule ..> JavaValidation
CoreJavaModule ..> JavaPersistence
CoreJavaModule ..> JavaPriority
JavaPriority ..> JavaValidation
JavaPriority ..> JavaPersistence
JavaPriority ..> CoreJavaModule
JavaDetection ..> JavaValidation
DownloadQueue o-- PendingJavaDownload
JavaPersistence o-- JavaConfig
Instance --> JavaPriority
Flow diagram for download_and_install_java processflowchart TD
A[start download_and_install_java] --> B[Create AdoptiumProvider]
B --> C["fetch_release(major_version, image_type)"]
C --> D[Determine install_base and version_dir]
D --> E[Create install_base directory]
E --> F[Load DownloadQueue]
F --> G[Add PendingJavaDownload and save queue]
G --> H[Compute archive_path]
H --> I{archive_path exists?}
I -->|No| K[need_download = true]
I -->|Yes| J[Verify checksum if available]
J -->|Invalid or missing| K[need_download = true]
J -->|Valid| L[need_download = false]
K --> M[download_with_resume]
L --> N[Skip download]
M --> O[Emit java-download-progress status Extracting]
N --> O[Emit java-download-progress status Extracting]
O --> P[Remove existing version_dir if present]
P --> Q[Create version_dir]
Q --> R{Archive extension}
R -->|.tar.gz or .tgz| S[extract_tar_gz to version_dir]
R -->|.zip| T[extract_zip to version_dir and find_top_level_dir]
R -->|other| U[Return error Unsupported archive format]
S --> V[Remove archive file]
T --> V[Remove archive file]
V --> W[Compute java_home and java_bin path]
W --> X{java_bin exists?}
X -->|No| Y[Return error Java executable not found]
X -->|Yes| Z[canonicalize java_bin and strip_unc_prefix]
Z --> AA[validation::check_java_installation]
AA --> AB{installation verified?}
AB -->|No| AC[Return error Failed to verify Java installation]
AB -->|Yes| AD[installation: JavaInstallation]
AD --> AE[Remove entry from DownloadQueue]
AE --> AF[Save DownloadQueue]
AF --> AG[Emit java-download-progress status Completed]
AG --> AH[Return JavaInstallation]
U --> AI[end]
Y --> AI[end]
AC --> AI[end]
AH --> AI[end]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- There are multiple implementations of
strip_unc_prefix(incore/java/detection.rs,core/java/validation.rs, and implicitly incore/java/mod.rsusage) with overlapping behavior; consider consolidating this into a single shared helper to avoid duplication and potential divergence. - The
JAVA_CHECK_TIMEOUTconstant incore/java/validation.rsis defined but never used; either wire it into the process invocation (e.g., by enforcing a timeout onjava -version) or remove it to keep the module minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are multiple implementations of `strip_unc_prefix` (in `core/java/detection.rs`, `core/java/validation.rs`, and implicitly in `core/java/mod.rs` usage) with overlapping behavior; consider consolidating this into a single shared helper to avoid duplication and potential divergence.
- The `JAVA_CHECK_TIMEOUT` constant in `core/java/validation.rs` is defined but never used; either wire it into the process invocation (e.g., by enforcing a timeout on `java -version`) or remove it to keep the module minimal.
## Individual Comments
### Comment 1
<location> `src-tauri/src/core/java/validation.rs:10` </location>
<code_context>
+
+use super::JavaInstallation;
+
+const JAVA_CHECK_TIMEOUT: Duration = Duration::from_secs(5);
+
+pub fn strip_unc_prefix(path: PathBuf) -> PathBuf {
</code_context>
<issue_to_address>
**issue (bug_risk):** Timeout constant is defined but not used, and `check_java_installation_blocking` may hang on broken Java binaries.
`JAVA_CHECK_TIMEOUT` implies the check should be time-bounded, but `Command::output` runs indefinitely, so a hanging `java -version` could block detection. Either apply this timeout to the command execution (e.g., via `wait_timeout`/a blocking task with `tokio::time::timeout`) or remove the constant to avoid confusion.
</issue_to_address>
### Comment 2
<location> `src-tauri/src/core/java/detection.rs:29` </location>
<code_context>
+ }
+}
+
+fn run_which_command_with_timeout() -> Option<String> {
+ let mut cmd = Command::new(if cfg!(windows) { "where" } else { "which" });
+ cmd.arg("java");
</code_context>
<issue_to_address>
**suggestion:** Function name implies a timeout but the implementation does not enforce one.
Given that it only calls `Command::output`, the current behavior doesn’t actually apply a timeout, which can be confusing alongside the timeout constant in `validation.rs`. Either implement an actual timeout here or rename the function to better match what it does.
Suggested implementation:
```rust
fn run_which_command_with_timeout() -> Option<String> {
use std::io::Read;
use std::process::Stdio;
use std::thread::sleep;
use std::time::{Duration, Instant};
let mut cmd = Command::new(if cfg!(windows) { "where" } else { "which" });
cmd.arg("java");
#[cfg(target_os = "windows")]
// Hide the console window on Windows
cmd.creation_flags(0x08000000);
// We only care about stdout from the command
cmd.stdout(Stdio::piped());
// TODO: consider sharing a timeout constant with validation.rs if appropriate
let timeout = Duration::from_secs(2);
let start = Instant::now();
let mut child = match cmd.spawn() {
Ok(child) => child,
Err(_) => return None,
};
loop {
match child.try_wait() {
Ok(Some(status)) => {
if status.success() {
let mut output = String::new();
if let Some(mut stdout) = child.stdout.take() {
let _ = stdout.read_to_string(&mut output);
}
return Some(output);
} else {
let _ = child.wait(); // Ensure we reap the child
return None;
}
}
Ok(None) => {
if start.elapsed() >= timeout {
let _ = child.kill();
let _ = child.wait(); // Reap the killed child
return None;
}
sleep(Duration::from_millis(50));
}
Err(_) => {
let _ = child.kill();
let _ = child.wait();
return None;
}
}
}
}
```
1. If you prefer to reuse an existing timeout constant from `validation.rs`, replace the hard-coded `Duration::from_secs(2)` with that constant (or a derived `Duration`) and move the `use std::time::{Duration, Instant};` import accordingly.
2. If your module already has `use` statements for `std::io::Read`, `std::process::Stdio`, `std::thread::sleep`, or `std::time::{Duration, Instant}`, you can remove the inner `use` lines from the function and keep them at the module level to match your existing style.
</issue_to_address>
### Comment 3
<location> `src-tauri/src/core/java/detection.rs:7` </location>
<code_context>
+#[cfg(target_os = "windows")]
+use std::os::windows::process::CommandExt;
+
+pub fn strip_unc_prefix(path: PathBuf) -> PathBuf {
+ #[cfg(target_os = "windows")]
+ {
</code_context>
<issue_to_address>
**suggestion:** There are two separate `strip_unc_prefix` implementations; centralizing them would reduce duplication.
`strip_unc_prefix` exists in both `detection.rs` and `validation.rs` with the same behavior. Prefer a single shared implementation (for example in `validation` or the top-level `java` module) and call it from both to avoid divergence and ease maintenance.
Suggested implementation:
```rust
use std::path::PathBuf;
use std::process::Command;
#[cfg(target_os = "windows")]
use std::os::windows::process::CommandExt;
// Reuse shared implementation from the parent `java` module.
use super::strip_unc_prefix;
pub fn find_sdkman_java() -> Option<PathBuf> {
```
To complete the refactor and actually centralize `strip_unc_prefix`:
1. Create a single shared implementation, for example in `src-tauri/src/core/java/mod.rs`:
- Add:
```rust
use std::path::PathBuf;
pub fn strip_unc_prefix(path: PathBuf) -> PathBuf {
#[cfg(target_os = "windows")]
{
// Remove the UNC prefix (\\?\) from Windows paths
let s = path.to_string_lossy().to_string();
if s.starts_with(r"\\?\") {
return PathBuf::from(&s[4..]);
}
}
path
}
```
- Ensure you don’t duplicate the function body elsewhere; this becomes the single source of truth.
2. In `src-tauri/src/core/java/validation.rs`, remove its local `strip_unc_prefix` definition and import the shared one instead:
- Add `use super::strip_unc_prefix;` (or an appropriate path like `use crate::core::java::strip_unc_prefix;` depending on how the module is declared).
- Update any calls to `strip_unc_prefix` to use this imported function (no signature changes needed if behavior remains the same).
3. If `strip_unc_prefix` needs to be available outside the `java` module, keep it `pub` in `mod.rs`; otherwise you can make it `pub(crate)` or `pub(super)` and adjust imports accordingly.
These additional changes ensure both `detection.rs` and `validation.rs` use the same implementation, eliminating duplication and the risk of divergence.
</issue_to_address>
### Comment 4
<location> `src-tauri/src/core/java/providers/adoptium.rs:96-13` </location>
<code_context>
+ for major_version in &available.available_releases {
</code_context>
<issue_to_address>
**suggestion (performance):** Catalog fetching performs many HTTP requests sequentially; this could be slow when many versions are available.
Each major-version/image-type pair triggers its own `GET` in a nested loop, all awaited one by one. On slower networks or with many `available_releases`, this will noticeably delay catalog loading. Consider using `futures::stream` / `join_all` with a bounded concurrency level so multiple requests can be fetched in parallel.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Resolved conflicts: - Merged Instance struct fields: kept java_path_override from refactor and added jvm_args_override & memory_override from upstream - Removed upstream's java.rs as it has been refactored into java/ module directory - Updated duplicate_instance to include all new fields Reviewed-by: Claude Sonnet 4.5
There was a problem hiding this comment.
Pull request overview
This PR refactors the Java detection system from a monolithic 1087-line java.rs file into a modular architecture with separate concerns for detection, validation, persistence, and provider abstraction. The refactoring converts synchronous Java detection functions to async operations and introduces a provider pattern to support multiple Java distribution sources.
Changes:
- Splits
core/java.rsinto modular structure:detection.rs,validation.rs,persistence.rs,priority.rs,provider.rs, andproviders/adoptium.rs - Converts all Java-related functions to async with proper
.awaitusage throughoutmain.rs - Adds new
JavaInstallationfields (arch,vendor,source) for richer Java information - Introduces instance-level Java path override field in
Instancestruct - Creates provider trait pattern for future support of multiple Java download sources
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/src/main.rs | Adds .await to all Java function calls, adds new detect_all_java_installations command and backward-compatible detect_java alias |
| src-tauri/src/core/mod.rs | Re-exports JavaInstallation at core module level |
| src-tauri/src/core/java/validation.rs | New module for Java validation logic with async check_java_installation, version/arch/vendor parsing |
| src-tauri/src/core/java/providers/mod.rs | Module declaration for provider implementations |
| src-tauri/src/core/java/providers/adoptium.rs | Adoptium provider implementation with trait methods for catalog/release fetching |
| src-tauri/src/core/java/provider.rs | Trait definition for Java download providers following SOLID principles |
| src-tauri/src/core/java/priority.rs | Java resolution priority logic (instance → global → preferred → auto-detect) |
| src-tauri/src/core/java/persistence.rs | Java configuration persistence with user-defined paths and preferred Java tracking |
| src-tauri/src/core/java/mod.rs | Main Java module consolidating detection, catalog, download, and installation logic |
| src-tauri/src/core/java/detection.rs | System Java detection across Linux/macOS/Windows paths and environment variables |
| src-tauri/src/core/java.rs | Complete removal of monolithic 1087-line file (replaced by modular structure) |
| src-tauri/src/core/instance.rs | Adds java_path_override field for per-instance Java configuration |
| use std::time::Duration; | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| use std::os::windows::process::CommandExt; | ||
|
|
||
| use super::JavaInstallation; | ||
|
|
||
| const JAVA_CHECK_TIMEOUT: Duration = Duration::from_secs(5); | ||
|
|
There was a problem hiding this comment.
The constant JAVA_CHECK_TIMEOUT is defined but never used in this file. If this was intended for implementing timeout functionality in check_java_installation, it should be utilized. Otherwise, it should be removed to avoid confusion.
| use std::time::Duration; | |
| #[cfg(target_os = "windows")] | |
| use std::os::windows::process::CommandExt; | |
| use super::JavaInstallation; | |
| const JAVA_CHECK_TIMEOUT: Duration = Duration::from_secs(5); | |
| #[cfg(target_os = "windows")] | |
| use std::os::windows::process::CommandExt; | |
| use super::JavaInstallation; |
src-tauri/src/core/java/detection.rs
Outdated
| // Only attempt 'which' or 'where' if is not Windows | ||
| // CAUTION: linux 'which' may return symlinks, so we need to canonicalize later |
There was a problem hiding this comment.
The comment "Only attempt 'which' or 'where' if is not Windows" is inaccurate. The code actually runs the command on all platforms, using "where" on Windows and "which" on other systems (line 30). The comment should be updated or removed to avoid confusion.
| // Only attempt 'which' or 'where' if is not Windows | |
| // CAUTION: linux 'which' may return symlinks, so we need to canonicalize later | |
| // Attempt to locate 'java' using 'where' on Windows or 'which' on Unix-like systems | |
| // CAUTION: on Linux and other Unix-like systems, 'which' may return symlinks, so we canonicalize later |
src-tauri/src/core/java/detection.rs
Outdated
| } | ||
| } | ||
|
|
||
| let home = std::env::var("HOME").unwrap_or_default(); |
There was a problem hiding this comment.
The variable home is declared but never used. The find_sdkman_java() function already retrieves and uses the HOME environment variable internally, so this declaration is redundant and should be removed.
| let home = std::env::var("HOME").unwrap_or_default(); |
src-tauri/src/core/java/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub async fn is_java_compatible( | ||
| java_path: &str, | ||
| required_major_version: Option<u64>, | ||
| max_major_version: Option<u32>, | ||
| ) -> bool { | ||
| let java_path_buf = PathBuf::from(java_path); | ||
| if let Some(java) = validation::check_java_installation(&java_path_buf).await { | ||
| let major = validation::parse_java_version(&java.version); | ||
| let meets_min = if let Some(required) = required_major_version { | ||
| major >= required as u32 | ||
| } else { | ||
| true | ||
| }; | ||
| let meets_max = if let Some(max_version) = max_major_version { | ||
| major <= max_version | ||
| } else { | ||
| true | ||
| }; | ||
| meets_min && meets_max | ||
| } else { |
There was a problem hiding this comment.
The is_version_compatible function duplicates the same logic found in priority.rs:52-72. Both implementations perform identical version compatibility checks. Consider consolidating this logic into a single location, possibly by making priority::is_version_compatible public and reusing it here, or by extracting it to the validation module.
| #[cfg(target_os = "windows")] | ||
| { | ||
| let s = path.to_string_lossy().to_string(); | ||
| if s.starts_with(r"\\?\") { | ||
| return PathBuf::from(&s[4..]); | ||
| } | ||
| } | ||
| path |
There was a problem hiding this comment.
The strip_unc_prefix function is duplicated in both validation.rs (lines 12-21) and detection.rs (lines 7-17). This violates the DRY (Don't Repeat Yourself) principle and creates maintenance overhead. Consider extracting this function to a shared utility module or making one module depend on the other's implementation.
| #[cfg(target_os = "windows")] | |
| { | |
| let s = path.to_string_lossy().to_string(); | |
| if s.starts_with(r"\\?\") { | |
| return PathBuf::from(&s[4..]); | |
| } | |
| } | |
| path | |
| super::detection::strip_unc_prefix(path) |
| fn run_which_command_with_timeout() -> Option<String> { | ||
| let mut cmd = Command::new(if cfg!(windows) { "where" } else { "which" }); | ||
| cmd.arg("java"); | ||
| #[cfg(target_os = "windows")] | ||
| // Hide the console window on Windows | ||
| cmd.creation_flags(0x08000000); | ||
|
|
||
| match cmd.output() { | ||
| Ok(output) => { | ||
| if output.status.success() { | ||
| Some(String::from_utf8_lossy(&output.stdout).to_string()) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| Err(_) => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
The function name run_which_command_with_timeout suggests timeout handling, but the implementation doesn't actually implement any timeout mechanism. The Command::output() call blocks indefinitely if the command hangs. Consider either:
- Renaming the function to remove the misleading "_with_timeout" suffix, or
- Implementing actual timeout functionality using
tokio::time::timeoutor a similar mechanism
This could be important for robustness if a Java installation is corrupted or the system is under heavy load.
| cached_at: now, | ||
| }; | ||
|
|
||
| let _ = super::super::save_catalog_cache(app_handle, &catalog); |
There was a problem hiding this comment.
The result of save_catalog_cache is intentionally ignored with let _ =. While this might be acceptable if cache saving failure shouldn't block the operation, consider at least logging the error for debugging purposes. A failed cache write could indicate disk space issues or permission problems that administrators should be aware of.
| let _ = super::super::save_catalog_cache(app_handle, &catalog); | |
| if let Err(e) = super::super::save_catalog_cache(app_handle, &catalog) { | |
| eprintln!("Failed to save Java catalog cache: {}", e); | |
| } |
| #[cfg(target_os = "windows")] | ||
| { | ||
| let s = path.to_string_lossy().to_string(); | ||
| if s.starts_with(r"\\?\") { | ||
| return PathBuf::from(&s[4..]); | ||
| } | ||
| } | ||
| path | ||
| } | ||
|
|
||
| pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> { | ||
| let path = path.clone(); | ||
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | ||
| .await | ||
| .ok()? | ||
| } | ||
|
|
||
| fn check_java_installation_blocking(path: &PathBuf) -> Option<JavaInstallation> { | ||
| let mut cmd = Command::new(path); | ||
| cmd.arg("-version"); | ||
| #[cfg(target_os = "windows")] | ||
| cmd.creation_flags(0x08000000); | ||
|
|
||
| let output = cmd.output().ok()?; | ||
|
|
||
| let version_output = String::from_utf8_lossy(&output.stderr); | ||
|
|
||
| let version = parse_version_string(&version_output)?; | ||
| let arch = extract_architecture(&version_output); | ||
| let vendor = extract_vendor(&version_output); | ||
| let is_64bit = version_output.contains("64-Bit"); | ||
|
|
||
| Some(JavaInstallation { | ||
| path: path.to_string_lossy().to_string(), | ||
| version, | ||
| arch, | ||
| vendor, | ||
| source: "system".to_string(), | ||
| is_64bit, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
This file uses tabs for indentation, which is inconsistent with the rest of the Rust codebase that uses 4 spaces (see files like src-tauri/src/core/fabric.rs, src-tauri/src/core/auth.rs, src-tauri/src/main.rs). The project's pre-commit configuration includes cargo fmt which should enforce consistent formatting. Run cargo fmt to automatically fix the indentation to match project conventions.
| #[cfg(target_os = "windows")] | |
| { | |
| let s = path.to_string_lossy().to_string(); | |
| if s.starts_with(r"\\?\") { | |
| return PathBuf::from(&s[4..]); | |
| } | |
| } | |
| path | |
| } | |
| pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> { | |
| let path = path.clone(); | |
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | |
| .await | |
| .ok()? | |
| } | |
| fn check_java_installation_blocking(path: &PathBuf) -> Option<JavaInstallation> { | |
| let mut cmd = Command::new(path); | |
| cmd.arg("-version"); | |
| #[cfg(target_os = "windows")] | |
| cmd.creation_flags(0x08000000); | |
| let output = cmd.output().ok()?; | |
| let version_output = String::from_utf8_lossy(&output.stderr); | |
| let version = parse_version_string(&version_output)?; | |
| let arch = extract_architecture(&version_output); | |
| let vendor = extract_vendor(&version_output); | |
| let is_64bit = version_output.contains("64-Bit"); | |
| Some(JavaInstallation { | |
| path: path.to_string_lossy().to_string(), | |
| version, | |
| arch, | |
| vendor, | |
| source: "system".to_string(), | |
| is_64bit, | |
| }) | |
| } | |
| #[cfg(target_os = "windows")] | |
| { | |
| let s = path.to_string_lossy().to_string(); | |
| if s.starts_with(r"\\?\") { | |
| return PathBuf::from(&s[4..]); | |
| } | |
| } | |
| path | |
| } | |
| pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> { | |
| let path = path.clone(); | |
| tokio::task::spawn_blocking(move || { | |
| check_java_installation_blocking(&path) | |
| }) | |
| .await | |
| .ok()? | |
| } | |
| fn check_java_installation_blocking(path: &PathBuf) -> Option<JavaInstallation> { | |
| let mut cmd = Command::new(path); | |
| cmd.arg("-version"); | |
| #[cfg(target_os = "windows")] | |
| cmd.creation_flags(0x08000000); | |
| let output = cmd.output().ok()?; | |
| let version_output = String::from_utf8_lossy(&output.stderr); | |
| let version = parse_version_string(&version_output)?; | |
| let arch = extract_architecture(&version_output); | |
| let vendor = extract_vendor(&version_output); | |
| let is_64bit = version_output.contains("64-Bit"); | |
| Some(JavaInstallation { | |
| path: path.to_string_lossy().to_string(), | |
| version, | |
| arch, | |
| vendor, | |
| source: "system".to_string(), | |
| is_64bit, | |
| }) |
| } | ||
| } | ||
|
|
||
| pub async fn detect_java_installations() -> Vec<JavaInstallation> { | ||
| let mut installations = Vec::new(); | ||
| let candidates = detection::get_java_candidates(); | ||
|
|
||
| for candidate in candidates { | ||
| if let Some(java) = validation::check_java_installation(&candidate).await { | ||
| if !installations | ||
| .iter() | ||
| .any(|j: &JavaInstallation| j.path == java.path) | ||
| { | ||
| installations.push(java); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| installations.sort_by(|a, b| { | ||
| let v_a = validation::parse_java_version(&a.version); | ||
| let v_b = validation::parse_java_version(&b.version); | ||
| v_b.cmp(&v_a) | ||
| }); |
There was a problem hiding this comment.
The user_defined_paths field in JavaConfig is not being utilized during Java detection. The detect_java_installations function only checks system-default paths through detection::get_java_candidates(), but doesn't include user-defined paths that may have been added via persistence::add_user_defined_path. This means users cannot add custom Java installation paths, making the persistence functionality incomplete.
Consider adding code to check user_defined_paths from the JavaConfig and validate those paths before adding them to the installations list.
| if let Some(cached) = super::super::load_cached_catalog(app_handle) { | ||
| return Ok(cached); | ||
| } |
There was a problem hiding this comment.
The cache access path super::super::load_cached_catalog(app_handle) uses a fragile double-parent reference. If the module structure changes (e.g., moving providers to a different location), this will break. Consider using an absolute path like crate::core::java::load_cached_catalog(app_handle) or re-exporting the cache functions at a stable location for better maintainability.
- Centralize strip_unc_prefix into java/mod.rs to eliminate duplication across detection.rs and validation.rs - Remove unused JAVA_CHECK_TIMEOUT constant from validation.rs - Implement actual timeout mechanism in run_which_command_with_timeout() using try_wait() loop - Parallelize Adoptium API requests for better catalog fetch performance Fixes: - Multiple strip_unc_prefix implementations consolidated - Timeout constant now properly enforced in which/where command execution - Catalog fetching now uses concurrent tokio::spawn tasks instead of sequential await Reviewed-by: Claude Sonnet 4.5
- Add #[allow(dead_code)] attributes to utility functions - Improve 64-bit detection with case-insensitive check - Support aarch64 architecture in bitness detection - Add TODO for future vendor expansion Reviewed-by: Claude Sonnet 4.5
Replace potentially panicking unwrap() call with expect() that includes a descriptive error message to aid debugging if the edge case occurs. Reviewed-by: Claude Sonnet 4.5
|
|
||
| #[allow(dead_code)] | ||
| pub fn add_user_defined_path(app_handle: &AppHandle, path: String) -> Result<(), String> { | ||
| let mut config = load_java_config(app_handle); | ||
| if !config.user_defined_paths.contains(&path) { | ||
| config.user_defined_paths.push(path); | ||
| } | ||
| save_java_config(app_handle, &config) | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn remove_user_defined_path(app_handle: &AppHandle, path: &str) -> Result<(), String> { | ||
| let mut config = load_java_config(app_handle); | ||
| config.user_defined_paths.retain(|p| p != path); | ||
| save_java_config(app_handle, &config) | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn set_preferred_java_path(app_handle: &AppHandle, path: Option<String>) -> Result<(), String> { | ||
| let mut config = load_java_config(app_handle); | ||
| config.preferred_java_path = path; | ||
| save_java_config(app_handle, &config) | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn get_preferred_java_path(app_handle: &AppHandle) -> Option<String> { | ||
| let config = load_java_config(app_handle); | ||
| config.preferred_java_path | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub fn update_last_detection_time(app_handle: &AppHandle) -> Result<(), String> { | ||
| let mut config = load_java_config(app_handle); | ||
| config.last_detection_time = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(); | ||
| save_java_config(app_handle, &config) | ||
| } |
There was a problem hiding this comment.
Multiple persistence functions (add_user_defined_path, remove_user_defined_path, set_preferred_java_path, update_last_detection_time) are marked with #[allow(dead_code)] and are not used anywhere in the codebase. These appear to be part of a planned feature for user-managed Java paths that isn't fully implemented yet. The JavaConfig struct fields (user_defined_paths, preferred_java_path, last_detection_time) are defined but never populated. Consider either implementing the full feature or removing this unused infrastructure to reduce maintenance burden.
| #[allow(dead_code)] | |
| pub fn add_user_defined_path(app_handle: &AppHandle, path: String) -> Result<(), String> { | |
| let mut config = load_java_config(app_handle); | |
| if !config.user_defined_paths.contains(&path) { | |
| config.user_defined_paths.push(path); | |
| } | |
| save_java_config(app_handle, &config) | |
| } | |
| #[allow(dead_code)] | |
| pub fn remove_user_defined_path(app_handle: &AppHandle, path: &str) -> Result<(), String> { | |
| let mut config = load_java_config(app_handle); | |
| config.user_defined_paths.retain(|p| p != path); | |
| save_java_config(app_handle, &config) | |
| } | |
| #[allow(dead_code)] | |
| pub fn set_preferred_java_path(app_handle: &AppHandle, path: Option<String>) -> Result<(), String> { | |
| let mut config = load_java_config(app_handle); | |
| config.preferred_java_path = path; | |
| save_java_config(app_handle, &config) | |
| } | |
| #[allow(dead_code)] | |
| pub fn get_preferred_java_path(app_handle: &AppHandle) -> Option<String> { | |
| let config = load_java_config(app_handle); | |
| config.preferred_java_path | |
| } | |
| #[allow(dead_code)] | |
| pub fn update_last_detection_time(app_handle: &AppHandle) -> Result<(), String> { | |
| let mut config = load_java_config(app_handle); | |
| config.last_detection_time = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_secs(); | |
| save_java_config(app_handle, &config) | |
| } |
| fn run_which_command_with_timeout() -> Option<String> { | ||
| let mut cmd = Command::new(if cfg!(windows) { "where" } else { "which" }); | ||
| cmd.arg("java"); | ||
| #[cfg(target_os = "windows")] | ||
| cmd.creation_flags(0x08000000); | ||
| cmd.stdout(Stdio::piped()); | ||
|
|
||
| let start = Instant::now(); | ||
| let mut child = cmd.spawn().ok()?; | ||
|
|
||
| loop { | ||
| match child.try_wait() { | ||
| Ok(Some(status)) => { | ||
| if status.success() { | ||
| let mut output = String::new(); | ||
| if let Some(mut stdout) = child.stdout.take() { | ||
| let _ = stdout.read_to_string(&mut output); | ||
| } | ||
| return Some(output); | ||
| } else { | ||
| let _ = child.wait(); | ||
| return None; | ||
| } | ||
| } | ||
| Ok(None) => { | ||
| if start.elapsed() >= WHICH_TIMEOUT { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return None; | ||
| } | ||
| sleep(Duration::from_millis(50)); | ||
| } | ||
| Err(_) => { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return None; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The run_which_command_with_timeout function uses std::thread::sleep in a polling loop, blocking the thread for 50ms intervals. Since this is called from the synchronous get_java_candidates function which is then called from async detect_java_installations, this blocking sleep could impact performance during Java detection. Consider using tokio::time::sleep and making this function async, or restructure the timeout mechanism to avoid blocking waits in an async context.
| pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> { | ||
| let path = path.clone(); | ||
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | ||
| .await | ||
| .ok()? | ||
| } | ||
|
|
There was a problem hiding this comment.
The path parameter is cloned before passing to spawn_blocking. Since PathBuf implements Clone and the parameter is already a reference, consider taking ownership of the PathBuf in the function signature instead of cloning a reference. This would avoid an unnecessary clone operation and make the API more ergonomic.
| pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> { | |
| let path = path.clone(); | |
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | |
| .await | |
| .ok()? | |
| } | |
| pub async fn check_java_installation(path: PathBuf) -> Option<JavaInstallation> { | |
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | |
| .await | |
| .ok()? | |
| } |
| pub notes: Option<String>, // 备注（可选） | ||
| pub mod_loader: Option<String>, // 模组加载器类型："fabric", "forge", "vanilla" | ||
| pub mod_loader_version: Option<String>, // 模组加载器版本 | ||
| pub java_path_override: Option<String>, // 实例级Java路径覆盖（可选） |
There was a problem hiding this comment.
The java_path_override field is added to the Instance struct but is not being used in the start_game function. The game launch logic (lines 203-288 in main.rs) only checks config.java_path and doesn't consider instance-level Java overrides. The priority resolution logic should integrate priority::resolve_java_for_launch which was designed to handle instance overrides, or alternatively, the start_game function needs to be updated to read and use the instance.java_path_override field before falling back to config.java_path.
src-tauri/src/core/java/detection.rs
Outdated
| } | ||
| } | ||
|
|
||
| let home = std::env::var("HOME").unwrap_or_default(); |
There was a problem hiding this comment.
The variable 'home' is assigned but never used in this scope. It appears to be a leftover from the refactoring where SDKMAN detection was moved to a separate function (find_sdkman_java). This unused variable should be removed.
| let home = std::env::var("HOME").unwrap_or_default(); |
| if let Ok(Some(release)) = task.await { | ||
| releases.push(release); |
There was a problem hiding this comment.
Task join errors are silently ignored with the pattern 'if let Ok(Some(release))'. If a task panics or fails to join, the error is discarded without logging. This could make debugging difficult if parallel fetches fail unexpectedly. Consider logging task join errors (Err case from task.await) to help diagnose issues during catalog fetching, especially since this involves multiple concurrent network requests.
| if let Ok(Some(release)) = task.await { | |
| releases.push(release); | |
| match task.await { | |
| Ok(Some(release)) => { | |
| releases.push(release); | |
| } | |
| Ok(None) => { | |
| // No release for this task; nothing to collect. | |
| } | |
| Err(e) => { | |
| eprintln!("AdoptiumProvider::fetch_catalog task join error: {:?}", e); | |
| } |
| pub async fn is_java_compatible( | ||
| java_path: &str, | ||
| required_major_version: Option<u64>, | ||
| max_major_version: Option<u32>, | ||
| ) -> bool { | ||
| let java_path_buf = PathBuf::from(java_path); | ||
| if let Some(java) = validation::check_java_installation(&java_path_buf).await { | ||
| let major = validation::parse_java_version(&java.version); | ||
| let meets_min = if let Some(required) = required_major_version { | ||
| major >= required as u32 | ||
| } else { | ||
| true | ||
| }; | ||
| let meets_max = if let Some(max_version) = max_major_version { | ||
| major <= max_version | ||
| } else { | ||
| true | ||
| }; | ||
| meets_min && meets_max | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
The version compatibility checking logic is duplicated between priority::is_version_compatible (lines 54-74 in priority.rs) and this function. This creates maintenance burden as any changes to compatibility logic need to be applied in both places. Consider extracting this logic into a shared helper function in the validation module, or having both functions delegate to priority::is_version_compatible.
| pub struct JavaInstallation { | ||
| pub path: String, | ||
| pub version: String, | ||
| pub arch: String, | ||
| pub vendor: String, | ||
| pub source: String, | ||
| pub is_64bit: bool, | ||
| } |
There was a problem hiding this comment.
The JavaInstallation struct has been updated to include new fields (arch, vendor, source), but the TypeScript interface at packages/ui/src/types/index.ts (lines 86-90) only defines the old fields (path, version, is_64bit). This will cause runtime errors when the frontend tries to deserialize JavaInstallation objects that include the new fields. The TypeScript interface should be updated to match the new Rust struct definition.
| pub async fn get_compatible_java( | ||
| app_handle: &AppHandle, | ||
| required_major_version: Option<u64>, | ||
| max_major_version: Option<u32>, | ||
| ) -> Option<JavaInstallation> { | ||
| let installations = detect_all_java_installations(app_handle).await; | ||
|
|
||
| if let Some(max_version) = max_major_version { | ||
| installations.into_iter().find(|java| { | ||
| let major = validation::parse_java_version(&java.version); | ||
| let meets_min = if let Some(required) = required_major_version { | ||
| major >= required as u32 | ||
| } else { | ||
| true | ||
| }; | ||
| meets_min && major <= max_version | ||
| }) | ||
| } else if let Some(required) = required_major_version { | ||
| installations.into_iter().find(|java| { | ||
| let major = validation::parse_java_version(&java.version); | ||
| major >= required as u32 | ||
| }) | ||
| } else { | ||
| installations.into_iter().next() | ||
| } | ||
| } |
There was a problem hiding this comment.
This function duplicates the version compatibility checking logic that also exists in is_java_compatible (lines 388-410) and priority::is_version_compatible. The nested if-else branches for checking version bounds appear in multiple places. Consider consolidating this into a single shared compatibility check function to reduce code duplication and improve maintainability.
src-tauri/src/core/java/priority.rs
Outdated
| #[allow(dead_code)] | ||
| pub async fn resolve_java_for_launch( | ||
| app_handle: &AppHandle, | ||
| instance_java_override: Option<&str>, | ||
| global_java_path: Option<&str>, | ||
| required_major_version: Option<u64>, | ||
| max_major_version: Option<u32>, | ||
| ) -> Option<JavaInstallation> { | ||
| if let Some(override_path) = instance_java_override { | ||
| if !override_path.is_empty() { | ||
| let path_buf = std::path::PathBuf::from(override_path); | ||
| if let Some(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Some(global_path) = global_java_path { | ||
| if !global_path.is_empty() { | ||
| let path_buf = std::path::PathBuf::from(global_path); | ||
| if let Some(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let preferred = persistence::get_preferred_java_path(app_handle); | ||
| if let Some(pref_path) = preferred { | ||
| let path_buf = std::path::PathBuf::from(&pref_path); | ||
| if let Some(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let installations = super::detect_all_java_installations(app_handle).await; | ||
| installations | ||
| .into_iter() | ||
| .find(|java| is_version_compatible(java, required_major_version, max_major_version)) | ||
| } |
There was a problem hiding this comment.
The resolve_java_for_launch function is marked with #[allow(dead_code)] and is never called. This suggests the priority resolution system described in the PR (instance override → global config → user-preferred → auto-detected) is not actually integrated into the game launch flow. If this is work-in-progress functionality, it should be noted in code comments or the PR description. Otherwise, this function should either be integrated or removed.
|
I will review this PR within the next week Thank you very much for your contribution |
|
@SourceryAI title |
sourcery-ai
bot
changed the title
|
lgtm, @HsiangNianian take a look? |
should test first function
|
|
To complete the |
test functionality
@fu050409 @HsiangNianian Need update listeners and event name to display the download progress correctly Though the path is correct and works properly when launching Path
----
C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java
PSPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java\te
murin-21-jdk
PSParentPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java
PSChildName : temurin-21-jdk
PSDrive : C
PSProvider : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : True
Name : temurin-21-jdk
FullName : C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java\temurin-21-jdk
Parent : java
Exists : True
Root : C:\
Extension :
CreationTime : 2026/1/26 18:32:09
CreationTimeUtc : 2026/1/26 17:32:09
LastAccessTime : 2026/1/26 18:32:12
LastAccessTimeUtc : 2026/1/26 17:32:12
LastWriteTime : 2026/1/26 18:32:09
LastWriteTimeUtc : 2026/1/26 17:32:09
Attributes : Directory
Mode : d-----
BaseName : temurin-21-jdk
Target : {}
LinkType :
PSPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java\te
murin-8-jdk
PSParentPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java
PSChildName : temurin-8-jdk
PSDrive : C
PSProvider : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : True
Name : temurin-8-jdk
FullName : C:\Users\begonia\AppData\Roaming\com.dropout.launcher\java\temurin-8-jdk
Parent : java
Exists : True
Root : C:\
Extension :
CreationTime : 2026/1/16 20:16:51
CreationTimeUtc : 2026/1/16 19:16:51
LastAccessTime : 2026/1/26 18:26:58
LastAccessTimeUtc : 2026/1/26 17:26:58
LastWriteTime : 2026/1/16 20:16:51
LastWriteTimeUtc : 2026/1/16 19:16:51
Attributes : Directory
Mode : d-----
BaseName : temurin-8-jdk
Target : {}
LinkType :sr2026-01-26.183250.mp4 |
…r handling - Extract version compatibility check into shared validation function - Remove duplicated version checking code across multiple modules - Simplify Java detection timeout logic in detection.rs - Expand vendor detection to support more JDK distributions (Dragonwell, Kona, Semeru, BiSheng, etc.) - Refactor start_game to use priority-based Java resolution - Improve error handling in Adoptium provider task collection Reviewed-by: Claude Sonnet 4.5
test functionality
|
write test in specific rust file and run cargo test |
Java 组件设计规范检查报告概述本报告对 ✅ 符合的设计原则1. 单一职责原则 (Single Responsibility Principle)状态: ✅ 符合 分析:
每个模块职责清晰，变更原因单一。 2. 显式依赖原则 (Explicit Dependencies Principle)状态: ✅ 符合 分析: // 好的例子 - 显式参数
pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation>
pub fn parse_java_version(version: &str) -> u32
pub async fn fetch_catalog(
&self,
app_handle: &AppHandle,
force_refresh: bool,
) -> Result<JavaCatalog, String>所有依赖都通过参数显式传入，没有隐式的全局状态或静态方法调用。 3. 关注点分离 (Separation of Concerns)状态: ✅ 符合 分析:
各层职责分离清晰，易于维护和测试。 4. 开闭原则 (Open-Closed Principle)状态: ✅ 符合 分析: // 通过 trait 实现扩展性
pub trait JavaProvider: Send + Sync {
async fn fetch_catalog(...) -> Result<JavaCatalog, String>;
async fn fetch_release(...) -> Result<JavaDownloadInfo, String>;
async fn available_versions(&self) -> Result<Vec<u32>, String>;
fn provider_name(&self) -> &'static str;
fn os_name(&self) -> &'static str;
fn arch_name(&self) -> &'static str;
fn install_prefix(&self) -> &'static str;
}通过 5. 接口隔离原则 (Interface Segregation Principle)状态: ✅ 符合 分析:
6. 依赖倒置原则 (Dependency Inversion Principle)状态: ✅ 符合 分析: // 高层模块依赖抽象
pub async fn fetch_java_catalog(
app_handle: &AppHandle,
force_refresh: bool,
) -> Result<JavaCatalog, String> {
let provider = AdoptiumProvider::new();
provider.fetch_catalog(app_handle, force_refresh).await
}通过 7. 快速失败原则 (Fail Fast)状态: ✅ 符合 分析: // 立即验证和返回错误
pub async fn check_java_installation(path: &PathBuf) -> Option<JavaInstallation> {
let mut cmd = Command::new(path);
cmd.arg("-version");
let output = cmd.output().ok()?; // 快速失败
let version_output = String::from_utf8_lossy(&output.stderr);
let version = parse_version_string(&version_output)?; // 快速失败
// ...
}使用 8. 不重复自己 (Don't Repeat Yourself)状态: ✅ 符合 分析:
避免了重复的字符串匹配和版本解析代码。 9. 持久化无关性 (Persistence Ignorance)状态: ✅ 符合 分析:
10. 明确告诉，不要询问 (Tell, Don't Ask)状态: ✅ 符合 分析: // 好的例子 - 告诉对象做什么
pub async fn resolve_java_for_launch(
app_handle: &AppHandle,
instance_java_override: Option<&str>,
global_java_path: Option<&str>,
required_major_version: Option<u64>,
max_major_version: Option<u32>,
) -> Option<JavaInstallation>调用者告诉系统需要什么版本的 Java，而不是查询所有 Java 然后自己过滤。 11. 保持简单 (Keep It Simple)状态: ✅ 符合 分析:
12. 架构敏捷性 (Architectural Agility)状态: ✅ 符合 分析:
13. 稳定依赖 (Stable Dependencies)状态: ✅ 符合 分析:
14. 一次且仅一次 (Once and Only Once)状态: ✅ 符合 分析:
15. 最少惊讶原则 (Principle of Least Astonishment)状态: ✅ 符合 分析:
16. Boy Scout 规则状态: ✅ 符合 分析:
|
| 原则 | 状态 | 分数 |
|---|---|---|
| 单一职责原则 | ✅ | 10/10 |
| 显式依赖原则 | ✅ | 10/10 |
| 关注点分离 | ✅ | 10/10 |
| 开闭原则 | ✅ | 9/10 |
| 接口隔离原则 | ✅ | 9/10 |
| 依赖倒置原则 | ✅ | 9/10 |
| 快速失败原则 | ✅ | 9/10 |
| 不重复自己 | ✅ | 9/10 |
| 持久化无关性 | ✅ | 10/10 |
| 明确告诉 | ✅ | 9/10 |
| 保持简单 | ✅ | 9/10 |
| 架构敏捷性 | ✅ | 9/10 |
| 稳定依赖 | ✅ | 10/10 |
| 一次且仅一次 | ✅ | 9/10 |
| 最少惊讶原则 | ✅ | 9/10 |
| Boy Scout 规则 | ✅ | 9/10 |
| 无效状态不可表示 | 6/10 | |
| 缓存策略 | 7/10 | |
| 错误处理一致性 | 7/10 | |
| 平台代码组织 | 7/10 | |
| 测试覆盖 | 5/10 | |
| 文档注释 | 6/10 | |
| 超时处理 | 7/10 | |
| 配置管理 | 6/10 |
总体评分: 8.2/10 ✅ 优秀
🎯 优先级改进建议
高优先级 (应立即处理)
- 添加单元测试 - 提高代码可靠性
- 统一错误处理 - 使用
Result<T, JavaError>替代混合的返回类型 - 无效状态不可表示 - 使用类型系统确保有效性
中优先级 (下一个迭代)
- 改进缓存策略 - 添加版本控制和自动清理
- 平台代码分离 - 将平台特定代码移到独立模块
- 添加文档注释 - 为所有公共 API 添加 doc 注释
低优先级 (长期改进)
- 超时处理现代化 - 使用
tokio::time::timeout - 配置管理 - 允许用户自定义参数
结论
src-tauri/src/core/java 组件整体设计良好，遵循了大多数 SOLID 原则和最佳实践。代码结构清晰，职责分离明确，易于维护和扩展。
主要改进空间在于：
- 类型系统的更好利用（无效状态不可表示）
- 错误处理的一致性
- 测试覆盖的完善
- 文档的补充
这些改进将进一步提升代码质量和可维护性。
Drafted by Sisyphus from OMO
Using model Claude 3.5 Sonnet
| let now = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(); |
There was a problem hiding this comment.
Another .unwrap() call on duration_since that could theoretically panic. This should be handled more gracefully, perhaps with unwrap_or(0) or by propagating an error.
| let now = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_secs(); | |
| let now = match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) { | |
| Ok(duration) => duration.as_secs(), | |
| Err(_) => 0, | |
| }; |
src-tauri/src/core/java/detection.rs
Outdated
| } | ||
| } | ||
|
|
||
| let home = std::env::var("HOME").unwrap_or_default(); |
There was a problem hiding this comment.
Unused variable 'home' is declared but never used in the Linux-specific section. This line appears to be left over from refactoring since the SDKMAN check doesn't need it. Consider removing this line.
| let home = std::env::var("HOME").unwrap_or_default(); |
| let vendor_name: HashMap<&str, &str> = [ | ||
| // Eclipse/Adoptium | ||
| ("temurin", "Temurin (Eclipse)"), | ||
| ("adoptium", "Eclipse Adoptium"), | ||
| // Amazon | ||
| ("corretto", "Corretto (Amazon)"), | ||
| ("amzn", "Corretto (Amazon)"), | ||
| // Alibaba | ||
| ("dragonwell", "Dragonwell (Alibaba)"), | ||
| ("albba", "Dragonwell (Alibaba)"), | ||
| // GraalVM | ||
| ("graalvm", "GraalVM"), | ||
| // Oracle | ||
| ("oracle", "Java SE Development Kit (Oracle)"), | ||
| // Tencent | ||
| ("kona", "Kona (Tencent)"), | ||
| // BellSoft | ||
| ("liberica", "Liberica (Bellsoft)"), | ||
| ("mandrel", "Mandrel (Red Hat)"), | ||
| // Microsoft | ||
| ("microsoft", "OpenJDK (Microsoft)"), | ||
| // SAP | ||
| ("sapmachine", "SapMachine (SAP)"), | ||
| // IBM | ||
| ("semeru", "Semeru (IBM)"), | ||
| ("sem", "Semeru (IBM)"), | ||
| // Azul | ||
| ("zulu", "Zulu (Azul Systems)"), | ||
| // Trava | ||
| ("trava", "Trava (Trava)"), | ||
| // Huawei | ||
| ("bisheng", "BiSheng (Huawei)"), | ||
| // Generic OpenJDK | ||
| ("openjdk", "OpenJDK"), | ||
| ] | ||
| .iter() | ||
| .cloned() | ||
| .collect(); |
There was a problem hiding this comment.
The vendor name HashMap is recreated on every call to extract_vendor. This is inefficient since the HashMap contains static data. Consider using once_cell::sync::Lazy or defining it as a static constant to avoid repeated allocations. This function is called during Java detection which can happen frequently.
| fn get_java_config_path(app_handle: &AppHandle) -> PathBuf { | ||
| app_handle | ||
| .path() | ||
| .app_data_dir() | ||
| .unwrap() | ||
| .join("java_config.json") |
There was a problem hiding this comment.
The .unwrap() call on app_data_dir() could panic if the app data directory cannot be determined. While this is unlikely in practice, it would be more robust to handle this error gracefully by returning a Result or using a fallback path.
| pub fn get_java_install_dir(app_handle: &AppHandle) -> PathBuf { | ||
| app_handle.path().app_data_dir().unwrap().join("java") | ||
| } | ||
|
|
||
| fn get_catalog_cache_path(app_handle: &AppHandle) -> PathBuf { | ||
| app_handle | ||
| .path() | ||
| .app_data_dir() | ||
| .unwrap() | ||
| .join("java_catalog_cache.json") |
There was a problem hiding this comment.
Multiple .unwrap() calls on app_data_dir() throughout this module could panic if the app data directory cannot be determined. Consider creating a helper function that returns a Result or handles the error more gracefully. This pattern appears in get_java_install_dir (line 98), get_catalog_cache_path (lines 104-105), and potentially other places.
| pub fn get_java_install_dir(app_handle: &AppHandle) -> PathBuf { | |
| app_handle.path().app_data_dir().unwrap().join("java") | |
| } | |
| fn get_catalog_cache_path(app_handle: &AppHandle) -> PathBuf { | |
| app_handle | |
| .path() | |
| .app_data_dir() | |
| .unwrap() | |
| .join("java_catalog_cache.json") | |
| /// Get the base application data directory, falling back to the OS temp dir | |
| /// if Tauri cannot determine an app-specific data directory. | |
| fn get_app_data_dir(app_handle: &AppHandle) -> PathBuf { | |
| app_handle | |
| .path() | |
| .app_data_dir() | |
| .unwrap_or_else(|| std::env::temp_dir()) | |
| } | |
| pub fn get_java_install_dir(app_handle: &AppHandle) -> PathBuf { | |
| get_app_data_dir(app_handle).join("java") | |
| } | |
| fn get_catalog_cache_path(app_handle: &AppHandle) -> PathBuf { | |
| get_app_data_dir(app_handle).join("java_catalog_cache.json") |
| let now = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(); |
There was a problem hiding this comment.
The .unwrap() call on duration_since could theoretically panic if system time is before UNIX_EPOCH or if there's a clock adjustment. While rare, it would be more robust to handle this with unwrap_or(0) or return an error.
| let now = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_secs(); | |
| let now = match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) { | |
| Ok(duration) => duration.as_secs(), | |
| // If system time is before UNIX_EPOCH or there is a clock issue, | |
| // treat the cache as invalid instead of panicking. | |
| Err(_) => return None, | |
| }; |
| const WHICH_TIMEOUT: Duration = Duration::from_secs(2); | ||
|
|
||
| pub fn find_sdkman_java() -> Option<PathBuf> { | ||
| let home = std::env::var("HOME").ok()?; | ||
| let sdkman_path = PathBuf::from(&home).join(".sdkman/candidates/java/current/bin/java"); | ||
| if sdkman_path.exists() { | ||
| Some(sdkman_path) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| fn run_which_command_with_timeout() -> Option<String> { | ||
| let mut cmd = Command::new(if cfg!(windows) { "where" } else { "which" }); | ||
| cmd.arg("java"); | ||
| // Hide console window | ||
| #[cfg(target_os = "windows")] | ||
| cmd.creation_flags(0x08000000); | ||
| cmd.stdout(Stdio::piped()); | ||
|
|
||
| let mut child = cmd.spawn().ok()?; | ||
|
|
||
| loop { | ||
| match child.try_wait() { | ||
| Ok(Some(status)) => { | ||
| if status.success() { | ||
| let mut output = String::new(); | ||
| if let Some(mut stdout) = child.stdout.take() { | ||
| let _ = stdout.read_to_string(&mut output); | ||
| } | ||
| return Some(output); | ||
| } else { | ||
| let _ = child.wait(); | ||
| return None; | ||
| } | ||
| } | ||
| Ok(None) => { | ||
| std::thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| Err(_) => { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return None; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The WHICH_TIMEOUT constant is defined but never used. The run_which_command_with_timeout function implements a timeout mechanism with busy-waiting (50ms sleep), but it never checks against the defined timeout duration. This could cause the function to hang indefinitely if the which/where command doesn't complete. Consider adding actual timeout enforcement using the constant or removing it if not needed.
| // Check if configured Java is compatible | ||
| // Resolve Java using priority-based resolution | ||
| // Priority: instance override > global config > user preference > auto-detect | ||
| // TODO: refactor into a separate function |
There was a problem hiding this comment.
The TODO comment indicates that this logic should be refactored into a separate function, but it hasn't been done. Since a resolve_java_for_launch function already exists in the priority module and is being called here, this TODO comment should either be removed (as the refactoring is complete) or clarified if additional refactoring is still needed.
| // TODO: refactor into a separate function |
| std::fs::create_dir_all( | ||
| config_path | ||
| .parent() | ||
| .expect("Java config path should have a parent directory"), | ||
| ) | ||
| .map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Using .expect() with a descriptive message here could panic. Since this function returns a Result, it would be better to handle this error by returning an error instead of panicking. Consider using .ok_or_else(|| "...".to_string())? to propagate the error.
| std::fs::create_dir_all( | |
| config_path | |
| .parent() | |
| .expect("Java config path should have a parent directory"), | |
| ) | |
| .map_err(|e| e.to_string())?; | |
| let parent_dir = config_path | |
| .parent() | |
| .ok_or_else(|| "Java config path should have a parent directory".to_string())?; | |
| std::fs::create_dir_all(parent_dir).map_err(|e| e.to_string())?; |
| created_at: std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(), |
There was a problem hiding this comment.
Another .unwrap() call on duration_since that could theoretically panic. This should be handled more gracefully, perhaps with unwrap_or(0) or by propagating an error.
- Added support for detecting Java installations from SDKMAN! in `find_sdkman_java`. - Improved `run_which_command_with_timeout` to handle command timeouts gracefully. - Introduced a unified `JavaError` enum for consistent error handling across Java operations. - Updated functions to return `Result` types instead of `Option` for better error reporting. - Enhanced `load_cached_catalog` and `save_catalog_cache` to use `JavaError`. - Refactored `fetch_java_catalog`, `fetch_java_release`, and `fetch_available_versions` to return `JavaError`. - Improved validation functions to return detailed errors when checking Java installations. - Added tests for version parsing and compatibility checks. - Updated `resolve_java_for_launch` to handle instance-specific and global Java paths.
| // Task completed but returned None, should not happen in current implementation | ||
| } | ||
| Err(e) => { | ||
| eprintln!("AdoptiumProvider::fetch_catalog task join error: {:?}", e); |
There was a problem hiding this comment.
The eprintln! on line 186 writes to stderr but uses a debug format. In production builds, this won't be visible to users and might be lost. Additionally, task join errors are usually programming errors (like panics in spawned tasks), not normal operational errors.
Consider using a proper logging framework or emitting an event to notify the user/frontend of the error. Alternatively, if task join failures are not expected and indicate bugs, consider propagating them as errors rather than silently continuing.
| eprintln!("AdoptiumProvider::fetch_catalog task join error: {:?}", e); | |
| return Err(JavaError::NetworkError(format!( | |
| "Failed to join Adoptium catalog fetch task: {}", | |
| e | |
| ))); |
| pub async fn check_java_installation(path: &PathBuf) -> Result<JavaInstallation, JavaError> { | ||
| if !path.exists() { | ||
| return Err(JavaError::NotFound); | ||
| } | ||
|
|
||
| let path = path.clone(); | ||
| tokio::task::spawn_blocking(move || check_java_installation_blocking(&path)) | ||
| .await | ||
| .map_err(|e| JavaError::VerificationFailed(format!("Task join error: {}", e)))? | ||
| } |
There was a problem hiding this comment.
The spawned task in check_java_installation checks if the path exists before spawning, but this creates a TOCTOU (Time-of-Check to Time-of-Use) race condition. The path could be deleted or become inaccessible between the check on line 22 and the actual use in the spawned task on line 27.
Consider moving the existence check inside the spawned blocking task or handling the error case more robustly when the path doesn't exist during execution.
| Ok(content) => serde_json::from_str(&content).unwrap_or_default(), | ||
| Err(_) => JavaConfig::default(), |
There was a problem hiding this comment.
The load_java_config function silently returns JavaConfig::default() on any deserialization error. This could hide corruption in the config file or schema changes that break deserialization. Users might lose their Java preferences without any notification.
Consider logging deserialization errors to help with debugging, or at minimum distinguishing between "file doesn't exist" (expected on first run) and "file exists but can't be parsed" (indicates a problem).
| Ok(content) => serde_json::from_str(&content).unwrap_or_default(), | |
| Err(_) => JavaConfig::default(), | |
| Ok(content) => match serde_json::from_str(&content) { | |
| Ok(config) => config, | |
| Err(err) => { | |
| eprintln!( | |
| "Failed to parse Java config at {}: {}. Falling back to default configuration.", | |
| config_path.display(), | |
| err | |
| ); | |
| JavaConfig::default() | |
| } | |
| }, | |
| Err(err) => { | |
| eprintln!( | |
| "Failed to read Java config at {}: {}. Falling back to default configuration.", | |
| config_path.display(), | |
| err | |
| ); | |
| JavaConfig::default() | |
| } |
| pub version: String, | ||
| pub arch: String, | ||
| pub vendor: String, |
There was a problem hiding this comment.
The JavaInstallation struct now includes arch, vendor, and source fields that were not present in the old implementation. This is a breaking change in the API contract, as any frontend code expecting the old structure with only path, version, and is_64bit will fail to deserialize the new structure.
Consider documenting this breaking change in the PR description and migration guide, or handle backward compatibility by making the new fields optional with #[serde(default)].
| pub version: String, | |
| pub arch: String, | |
| pub vendor: String, | |
| pub version: String, | |
| #[serde(default)] | |
| pub arch: String, | |
| #[serde(default)] | |
| pub vendor: String, | |
| #[serde(default)] |
| // Check instance-specific override first | ||
| if let Some(override_path) = instance_java_override { | ||
| if !override_path.is_empty() { | ||
| let path_buf = std::path::PathBuf::from(override_path); | ||
| if let Ok(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the priority resolver, if the instance override path is provided but invalid or incompatible, the function silently falls through to check the global path. There's no logging or notification to the user that their instance-specific override was ignored.
Consider adding logging (via emit_log or similar) to inform users when their instance Java override is invalid or incompatible, helping with debugging configuration issues.
| pub async fn resolve_java_for_launch( | ||
| app_handle: &AppHandle, | ||
| instance_java_override: Option<&str>, | ||
| global_java_path: Option<&str>, | ||
| required_major_version: Option<u64>, | ||
| max_major_version: Option<u32>, | ||
| ) -> Option<JavaInstallation> { | ||
| // Check instance-specific override first | ||
| if let Some(override_path) = instance_java_override { | ||
| if !override_path.is_empty() { | ||
| let path_buf = std::path::PathBuf::from(override_path); | ||
| if let Ok(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check global Java path setting | ||
| if let Some(global_path) = global_java_path { | ||
| if !global_path.is_empty() { | ||
| let path_buf = std::path::PathBuf::from(global_path); | ||
| if let Ok(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check preferred Java path from config | ||
| let preferred = persistence::get_preferred_java_path(app_handle); | ||
| if let Some(pref_path) = preferred { | ||
| let path_buf = std::path::PathBuf::from(&pref_path); | ||
| if let Ok(java) = validation::check_java_installation(&path_buf).await { | ||
| if is_version_compatible(&java, required_major_version, max_major_version) { | ||
| return Some(java); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fall back to first compatible installation from system scan | ||
| let installations = super::detect_all_java_installations(app_handle).await; | ||
| installations | ||
| .into_iter() | ||
| .find(|java| is_version_compatible(java, required_major_version, max_major_version)) | ||
| } |
There was a problem hiding this comment.
The resolve_java_for_launch function implements critical priority-based resolution logic that determines which Java installation to use for game launching, but it has no test coverage. This is a core function that affects game startup reliability.
Since the validation module has tests and this function follows a clear priority order, consider adding unit tests for this function to verify: 1) priority order is respected, 2) version compatibility checks work correctly, 3) fallback to system scan works, 4) empty string paths are handled correctly.
| pub fn get_java_candidates() -> Vec<PathBuf> { | ||
| let mut candidates = Vec::new(); | ||
|
|
||
| // Try to find Java in PATH using 'which' or 'where' command with timeout | ||
| // CAUTION: linux 'which' may return symlinks, so we need to canonicalize later | ||
| if let Some(paths_str) = run_which_command_with_timeout() { | ||
| for line in paths_str.lines() { | ||
| let path = PathBuf::from(line.trim()); | ||
| if path.exists() { | ||
| let resolved = std::fs::canonicalize(&path).unwrap_or(path); | ||
| let final_path = strip_unc_prefix(resolved); | ||
| candidates.push(final_path); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| { | ||
| let linux_paths = [ | ||
| "/usr/lib/jvm", | ||
| "/usr/java", | ||
| "/opt/java", | ||
| "/opt/jdk", | ||
| "/opt/openjdk", | ||
| ]; | ||
|
|
||
| for base in &linux_paths { | ||
| if let Ok(entries) = std::fs::read_dir(base) { | ||
| for entry in entries.flatten() { | ||
| let java_path = entry.path().join("bin/java"); | ||
| if java_path.exists() { | ||
| candidates.push(java_path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check common SDKMAN! java candidates | ||
| if let Some(sdkman_java) = find_sdkman_java() { | ||
| candidates.push(sdkman_java); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| { | ||
| let mac_paths = [ | ||
| "/Library/Java/JavaVirtualMachines", | ||
| "/System/Library/Java/JavaVirtualMachines", | ||
| "/usr/local/opt/openjdk/bin/java", | ||
| "/opt/homebrew/opt/openjdk/bin/java", | ||
| ]; | ||
|
|
||
| for path in &mac_paths { | ||
| let p = PathBuf::from(path); | ||
| if p.is_dir() { | ||
| if let Ok(entries) = std::fs::read_dir(&p) { | ||
| for entry in entries.flatten() { | ||
| let java_path = entry.path().join("Contents/Home/bin/java"); | ||
| if java_path.exists() { | ||
| candidates.push(java_path); | ||
| } | ||
| } | ||
| } | ||
| } else if p.exists() { | ||
| candidates.push(p); | ||
| } | ||
| } | ||
|
|
||
| // Check common Homebrew java candidates for aarch64 macs | ||
| let homebrew_arm = PathBuf::from("/opt/homebrew/Cellar/openjdk"); | ||
| if homebrew_arm.exists() { | ||
| if let Ok(entries) = std::fs::read_dir(&homebrew_arm) { | ||
| for entry in entries.flatten() { | ||
| let java_path = entry | ||
| .path() | ||
| .join("libexec/openjdk.jdk/Contents/Home/bin/java"); | ||
| if java_path.exists() { | ||
| candidates.push(java_path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check common SDKMAN! java candidates | ||
| if let Some(sdkman_java) = find_sdkman_java() { | ||
| candidates.push(sdkman_java); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| { | ||
| let program_files = | ||
| std::env::var("ProgramFiles").unwrap_or_else(|_| "C:\\Program Files".to_string()); | ||
| let program_files_x86 = std::env::var("ProgramFiles(x86)") | ||
| .unwrap_or_else(|_| "C:\\Program Files (x86)".to_string()); | ||
| let local_app_data = std::env::var("LOCALAPPDATA").unwrap_or_default(); | ||
|
|
||
| // Common installation paths for various JDK distributions | ||
| let mut win_paths = vec![]; | ||
| for base in &[&program_files, &program_files_x86, &local_app_data] { | ||
| win_paths.push(format!("{}\\Java", base)); | ||
| win_paths.push(format!("{}\\Eclipse Adoptium", base)); | ||
| win_paths.push(format!("{}\\AdoptOpenJDK", base)); | ||
| win_paths.push(format!("{}\\Microsoft\\jdk", base)); | ||
| win_paths.push(format!("{}\\Zulu", base)); | ||
| win_paths.push(format!("{}\\Amazon Corretto", base)); | ||
| win_paths.push(format!("{}\\BellSoft\\LibericaJDK", base)); | ||
| win_paths.push(format!("{}\\Programs\\Eclipse Adoptium", base)); | ||
| } | ||
|
|
||
| for base in &win_paths { | ||
| let base_path = PathBuf::from(base); | ||
| if base_path.exists() { | ||
| if let Ok(entries) = std::fs::read_dir(&base_path) { | ||
| for entry in entries.flatten() { | ||
| let java_path = entry.path().join("bin\\java.exe"); | ||
| if java_path.exists() { | ||
| candidates.push(java_path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check JAVA_HOME environment variable | ||
| if let Ok(java_home) = std::env::var("JAVA_HOME") { | ||
| let bin_name = if cfg!(windows) { "java.exe" } else { "java" }; | ||
| let java_path = PathBuf::from(&java_home).join("bin").join(bin_name); | ||
| if java_path.exists() { | ||
| candidates.push(java_path); | ||
| } | ||
| } | ||
|
|
||
| candidates | ||
| } |
There was a problem hiding this comment.
The detection module contains complex platform-specific logic for finding Java installations across different operating systems, but has no test coverage. Given that it's a new module with timeout handling and path resolution logic that varies by OS, it would benefit from tests.
Consider adding tests for: 1) SDKMAN detection, 2) timeout behavior of which/where commands, 3) platform-specific path candidate generation, 4) path deduplication and canonicalization.
| let need_download = if archive_path.exists() { | ||
| if let Some(expected_checksum) = &info.checksum { | ||
| let data = std::fs::read(&archive_path)?; | ||
| !crate::core::downloader::verify_checksum(&data, Some(expected_checksum), None) |
There was a problem hiding this comment.
Reading the entire archive file into memory on line 356 could cause memory issues for large Java distributions (which can be hundreds of megabytes). This happens when verifying if a re-download is needed.
Consider using a streaming checksum verification approach or reading the file in chunks to reduce memory pressure, especially on systems with limited RAM.
| let need_download = if archive_path.exists() { | |
| if let Some(expected_checksum) = &info.checksum { | |
| let data = std::fs::read(&archive_path)?; | |
| !crate::core::downloader::verify_checksum(&data, Some(expected_checksum), None) | |
| fn compute_sha1_file(path: &std::path::Path) -> Result<String, JavaError> { | |
| use std::fs::File; | |
| use std::io::{BufReader, Read}; | |
| use sha1::{Digest, Sha1}; | |
| let file = File::open(path)?; | |
| let mut reader = BufReader::new(file); | |
| let mut hasher = Sha1::new(); | |
| let mut buffer = [0u8; 8 * 1024]; | |
| loop { | |
| let read_bytes = reader.read(&mut buffer)?; | |
| if read_bytes == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..read_bytes]); | |
| } | |
| Ok(format!("{:x}", hasher.finalize())) | |
| } | |
| fn compute_sha256_file(path: &std::path::Path) -> Result<String, JavaError> { | |
| use std::fs::File; | |
| use std::io::{BufReader, Read}; | |
| use sha2::{Digest, Sha256}; | |
| let file = File::open(path)?; | |
| let mut reader = BufReader::new(file); | |
| let mut hasher = Sha256::new(); | |
| let mut buffer = [0u8; 8 * 1024]; | |
| loop { | |
| let read_bytes = reader.read(&mut buffer)?; | |
| if read_bytes == 0 { | |
| break; | |
| } | |
| hasher.update(&buffer[..read_bytes]); | |
| } | |
| Ok(format!("{:x}", hasher.finalize())) | |
| } | |
| let need_download = if archive_path.exists() { | |
| if let Some(expected_checksum) = &info.checksum { | |
| let expected = expected_checksum.to_ascii_lowercase(); | |
| let computed = if expected.len() == 40 { | |
| compute_sha1_file(&archive_path)? | |
| } else { | |
| compute_sha256_file(&archive_path)? | |
| }; | |
| computed != expected |
| Err(_) => Some(JavaReleaseInfo { | ||
| major_version, | ||
| image_type, | ||
| version: format!("{}.x", major_version), | ||
| release_name: format!("jdk-{}", major_version), | ||
| release_date: None, | ||
| file_size: 0, | ||
| checksum: None, | ||
| download_url: String::new(), | ||
| is_lts, | ||
| is_available: false, | ||
| architecture: arch, | ||
| }), | ||
| } | ||
| }); |
There was a problem hiding this comment.
The error handling in the parallel fetch tasks silently swallows network errors and creates placeholder entries with is_available: false. While this prevents the entire catalog fetch from failing, it means users won't know if there was a transient network issue vs. the version genuinely not being available for their platform.
Consider distinguishing between network errors (which might be retried) and genuine "not available" responses (HTTP 404), and either logging the network errors or providing that information in the returned catalog structure.
| let asset = assets | ||
| .into_iter() | ||
| .next() | ||
| .ok_or_else(|| JavaError::NotFound)?; |
There was a problem hiding this comment.
The fetch_release method returns JavaError::NotFound when no assets are found, but this error variant has no message parameter. This makes it difficult to distinguish between different "not found" scenarios (version doesn't exist vs. not available for platform vs. API error).
Consider using a more descriptive error like JavaError::Other(format!("Java {} {} not found for current platform", major_version, image_type)) to provide better context to users.
| .ok_or_else(|| JavaError::NotFound)?; | |
| .ok_or_else(|| { | |
| JavaError::Other(format!( | |
| "Java {} {} not found for current platform (os={}, arch={})", | |
| major_version, image_type, os, arch | |
| )) | |
| })?; |
…eanup - Add CACHE_VERSION constant for cache format compatibility tracking - Add MAX_CACHE_SIZE_BYTES limit (10 MB) to prevent unbounded cache growth - Add cache_version field to JavaCatalog struct with default value - Implement cache version validation in load_cached_catalog() - Implement cache size enforcement in save_catalog_cache() - Add cleanup_expired_caches() for background cache cleanup - Add enforce_cache_size_limit() to validate cache file sizes - Add is_cache_version_compatible() helper function - Automatically clean up expired caches on load and clear operations - Validate cache version before using cached data Fixes: - Cache expiration without automatic cleanup (now cleaned on load) - Missing cache version control (now validates format compatibility) - Unbounded cache size growth (now limited to 10 MB) Reviewed-by: Claude 3.5 Sonnet
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
4 participants
Summary by Sourcery
Refactor Java runtime handling into an asynchronous, modular subsystem with support for detection, compatibility selection, downloading, and per-instance overrides.
New Features:
Enhancements: