From c90d3a7a8ea0f93cc0300b78e23e9939640d9eab Mon Sep 17 00:00:00 2001 From: arookieofc Date: Mon, 9 Nov 2026 11:08:28 +0800 Subject: [PATCH] fix(java): run jdtls on a new-enough JDK behind Scoop shims and project JAVA_HOME (#2230) * fix(java): run jdtls on a new-enough JDK behind Scoop shims and project JAVA_HOME Two ways jdtls ended up on a JDK too old for its bundles (#2227): - jdtls_min_java resolved the command's own path to derive the install root, but a Scoop shim (shims\jdtls.exe) hides the release layout, so the bundles' real requirement fell back to JDTLS_MIN_JAVA (21) while a 1.64 install needs JavaSE 25. Extract the resolve/follow-shim flow from launch_directly into jdtls::script_for and share it. - configure_jdtls only wrote JAVA_HOME when the config had none, so a project JAVA_HOME (JDK 8 for compilation) won over the chosen server_jdk. Probe the configured value: a home too old or incomplete for the server is replaced by server_jdk or any usable JDK; an unresolvable explicit value stays (the user's call). * test(java): compare JDK homes against canonical paths jdk_at reports the canonicalized home (macOS /var -> /private/var, Windows 8.3 short names), which configure_jdtls now writes to JAVA_HOME. Have test_jdk canonicalize too, and assert the runtimes/JAVA_HOME values against the Jdk.home fields instead of raw fixtures.path() joins, which also differ on Windows when the join arg uses forward slashes. * test(jdtls): compare JAVA_HOME against the canonical jdk home test_jdk now canonicalizes home, so the duplicate-versions assertion must compare against server.home rather than the raw tempdir join, which on Windows carries 8.3 short names. --- crates/ide-sdk/src/java.rs | 137 +++++++++++++++++++++++++++--------- crates/ide-sdk/src/jdtls.rs | 39 ++++++---- 2 files changed, 126 insertions(+), 50 deletions(-) diff --git a/crates/ide-sdk/src/java.rs b/crates/ide-sdk/src/java.rs index 302558652..8403df5ba 100644 --- a/crates/ide-sdk/src/java.rs +++ b/crates/ide-sdk/src/java.rs @@ -279,11 +279,8 @@ pub fn jdtls_runtime(jdks: &[Jdk], min_java: u32) -> Option<&Jdk> { /// for 21, but the Eclipse platform it ships moves on (1.63 needs 25), and on /// an older JDK the server dies resolving its bundles. pub fn jdtls_min_java(cfg: &LanguageServerConfig) -> u32 { - let search = cfg.env.get("PATH").map(std::ffi::OsString::from).or_else(|| std::env::var_os("PATH")); - let required = search - .and_then(|s| ide_run::which::find_executable(&cfg.command, &s)) - .and_then(|exe| dunce::canonicalize(exe).ok()) - .and_then(|exe| Some(exe.parent()?.parent()?.to_path_buf())) + let required = crate::jdtls::script_for(cfg) + .and_then(|script| Some(script.parent()?.parent()?.to_path_buf())) .and_then(|home| bundles_min_java(&home)); required.map_or(JDTLS_MIN_JAVA, |v| v.max(JDTLS_MIN_JAVA)) } @@ -506,21 +503,31 @@ pub fn configure_jdtls( } } let jdks = all.as_slice(); - if cfg.launch_error.is_none() && !cfg.env.contains_key("JAVA_HOME") { - let runtime = match server_jdk { - Some(jdk) => server_jdk_error(jdk, min_java).map_or(Ok(jdk), Err), - None => jdtls_runtime(jdks, min_java).ok_or_else(|| missing_runtime_message(jdks, min_java)), - }; - match runtime { - Ok(jdk) => { - cfg.env.insert("JAVA_HOME".to_owned(), jdk.home.to_string_lossy().into_owned()); + if cfg.launch_error.is_none() { + // A `JAVA_HOME` the config inherits (e.g. the project's JDK 8 for + // compilation) must not run jdtls: a chosen `server_jdk`, else any + // usable JDK, replaces it when it probes as too old or incomplete for + // the server (issue #2227). A `JAVA_HOME` that cannot be probed is the + // user's call and is left untouched. + let env_jdk = cfg.env.get("JAVA_HOME").and_then(|home| jdk_at(Path::new(home))); + if cfg.env.contains_key("JAVA_HOME") && env_jdk.is_none() { + // Unresolvable explicit value: keep it, jdtls reports what it finds. + } else { + let existing = env_jdk.filter(|jdk| server_jdk_error(jdk, min_java).is_none()); + let runtime = if let Some(jdk) = server_jdk { + server_jdk_error(jdk, min_java).map_or(Ok(jdk), Err) + } else if let Some(jdk) = existing.as_ref() { + Ok(jdk) + } else { + jdtls_runtime(jdks, min_java).ok_or_else(|| missing_runtime_message(jdks, min_java)) + }; + match runtime { + Ok(jdk) => { + cfg.env.insert("JAVA_HOME".to_owned(), jdk.home.to_string_lossy().into_owned()); + } + Err(e) => cfg.launch_error = Some(e), } - Err(e) => cfg.launch_error = Some(e), } - } else if cfg.launch_error.is_none() - && let Some(jdk) = cfg.env.get("JAVA_HOME").and_then(|home| jdk_at(Path::new(home))) - { - cfg.launch_error = server_jdk_error(&jdk, min_java); } let mut runtimes = runtimes(&project_jdks); if let Some(project) = project_jdk.or_else(|| pick_jdk(&project_jdks, project_java_version(server_root))) { @@ -715,6 +722,10 @@ mod tests { .unwrap(); write_test_java(&home); std::fs::write(home.join("bin").join(crate::exe("javac")), "").unwrap(); + // `jdk_at` reports the canonical home; `configure_jdtls` writes it to + // JAVA_HOME, so tests compare against the same real path (macOS + // /var→/private/var, Windows 8.3 short names). + let home = dunce::canonicalize(home).unwrap(); Jdk { major, home } } @@ -833,7 +844,7 @@ mod tests { "name": "JavaSE-21", "path": pick_jdk(&installed, None).unwrap().home, "default": true }]) ); - assert_eq!(cfg.env["JAVA_HOME"], fixtures.path().join("custom-server-runtime").to_string_lossy()); + assert_eq!(cfg.env["JAVA_HOME"], server.home.to_string_lossy()); } #[test] @@ -1007,15 +1018,11 @@ mod tests { ); let opts = serde_json::to_value(cfg.initialization_options.clone().unwrap()).unwrap(); let rt = &opts["settings"]["java"]["configuration"]["runtimes"]; - assert_eq!(rt[0], json!({ "name": "JavaSE-1.8", "path": fixtures.path().join("jdk8") })); - assert_eq!(rt[1], json!({ "name": "JavaSE-21", "path": fixtures.path().join("jdk21a") })); - assert_eq!(rt[2], json!({ "name": "JavaSE-25", "path": fixtures.path().join("jdk25"), "default": true })); + assert_eq!(rt[0], json!({ "name": "JavaSE-1.8", "path": jdks[3].home })); + assert_eq!(rt[1], json!({ "name": "JavaSE-21", "path": jdks[1].home })); + assert_eq!(rt[2], json!({ "name": "JavaSE-25", "path": jdks[0].home, "default": true })); assert_eq!(opts["bundles"], json!(["/tools/java-debug/bin/com.microsoft.java.debug.plugin.jar"])); - assert_eq!( - cfg.env["JAVA_HOME"], - fixtures.path().join("jdk25").to_string_lossy(), - "first JDK new enough to run jdtls" - ); + assert_eq!(cfg.env["JAVA_HOME"], jdks[0].home.to_string_lossy(), "first JDK new enough to run jdtls"); assert_eq!(cfg.launch_error, None); // Deterministic (reconfigure compares configs) and idempotent. @@ -1049,8 +1056,8 @@ mod tests { ); let opts = serde_json::to_value(cfg.initialization_options.clone().unwrap()).unwrap(); let rt = &opts["settings"]["java"]["configuration"]["runtimes"]; - assert_eq!(rt[1], json!({ "name": "JavaSE-21", "path": fixtures.path().join("jdk21b"), "default": true })); - assert_eq!(cfg.env["JAVA_HOME"], fixtures.path().join("jdk21b").to_string_lossy()); + assert_eq!(rt[1], json!({ "name": "JavaSE-21", "path": project.home, "default": true })); + assert_eq!(cfg.env["JAVA_HOME"], project.home.to_string_lossy()); let mut other: LanguageServerConfig = toml::from_str("name = \"pyright\"\ncommand = \"x\"").unwrap(); configure_jdtls( @@ -1222,11 +1229,71 @@ mod tests { assert!(err.contains("Download JDK 25"), "{err}"); // Once a JDK 25 has been downloaded, Auto runs jdtls on it. - let downloaded = [installed.as_slice(), &[test_jdk(fixtures.path(), 25, "/tools/jdks/temurin-25")]].concat(); + let temurin = test_jdk(fixtures.path(), 25, "/tools/jdks/temurin-25"); + let downloaded = [installed.as_slice(), std::slice::from_ref(&temurin)].concat(); let mut cfg = jdtls(); configure_jdtls(&mut cfg, Path::new("/p"), Path::new("/data"), &downloaded, None, None, 25, &[], None); assert_eq!(cfg.launch_error, None); - assert_eq!(cfg.env["JAVA_HOME"], fixtures.path().join("tools/jdks/temurin-25").to_string_lossy()); + assert_eq!(cfg.env["JAVA_HOME"], temurin.home.to_string_lossy()); + } + + #[test] + fn issue_2227_a_project_java_home_too_old_for_the_server_is_replaced() { + // A project's JAVA_HOME (JDK 8 for compilation) must not run jdtls when + // the chosen server_jdk — or any usable JDK — is new enough. + let fixtures = tempfile::tempdir().unwrap(); + let jdtls = || -> LanguageServerConfig { toml::from_str("name = \"jdtls\"\ncommand = \"jdtls\"").unwrap() }; + let project = test_jdk(fixtures.path(), 8, "/jdk8"); + let server = test_jdk(fixtures.path(), 25, "/jdk25"); + + // The chosen server JDK overrides the project's JAVA_HOME. + let mut cfg = jdtls(); + cfg.env.insert("JAVA_HOME".into(), project.home.to_string_lossy().into_owned()); + configure_jdtls( + &mut cfg, + Path::new("/p"), + Path::new("/data"), + std::slice::from_ref(&project), + None, + Some(&server), + 25, + &[], + None, + ); + assert_eq!(cfg.launch_error, None); + assert_eq!(cfg.env["JAVA_HOME"], server.home.to_string_lossy()); + + // Auto picks any usable installed JDK over the stale JAVA_HOME. + let mut cfg = jdtls(); + cfg.env.insert("JAVA_HOME".into(), project.home.to_string_lossy().into_owned()); + let installed = [project.clone(), server.clone()]; + configure_jdtls(&mut cfg, Path::new("/p"), Path::new("/data"), &installed, None, None, 25, &[], None); + assert_eq!(cfg.launch_error, None); + assert_eq!(cfg.env["JAVA_HOME"], server.home.to_string_lossy()); + + // A JAVA_HOME new enough for the server stays. + let mut cfg = jdtls(); + cfg.env.insert("JAVA_HOME".into(), server.home.to_string_lossy().into_owned()); + configure_jdtls(&mut cfg, Path::new("/p"), Path::new("/data"), &installed, None, None, 25, &[], None); + assert_eq!(cfg.launch_error, None); + assert_eq!(cfg.env["JAVA_HOME"], server.home.to_string_lossy()); + + // Nothing usable to replace it with: the too-old JAVA_HOME loses to the + // same missing-runtime error a missing JAVA_HOME gives. + let mut cfg = jdtls(); + cfg.env.insert("JAVA_HOME".into(), project.home.to_string_lossy().into_owned()); + configure_jdtls( + &mut cfg, + Path::new("/p"), + Path::new("/data"), + std::slice::from_ref(&project), + None, + None, + 25, + &[], + None, + ); + assert!(cfg.launch_error.unwrap().contains("JDK 25 or newer"), "auto must say what to install"); } #[test] @@ -1288,23 +1355,23 @@ mod tests { let cfg = run(Some(&jdk(21, "/jdk21")), JDTLS_MIN_JAVA); assert_eq!( (cfg.env["JAVA_HOME"].clone(), cfg.launch_error), - (fixtures.path().join("jdk21").to_string_lossy().into_owned(), None) + (jdk(21, "/jdk21").home.to_string_lossy().into_owned(), None) ); let opts = serde_json::to_value(cfg.initialization_options.unwrap()).unwrap(); let rt = &opts["settings"]["java"]["configuration"]["runtimes"]; assert_eq!( rt[0], - json!({ "name": "JavaSE-17", "path": fixtures.path().join("jdk17"), "default": true }), + json!({ "name": "JavaSE-17", "path": project.home, "default": true }), "still compiles on 17" ); // Not set: the first installed JDK new enough, as before. - assert_eq!(run(None, JDTLS_MIN_JAVA).env["JAVA_HOME"], fixtures.path().join("jdk25").to_string_lossy()); + assert_eq!(run(None, JDTLS_MIN_JAVA).env["JAVA_HOME"], jdk(25, "/jdk25").home.to_string_lossy()); // A chosen JDK outside the scanned locations. let own = jdk(25, "/opt/own-jdk"); let cfg = run(Some(&own), 25); - assert_eq!(cfg.env["JAVA_HOME"], fixtures.path().join("opt/own-jdk").to_string_lossy()); + assert_eq!(cfg.env["JAVA_HOME"], own.home.to_string_lossy()); // Too old for this jdtls: the user's choice is not overridden, it is explained. let old = jdk(21, "/jdk21"); diff --git a/crates/ide-sdk/src/jdtls.rs b/crates/ide-sdk/src/jdtls.rs index a859d3932..a8426aa03 100644 --- a/crates/ide-sdk/src/jdtls.rs +++ b/crates/ide-sdk/src/jdtls.rs @@ -228,6 +228,29 @@ pub fn java_args(install: &JdtlsInstall, java_major: u32, script_args: &[String] args } +/// The release script a `jdtls` command on `PATH` resolves to: itself when it +/// already is one, or the script its Scoop shim launches. `None` for a command +/// that is neither a release script nor a jdtls Scoop shim. +/// +/// Shared with [`crate::java::jdtls_min_java`], which needs the same install +/// root to read the bundles' required Java version. +pub(crate) fn script_for(cfg: &LanguageServerConfig) -> Option { + if cfg.name != "jdtls" { + return None; + } + let search = cfg.env.get("PATH").map(std::ffi::OsString::from).or_else(|| std::env::var_os("PATH")); + let script = search.and_then(|s| ide_run::which::find_executable(&cfg.command, &s))?; + let resolved = dunce::canonicalize(script).ok()?; + let name = resolved.file_name().map(|n| n.to_string_lossy().to_ascii_lowercase()); + match name.as_deref() { + Some(n) if SCRIPTS.contains(&n) => Some(resolved), + // A scoop shim (`shims\jdtls.exe` from PATHEXT) hides the release + // layout behind itself (issue #794); follow its `.shim` to the script. + Some("jdtls.exe") if cfg!(windows) => shim_target(&resolved), + _ => None, + } +} + /// Scoop shims either point at the release script or run Python with the /// script in `args`. Paths may be quoted and contain spaces and backslashes. fn shim_target(shim_exe: &Path) -> Option { @@ -263,21 +286,7 @@ pub fn launch_directly(cfg: &mut LanguageServerConfig) { if cfg.name != "jdtls" || cfg.launch_error.is_some() { return; } - let search = cfg.env.get("PATH").map(std::ffi::OsString::from).or_else(|| std::env::var_os("PATH")); - let Some(script) = search.and_then(|s| ide_run::which::find_executable(&cfg.command, &s)) else { return }; - let Ok(resolved) = dunce::canonicalize(&script) else { return }; - let name = resolved.file_name().map(|n| n.to_string_lossy().to_ascii_lowercase()); - let script = match name.as_deref() { - Some(n) if SCRIPTS.contains(&n) => resolved, - // A scoop shim (`shims\jdtls.exe` from PATHEXT) hides the release layout - // behind itself, so the launch ran the shim's own command line and the - // configured JDK never applied (issue #794). - Some("jdtls.exe") if cfg!(windows) => match shim_target(&resolved) { - Some(target) => target, - None => return, - }, - _ => return, - }; + let Some(script) = script_for(cfg) else { return }; let Some(install) = jdtls_home(&script).and_then(|h| find_install(&h, std::env::consts::OS, std::env::consts::ARCH)) else {