Skip to content

Commit cbc4a78

Browse files
e-kotovCopilot
andauthored
fix macos picking up java stubs from /usr (#96)
* Introduce resolve_symlinks utility; normalize JAVA_HOME and use effective_java_home for cache keys; replace repeated symlink resolution with helper in java_find; remove redundant version-sorting * improve symlink path resolution robustness, and add rJava version caching tests. * fix tests * simplify effective Java home determination logic. * fix pkgdown * strenghen java version checks * more robust path detection on macos * improve reliability of java path * fix tests * fix ci live tests * add code to debug CI * more ci debug * ci debug * CI debug * test macos ci * Remove debug prints and add macOS DYLD_LIBRARY_PATH/JAVA_HOME handling for java version check * remove debug from tests * remove debug from tests * fix java 8 fails on macos ci * macos java stub fix * prevent regression on macos * add tests for new code * Update R/internal_utilities.R Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
1 parent 28000df commit cbc4a78

11 files changed

+336
-25
lines changed

R/internal_utilities.R

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,8 @@ get_libjvm_path <- function(java_home) {
227227
recursive = TRUE,
228228
full.names = TRUE
229229
)
230+
# Ignore AppleDouble files (._)
231+
all_files <- all_files[!grepl("/\\._", all_files)]
230232
if (length(all_files) > 0) lib_path <- all_files[1]
231233
}
232234
}
@@ -383,3 +385,23 @@ java_check_current_rjava_version <- function() {
383385

384386
return(major)
385387
}
388+
389+
#' Find the actual extracted directory, ignoring hidden/metadata files
390+
#'
391+
#' @param temp_dir The directory where files were extracted.
392+
#' @return The path to the first non-hidden directory found.
393+
#' @keywords internal
394+
._find_extracted_dir <- function(temp_dir) {
395+
# Ignore hidden files like .DS_Store or AppleDouble files (._)
396+
all_files <- list.files(temp_dir, full.names = TRUE)
397+
extracted_dirs <- all_files[
398+
dir.exists(all_files) & !grepl("^\\._", basename(all_files))
399+
]
400+
401+
if (length(extracted_dirs) == 0) {
402+
cli::cli_abort(
403+
"No directory found after unpacking the Java distribution at {.path {temp_dir}}"
404+
)
405+
}
406+
return(extracted_dirs[1])
407+
}

R/java_env.R

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -455,24 +455,67 @@ java_check_version_cmd <- function(
455455
return(FALSE)
456456
}
457457

458-
# Check if java executable exists in the PATH
459-
if (!nzchar(Sys.which("java"))) {
460-
if (!is.null(java_home)) {
461-
Sys.setenv(JAVA_HOME = old_java_home)
462-
Sys.setenv(PATH = old_path)
458+
# Get Java path - use explicit path if java_home is specified
459+
if (!is.null(java_home)) {
460+
# Direct check of the executable within the provided java_home
461+
# This avoids Sys.which potentially picking up system shims (common on macOS)
462+
# or other executables in the PATH
463+
464+
# Determine executable name based on OS
465+
exe_name <- if (.Platform$OS.type == "windows") "java.exe" else "java"
466+
java_bin_candidate <- file.path(java_home, "bin", exe_name)
467+
468+
if (file.exists(java_bin_candidate)) {
469+
java_bin <- java_bin_candidate
470+
} else {
471+
# Fallback to failing if not found in the expected location
472+
return(FALSE)
463473
}
464-
return(FALSE)
474+
} else {
475+
java_bin <- Sys.which("java")
476+
if (!nzchar(java_bin)) {
477+
return(FALSE)
478+
}
479+
}
480+
which_java <- java_bin
481+
482+
# On macOS, the 'java' launcher stub dynamically loads libjvm.dylib.
483+
# If another JDK (e.g., system Temurin) is in the default library search path,
484+
# the launcher may load the wrong JVM library even if JAVA_HOME is set.
485+
# We fix this by targeting DYLD_LIBRARY_PATH specifically for this subprocess.
486+
# Note: We do NOT set DYLD_LIBRARY_PATH globally in java_env_set() because:
487+
# 1. rJava's .jinit() loads libjvm.dylib directly from path, bypassing the stub.
488+
# 2. Global DYLD_* vars are restricted by macOS SIP and can cause side effects.
489+
if (Sys.info()[["sysname"]] == "Darwin" && !is.null(java_home)) {
490+
libjvm_path <- get_libjvm_path(java_home)
491+
if (!is.null(libjvm_path)) {
492+
lib_server <- dirname(libjvm_path)
493+
old_dyld_path <- Sys.getenv("DYLD_LIBRARY_PATH", unset = NA)
494+
if (is.na(old_dyld_path)) {
495+
on.exit(Sys.unsetenv("DYLD_LIBRARY_PATH"), add = TRUE)
496+
} else {
497+
on.exit(Sys.setenv(DYLD_LIBRARY_PATH = old_dyld_path), add = TRUE)
498+
}
499+
Sys.setenv(DYLD_LIBRARY_PATH = lib_server)
500+
}
501+
502+
# Also temporarily set JAVA_HOME for this check
503+
saved_java_home_for_darwin <- Sys.getenv("JAVA_HOME", unset = NA)
504+
if (is.na(saved_java_home_for_darwin)) {
505+
on.exit(Sys.unsetenv("JAVA_HOME"), add = TRUE)
506+
} else {
507+
on.exit(Sys.setenv(JAVA_HOME = saved_java_home_for_darwin), add = TRUE)
508+
}
509+
Sys.setenv(JAVA_HOME = java_home)
465510
}
466511

467-
# Get Java path and version info (without printing)
468-
which_java <- Sys.which("java")
469512
java_ver <- tryCatch(
470513
system2(
471-
"java",
514+
java_bin,
472515
args = "-version",
473516
stdout = TRUE,
474517
stderr = TRUE,
475-
timeout = 10
518+
timeout = 30
476519
),
477520
error = function(e) NULL
478521
)

R/java_find.R

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,12 @@
130130
# Filter out rJavaEnv cache paths - we only want true system installations
131131
candidates <- Filter(function(x) !is_rjavaenv_cache_path(x), candidates)
132132

133+
# macOS-specific: Filter out /usr to avoid system stubs (like /usr/bin/java)
134+
# which can cause timeouts and aren't real JDK homes
135+
if (os == "macos") {
136+
candidates <- Filter(function(x) x != "/usr", candidates)
137+
}
138+
133139
# Must contain bin/java to be valid - this prevents returning corrupted/empty JDK folders
134140
valid_homes <- Filter(
135141
function(x) {

R/java_unpack.R

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,14 @@ java_unpack <- function(
9797
}
9898

9999
# Safely find the extracted directory
100-
extracted_root_dir <- list.files(temp_dir, full.names = TRUE)[1]
100+
extracted_root_dir <- ._find_extracted_dir(temp_dir)
101+
101102
if (platform == "macos") {
102103
extracted_dir <- file.path(extracted_root_dir, "Contents", "Home")
104+
# Some distributions might not have Contents/Home if they were prepared differently
105+
if (!dir.exists(extracted_dir)) {
106+
extracted_dir <- extracted_root_dir
107+
}
103108
} else {
104109
extracted_dir <- extracted_root_dir
105110
}

tests/testthat/test-java_build_env_mocked.R

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@ test_that("java_build_env_set session mode outputs success message", {
1414
)
1515

1616
expect_message(
17-
java_build_env_set(
17+
suppressWarnings(java_build_env_set(
1818
java_home = "/mock/java",
1919
where = "session",
2020
quiet = FALSE
21-
),
21+
)),
2222
"Build environment variables set"
2323
)
2424
})
@@ -109,12 +109,12 @@ test_that("java_build_env_set project mode outputs success message", {
109109
)
110110

111111
expect_message(
112-
java_build_env_set(
112+
suppressWarnings(java_build_env_set(
113113
java_home = "/mock/java",
114114
where = "project",
115115
project_path = proj_dir,
116116
quiet = FALSE
117-
),
117+
)),
118118
"Build environment set in .Rprofile"
119119
)
120120
})
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Unit tests for java_check_version_cmd and internal helpers
2+
3+
test_that("._java_version_check_impl_original handles macOS DYLD_LIBRARY_PATH logic", {
4+
skip_on_cran()
5+
6+
# Mock Sys.info to return Darwin
7+
local_mocked_bindings(
8+
Sys.info = function() c(sysname = "Darwin"),
9+
.package = "base"
10+
)
11+
12+
# Mock get_libjvm_path
13+
local_mocked_bindings(
14+
get_libjvm_path = function(home) {
15+
file.path(home, "lib", "server", "libjvm.dylib")
16+
},
17+
.package = "rJavaEnv"
18+
)
19+
20+
# Mock java_env_set_session to prevent side effects
21+
local_mocked_bindings(
22+
java_env_set_session = function(...) TRUE,
23+
.package = "rJavaEnv"
24+
)
25+
26+
# Mock system2
27+
local_mocked_bindings(
28+
system2 = function(command, args, ...) {
29+
# Check environment within the "subprocess"
30+
dyld <- Sys.getenv("DYLD_LIBRARY_PATH")
31+
java_home <- Sys.getenv("JAVA_HOME")
32+
33+
# For verification purposes, return these values in the output
34+
return(c(
35+
paste0("DYLD_LIBRARY_PATH=", dyld),
36+
paste0("JAVA_HOME=", java_home),
37+
'openjdk version "21.0.1" 2023-10-17'
38+
))
39+
},
40+
.package = "base"
41+
)
42+
43+
# Mock file.exists logic
44+
local_mocked_bindings(
45+
file.exists = function(path) {
46+
# Always return true for this test
47+
return(TRUE)
48+
},
49+
.package = "base"
50+
)
51+
52+
java_home_mock <- "/mock/java/home"
53+
54+
result <- rJavaEnv:::._java_version_check_impl_original(
55+
java_home = java_home_mock
56+
)
57+
58+
output_text <- paste(result$java_version_output, collapse = "\n")
59+
60+
# Use regular expressions to match output
61+
expect_match(output_text, "/mock/java/home/lib/server")
62+
expect_match(output_text, "/mock/java/home")
63+
expect_match(result$major_version, "^21$")
64+
})
65+
66+
test_that("._java_version_check_impl_original returns FALSE when java binary not found", {
67+
java_home_mock <- "/mock/java/home"
68+
69+
local_mocked_bindings(
70+
java_env_set_session = function(...) TRUE,
71+
.package = "rJavaEnv"
72+
)
73+
74+
# Mock file.exists to return FALSE for the java binary check
75+
local_mocked_bindings(
76+
file.exists = function(paths) {
77+
if (grepl("bin/java", paths)) {
78+
return(FALSE)
79+
}
80+
TRUE
81+
},
82+
.package = "base"
83+
)
84+
85+
expect_false(rJavaEnv:::._java_version_check_impl_original(
86+
java_home = java_home_mock
87+
))
88+
})
89+
90+
test_that("._java_version_check_impl_original handles timeout (status 124)", {
91+
java_home_mock <- "/mock/java/home"
92+
93+
local_mocked_bindings(
94+
java_env_set_session = function(...) TRUE,
95+
.package = "rJavaEnv"
96+
)
97+
98+
local_mocked_bindings(
99+
file.exists = function(...) TRUE,
100+
.package = "base"
101+
)
102+
103+
local_mocked_bindings(
104+
system2 = function(...) {
105+
res <- character(0)
106+
attr(res, "status") <- 124
107+
res
108+
},
109+
.package = "base"
110+
)
111+
112+
expect_false(rJavaEnv:::._java_version_check_impl_original(
113+
java_home = java_home_mock
114+
))
115+
})
116+
117+
118+
test_that("._java_version_check_impl_original handles garbage output", {
119+
java_home_mock <- "/mock/java/home"
120+
121+
local_mocked_bindings(
122+
java_env_set_session = function(...) TRUE,
123+
.package = "rJavaEnv"
124+
)
125+
126+
local_mocked_bindings(
127+
file.exists = function(...) TRUE,
128+
.package = "base"
129+
)
130+
131+
local_mocked_bindings(
132+
system2 = function(...) {
133+
c("Some random error", "No version here")
134+
},
135+
.package = "base"
136+
)
137+
138+
expect_false(rJavaEnv:::._java_version_check_impl_original(
139+
java_home = java_home_mock
140+
))
141+
})

tests/testthat/test-java_env_version_impl.R

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,15 @@ test_that("._java_version_check_impl_original parses java -version output", {
99
bin_dir <- file.path(temp_java, "bin")
1010
dir.create(bin_dir, recursive = TRUE)
1111

12+
# Create a dummy java binary so Sys.which finds it
13+
# This is required for the new robust path detection logic
14+
java_exe <- file.path(bin_dir, "java")
15+
if (.Platform$OS.type == "windows") {
16+
java_exe <- paste0(java_exe, ".exe")
17+
}
18+
file.create(java_exe)
19+
Sys.chmod(java_exe, "755")
20+
1221
# Mock system2 to return known version string
1322
local_mocked_bindings(
1423
system2 = function(command, args, ...) {

tests/testthat/test-java_find.R

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,24 @@ test_that("java_find_system normalizes paths", {
9696
messy_path <- paste0(java_home, "///")
9797
withr::local_envvar(JAVA_HOME = messy_path)
9898

99+
# Mock version check to accept the dummy java
100+
local_mocked_bindings(
101+
._java_version_check_impl_original = function(java_home) {
102+
list(
103+
major_version = "17",
104+
java_home = java_home,
105+
java_path = "mock",
106+
java_version_output = "mock"
107+
)
108+
},
109+
.package = "rJavaEnv"
110+
)
111+
99112
result <- java_find_system(quiet = TRUE)
100113

101114
# Result should be normalized (no trailing slashes for files)
102115
expect_false(any(grepl("//", result$java_home)))
103-
expect_true(java_home %in% result$java_home)
116+
expect_true(normalizePath(java_home, winslash = "/") %in% result$java_home)
104117
})
105118

106119
test_that("java_find_system handles empty JAVA_HOME gracefully", {

tests/testthat/test-java_full-cycle-live.R

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ test_that("full download, install, check, and clear cycle works for all versions
7878
testthat::expect_true(dir.exists(java_home_path), info = context_info)
7979

8080
# --- Step C: Check and Verify ---
81+
8182
cli::cli_inform("-> Step 3: Verifying with java_check_version_cmd()...")
8283
cmd_version_result <- java_check_version_cmd(
8384
java_home = java_home_path,
@@ -88,15 +89,9 @@ test_that("full download, install, check, and clear cycle works for all versions
8889
java_version,
8990
info = context_info
9091
)
91-
if (cmd_version_result == java_version) {
92-
cli::cli_inform(
93-
"Successfully verified Java {java_version} with command line."
94-
)
95-
} else {
96-
cli::cli_alert_danger(
97-
"Command line verification failed for Java {java_version}."
98-
)
99-
}
92+
cli::cli_inform(
93+
"Successfully verified Java {java_version} with command line."
94+
)
10095

10196
cli::cli_inform("-> Step 4: Verifying with java_check_version_rjava()...")
10297
rjava_version_result <- java_check_version_rjava(

0 commit comments

Comments
 (0)