Skip to content

Conversation

Copy link

Copilot AI commented Jan 5, 2026

The kernel should fail fast when R_HOME is invalid and log when R env discovery fails before loading libraries.

  • R_HOME validation: Check existence of the resolved R_HOME path immediately after r_home_setup, panicking with a clear path-specific message if missing or unreadable.
  • Env discovery logging: Wrap the R command used to fetch share/include/doc paths in a match; on failure, emit an error log, preserving existing success handling.

Example:

let r_home = r_home_setup();
match r_home.try_exists() {
    Ok(true) => (),
    Ok(false) => panic!("The `R_HOME` path '{}' does not exist.", r_home.display()),
    Err(err) => panic!("Can't check if `R_HOME` path exists: {err}"),
}

match r_command(|cmd| { /* ... */ }) {
    Ok(output) => { /* parse and set envs */ }
    Err(err) => log::error!("Failed to discover R envvars: {err}"),
};

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https -I /home/REDACTED/work/ark/ark/target/debug/build/zmq-sys-c862c33cd3a44fa9/out --gdwarf-4 --64 -o 2wklqb7xgg.1hztmw8.rcgu.o mxffn8yka9.1hztmw8.rcgu.o zw4ve52ou6.1hztmw8.rcgu.o 5ry69rcexk.1hztmw8.rcgu.o 0phvhlm4b5.1hztmw8.rcgu.o xfri8wg97u.1hztmw8.rcgu.o a7d053c6gw.1hztmw8.rcgu.o 3qle9k0cmd.1hztmw8.rcgu.o r4el06n8il.1hztmw8.rcgu.o qdyyu0aiiz.1hztmw8.rcgu.o 65yysuh2hs.1hztmw8.rcgu.o jij157dqkn.1hztmw8.rcgu.o 2mvn4118p5.1hztmw8.rcgu.o f2coryg2l5.1hztmw8.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

/copilot Please send a PR addressing these two bullet points:

Around https://github.com/posit-dev/ark/blob/main/crates/ark/src/interface.rs#L555-L561

We should check that the r_home path exists before trying to find the library. This way we can give a more accurate error message. This should happen before running the R command to get the paths.

If the R command fails altogether, we should log an error message.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: lionel- <4465050+lionel-@users.noreply.github.com>
Copilot AI changed the title [WIP] Add checks for r_home path existence and R command errors Validate R_HOME path early and surface env discovery failures Jan 5, 2026
Copilot AI requested a review from lionel- January 5, 2026 11:27
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context is at: #997

The goal of this PR is to improve error reporting when the R_HOME install path is invalid.

@lionel- lionel- marked this pull request as ready for review January 5, 2026 12:18
@github-actions
Copy link

github-actions bot commented Jan 5, 2026

Thank you for this PR! We ask that you sign our Contributor License Agreement before we accept your contribution. You can sign the CLA by posting a comment on this PR saying:


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link
Contributor

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! But I would like to see the check move into r_home_setup()

Comment on lines 555 to 561
let r_home = r_home_setup();

match r_home.try_exists() {
Ok(true) => (),
Ok(false) => panic!("The `R_HOME` path '{}' does not exist.", r_home.display()),
Err(err) => panic!("Can't check if `R_HOME` path exists: {err}"),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somewhat strongly think this should just move into r_home_setup().

i.e. make an invariant of r_home_setup() a guarantee that it has checked that the R_HOME it found does exist.

Copy link
Contributor

@lionel- lionel- Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright but I'll have r_home_setup() return a Result, I don't think it should be the one to panic

std::env::set_var("R_INCLUDE_DIR", vars[1]);
std::env::set_var("R_DOC_DIR", vars[2]);
};
match r_command(|command| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC all this does is handle the error path of r_command() better, rather than just ignoring it. That seems nice

@lionel- lionel- force-pushed the copilot/check-r-home-path-existence branch from f76261d to 3b12de5 Compare January 8, 2026 09:27
// running in Ark.
unsafe { std::env::set_var("R_HOME", &home) };

// Check that `R` can be called
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now also checking that R can be called

@lionel- lionel- requested a review from DavisVaughan January 8, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants