From 97174047b0e0c3f3198eb8f4fc020834526c41fb Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 15:50:34 +0100 Subject: [PATCH 1/8] Use `R_HOME` in `r_command()` if defined --- crates/harp/src/command.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/harp/src/command.rs b/crates/harp/src/command.rs index d99f49edf..2c09ecfe6 100644 --- a/crates/harp/src/command.rs +++ b/crates/harp/src/command.rs @@ -24,9 +24,18 @@ where { assert!(COMMAND_R_LOCATIONS.len() > 0); + // If `R_HOME` is defined use that + let locations = COMMAND_R_LOCATIONS.map(|loc| { + if let Ok(r_home) = std::env::var("R_HOME") { + std::path::Path::new(&r_home).join(loc) + } else { + std::path::Path::new(loc).to_path_buf() + } + }); + let mut out = None; - for program in COMMAND_R_LOCATIONS.iter() { + for program in locations.into_iter() { // Build the `Command` from the user's function let mut command = Command::new(program); build(&mut command); From 80fd3dfa5a906bedb9af0399611f18c9b16b711a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 15:51:02 +0100 Subject: [PATCH 2/8] Define include, share, and doc environment variables --- CHANGELOG.md | 11 +++++++++++ crates/ark/src/interface.rs | 29 +++++++++++++++++++++++++++++ crates/ark/tests/kernel.rs | 21 +++++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14f72f641..8ee39f2ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ # 0.1.9000 +## 2024-11 + +- Jupyter: The following environment variables are now set in the same way that R does: + + - `R_SHARE_DIR` + - `R_INCLUDE_DIR` + - `R_DOC_DIR` + + This solves a number of problems in situations that depend on these variables being defined (https://github.com/posit-dev/positron/issues/3637). + + ## 2024-10 - Objects assigned at top level are now indexed, in addition to assigned functions. When a name is assigned multiple times, we now only index the first occurrence. This allows you to jump to the first "declaration" of the variable. In the future we'll improve this mechanism so that you can jump to the most recent assignment. diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 73797fbfb..a5b3f3c18 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -373,6 +373,35 @@ impl RMain { }, }; + // `R_HOME` is now defined no matter what and will be used by + // `r_command()`. Let's discover the other important environment + // variables set by R's shell script frontend. + // https://github.com/posit-dev/positron/issues/3637 + if let Ok(output) = + r_command(|command| { + // From https://github.com/rstudio/rstudio/blob/74696236/src/cpp/core/r_util/REnvironmentPosix.cpp#L506-L515 + command.arg("--vanilla").arg("-s").arg("-e").arg( + r#"cat(paste(R.home('share'), R.home('include'), R.home('doc'), sep=':'))"#, + ); + }) + { + if let Ok(vars) = String::from_utf8(output.stdout) { + let vars: Vec<&str> = vars.trim().split(':').collect(); + if vars.len() == 3 { + // Set the R env vars as the R shell script frontend would + unsafe { + std::env::set_var("R_SHARE_DIR", vars[0]); + std::env::set_var("R_INCLUDE_DIR", vars[1]); + std::env::set_var("R_DOC_DIR", vars[2]); + }; + } else { + log::warn!("Unexpected output for R envvars"); + } + } else { + log::warn!("Could not read stdout for R envvars"); + }; + }; + let libraries = RLibraries::from_r_home_path(&r_home); libraries.initialize_pre_setup_r(); diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index acae3001a..1eb5db405 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -537,3 +537,24 @@ fn test_stdin_from_menu() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } + +#[test] +fn test_env_vars() { + // These environment variables are set by R's shell script frontend. + // We set these in Ark as well. + let frontend = DummyArkFrontend::lock(); + + let code = "stopifnot( + nzchar(Sys.getenv('R_SHARE_DIR')), + nzchar(Sys.getenv('R_INCLUDE_DIR')), + nzchar(Sys.getenv('R_DOC_DIR')) + )"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} From 15914e1e0b1b575f571e068d94d7dd0207db6ab8 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 18:00:00 +0100 Subject: [PATCH 3/8] Don't capture output in tests --- .github/workflows/test-linux.yml | 2 +- .github/workflows/test-macos.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index 19db0fc8d..dd820c216 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -71,4 +71,4 @@ jobs: - name: Run Unit Tests run: | - cargo test --verbose + cargo test --verbose -- --nocapture diff --git a/.github/workflows/test-macos.yml b/.github/workflows/test-macos.yml index 2bf169fd5..a9089c197 100644 --- a/.github/workflows/test-macos.yml +++ b/.github/workflows/test-macos.yml @@ -45,4 +45,4 @@ jobs: - name: Run Unit Tests run: | - cargo test --verbose + cargo test --verbose -- --nocapture From 0c130c278f864398cd414a2cf31f11ba75ef9dc8 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 19:10:39 +0100 Subject: [PATCH 4/8] Fix `R_HOME` joining --- crates/harp/src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/harp/src/command.rs b/crates/harp/src/command.rs index 2c09ecfe6..dac1e7153 100644 --- a/crates/harp/src/command.rs +++ b/crates/harp/src/command.rs @@ -27,7 +27,7 @@ where // If `R_HOME` is defined use that let locations = COMMAND_R_LOCATIONS.map(|loc| { if let Ok(r_home) = std::env::var("R_HOME") { - std::path::Path::new(&r_home).join(loc) + std::path::Path::new(&r_home).join("bin").join(loc) } else { std::path::Path::new(loc).to_path_buf() } From 76ca0ceb1428d30e4ea282e0cc41eda905d9deb4 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 19:13:07 +0100 Subject: [PATCH 5/8] Rename `COMMAND_R_LOCATION` to `_NAMES` --- crates/harp/src/command.rs | 6 +++--- crates/harp/src/sys/unix/command.rs | 2 +- crates/harp/src/sys/windows/command.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/harp/src/command.rs b/crates/harp/src/command.rs index dac1e7153..2c3dd8a15 100644 --- a/crates/harp/src/command.rs +++ b/crates/harp/src/command.rs @@ -9,7 +9,7 @@ use std::io; use std::process::Command; use std::process::Output; -use crate::sys::command::COMMAND_R_LOCATIONS; +use crate::sys::command::COMMAND_R_NAMES; /// Execute a `Command` for R, trying multiple locations where R might exist /// @@ -22,10 +22,10 @@ pub fn r_command(build: F) -> io::Result where F: Fn(&mut Command), { - assert!(COMMAND_R_LOCATIONS.len() > 0); + assert!(COMMAND_R_NAMES.len() > 0); // If `R_HOME` is defined use that - let locations = COMMAND_R_LOCATIONS.map(|loc| { + let locations = COMMAND_R_NAMES.map(|loc| { if let Ok(r_home) = std::env::var("R_HOME") { std::path::Path::new(&r_home).join("bin").join(loc) } else { diff --git a/crates/harp/src/sys/unix/command.rs b/crates/harp/src/sys/unix/command.rs index 13ffd2ced..9de5ebd7d 100644 --- a/crates/harp/src/sys/unix/command.rs +++ b/crates/harp/src/sys/unix/command.rs @@ -6,4 +6,4 @@ */ /// Locations on the `PATH` to look for R when using `Command::new()` -pub(crate) const COMMAND_R_LOCATIONS: [&str; 1] = ["R"]; +pub(crate) const COMMAND_R_NAMES: [&str; 1] = ["R"]; diff --git a/crates/harp/src/sys/windows/command.rs b/crates/harp/src/sys/windows/command.rs index 5151f7d4c..a982e53a4 100644 --- a/crates/harp/src/sys/windows/command.rs +++ b/crates/harp/src/sys/windows/command.rs @@ -6,4 +6,4 @@ */ /// Locations on the `PATH` to look for R when using `Command::new()` -pub(crate) const COMMAND_R_LOCATIONS: [&str; 2] = ["R", "R.bat"]; +pub(crate) const COMMAND_R_NAMES: [&str; 2] = ["R", "R.bat"]; From 109f706fa5892972dac0873232083454de72516b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 19:34:00 +0100 Subject: [PATCH 6/8] Adjust test --- crates/ark/tests/kernel.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 1eb5db405..843c76648 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -545,9 +545,9 @@ fn test_env_vars() { let frontend = DummyArkFrontend::lock(); let code = "stopifnot( - nzchar(Sys.getenv('R_SHARE_DIR')), - nzchar(Sys.getenv('R_INCLUDE_DIR')), - nzchar(Sys.getenv('R_DOC_DIR')) + identical(Sys.getenv('R_SHARE_DIR'), R.home('share')), + identical(Sys.getenv('R_INCLUDE_DIR'), R.home('include')), + identical(Sys.getenv('R_DOC_DIR'), R.home('doc')) )"; frontend.send_execute_request(code, ExecuteRequestOptions::default()); frontend.recv_iopub_busy(); From afc935fc8a0742ed67bf1321a0486b74557994f9 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 25 Nov 2024 19:36:41 +0100 Subject: [PATCH 7/8] Split on `;` for Windows support --- crates/ark/src/interface.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index a5b3f3c18..bd91b7d97 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -381,12 +381,12 @@ impl RMain { r_command(|command| { // From https://github.com/rstudio/rstudio/blob/74696236/src/cpp/core/r_util/REnvironmentPosix.cpp#L506-L515 command.arg("--vanilla").arg("-s").arg("-e").arg( - r#"cat(paste(R.home('share'), R.home('include'), R.home('doc'), sep=':'))"#, + r#"cat(paste(R.home('share'), R.home('include'), R.home('doc'), sep=';'))"#, ); }) { if let Ok(vars) = String::from_utf8(output.stdout) { - let vars: Vec<&str> = vars.trim().split(':').collect(); + let vars: Vec<&str> = vars.trim().split(';').collect(); if vars.len() == 3 { // Set the R env vars as the R shell script frontend would unsafe { From c804fdb25e6dea08ebf7eb14485e242c0e81f3a2 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 27 Nov 2024 15:33:02 +0100 Subject: [PATCH 8/8] Split `r_command_from_path()` --- crates/ark/src/interface.rs | 13 +++++++-- crates/harp/src/command.rs | 49 ++++++++++++++++++++++++++------- crates/harp/src/fixtures/mod.rs | 4 +-- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index bd91b7d97..234f6aaed 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -54,6 +54,7 @@ use crossbeam::channel::Receiver; use crossbeam::channel::Sender; use crossbeam::select; use harp::command::r_command; +use harp::command::r_command_from_path; use harp::environment::r_ns_env; use harp::environment::Environment; use harp::environment::R_ENVS; @@ -356,18 +357,24 @@ impl RMain { args.push(CString::new(arg).unwrap().into_raw()); } - // Get `R_HOME` from env var, typically set by Positron / CI / kernel specification let r_home = match std::env::var("R_HOME") { - Ok(home) => PathBuf::from(home), + Ok(home) => { + // Get `R_HOME` from env var, typically set by Positron / CI / kernel specification + PathBuf::from(home) + }, Err(_) => { // Get `R_HOME` from `PATH`, via `R` - let Ok(result) = r_command(|command| { + let Ok(result) = r_command_from_path(|command| { command.arg("RHOME"); }) else { panic!("Can't find R or `R_HOME`"); }; + let r_home = String::from_utf8(result.stdout).unwrap(); let r_home = r_home.trim(); + + // Now set `R_HOME`. From now on, `r_command()` can be used to + // run exactly the same R as is running in Ark. unsafe { std::env::set_var("R_HOME", r_home) }; PathBuf::from(r_home) }, diff --git a/crates/harp/src/command.rs b/crates/harp/src/command.rs index 2c3dd8a15..d89cc2f75 100644 --- a/crates/harp/src/command.rs +++ b/crates/harp/src/command.rs @@ -6,16 +6,20 @@ // use std::io; +use std::path::PathBuf; use std::process::Command; use std::process::Output; use crate::sys::command::COMMAND_R_NAMES; -/// Execute a `Command` for R, trying multiple locations where R might exist +/// Execute a `Command` for R, trying multiple names for the R executable /// /// - For unix, this look at `R` /// - For Windows, this looks at `R` (`R.exe`) and `R.bat` (for rig compatibility) /// +/// The executable name is joined to the path in `R_HOME`. If not set, this is a +/// panic. Use `r_command_from_path()` to exectute R from `PATH` instead. +/// /// Returns the `Ok()` value of the first success, or the `Err()` value of the /// last failure if all locations fail. pub fn r_command(build: F) -> io::Result @@ -24,15 +28,40 @@ where { assert!(COMMAND_R_NAMES.len() > 0); - // If `R_HOME` is defined use that - let locations = COMMAND_R_NAMES.map(|loc| { - if let Ok(r_home) = std::env::var("R_HOME") { - std::path::Path::new(&r_home).join("bin").join(loc) - } else { - std::path::Path::new(loc).to_path_buf() - } - }); + // Safety: Caller must ensure `R_HOME` is defined. That's usually the case + // once Ark has properly started. + let r_home = std::env::var("R_HOME").unwrap(); + + let locations: Vec = COMMAND_R_NAMES + .map(|loc| std::path::Path::new(&r_home).join("bin").join(loc)) + .into(); + + r_command_from_locs(locations, build) +} +/// Execute a `Command` for R found on the `PATH` +/// +/// This is like `r_command()` but doesn't assume `R_HOME` is defined. +/// Instead, the R executable is executed as a bare name and the shell +/// executes it from `PATH`. +pub fn r_command_from_path(build: F) -> io::Result +where + F: Fn(&mut Command), +{ + assert!(COMMAND_R_NAMES.len() > 0); + + // Use the bare command names so they are found from the `PATH` + let locations: Vec = COMMAND_R_NAMES + .map(|loc| std::path::Path::new(loc).to_path_buf()) + .into(); + + r_command_from_locs(locations, build) +} + +fn r_command_from_locs(locations: Vec, build: F) -> io::Result +where + F: Fn(&mut Command), +{ let mut out = None; for program in locations.into_iter() { @@ -53,6 +82,6 @@ where } } - // SAFETY: The `assert!` above ensures at least 1 program location is provided + // Unwrap: The `assert!` above ensures at least 1 program location is provided out.unwrap() } diff --git a/crates/harp/src/fixtures/mod.rs b/crates/harp/src/fixtures/mod.rs index ebfb38a14..4444a8ea5 100644 --- a/crates/harp/src/fixtures/mod.rs +++ b/crates/harp/src/fixtures/mod.rs @@ -17,7 +17,7 @@ use libr::R_CStackLimit; use libr::Rf_initialize_R; use stdext::cargs; -use crate::command::r_command; +use crate::command::r_command_from_path; use crate::library::RLibraries; use crate::R_MAIN_THREAD_ID; @@ -53,7 +53,7 @@ pub fn r_test_init() { let r_home = match std::env::var("R_HOME") { Ok(r_home) => PathBuf::from(r_home), Err(_) => { - let result = r_command(|command| { + let result = r_command_from_path(|command| { command.arg("RHOME"); }) .expect("Can't locate R to determine `R_HOME`.");