-
Notifications
You must be signed in to change notification settings - Fork 23
Validate R_HOME path early and surface env discovery failures #999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: lionel- <4465050+lionel-@users.noreply.github.com>
lionel-
left a comment
There was a problem hiding this 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.
|
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. |
DavisVaughan
left a comment
There was a problem hiding this 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()
crates/ark/src/interface.rs
Outdated
| 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}"), | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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| { |
There was a problem hiding this comment.
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
f76261d to
3b12de5
Compare
| // running in Ark. | ||
| unsafe { std::env::set_var("R_HOME", &home) }; | ||
|
|
||
| // Check that `R` can be called |
There was a problem hiding this comment.
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
The kernel should fail fast when
R_HOMEis invalid and log when R env discovery fails before loading libraries.R_HOMEpath immediately afterr_home_setup, panicking with a clear path-specific message if missing or unreadable.match; on failure, emit an error log, preserving existing success handling.Example:
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/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
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.