Add hard fail on unsupported R version#1199
Conversation
| } | ||
| } | ||
|
|
||
| // It is assumed that r_home matches the R_HOME env var. Usually the input will |
There was a problem hiding this comment.
That's a bit confusing. If we're passing r_home through, we should probably run that R?
There was a problem hiding this comment.
Right now we're only passing it to return it along with the version. That seems misleading, since it suggests the function needs it but not really.
There was a problem hiding this comment.
yeah, I agree... this was the minimally invasive path but it does lead to some weirdness
There was a problem hiding this comment.
How would you feel about r_home_setup() returning an RVersion instead of a PathBuf? That is the nicest way to fold all of this together I think?
There was a problem hiding this comment.
Good idea. How about a new RHome struct with version and path field?
There was a problem hiding this comment.
I'm fine with that - the use of RVersion is pretty much confined to this so it's easy to generalise it
There was a problem hiding this comment.
Do you mind if i merge #1172 first? It uses / moves around some things with r_home_setup()
update: merged
There was a problem hiding this comment.
ok, with R_HOME always set we should probably just remove r_home from the RVersion struct and use that..?
There was a problem hiding this comment.
ok, with R_HOME always set we should probably just remove r_home from the RVersion struct and use that..?
It's still useful to have it in a struct, looking up an envvar is not thread safe and should in principle only be done from the R thread (we've seen crashes with 0mq init because of this in the past). The less we look up envvars, the better.
Clean up `RVersion` and `r_command()` cruft accumulated over time
|
@DavisVaughan Can you give the latest iteration I quick glance? I changed the version check to read from the DESCRIPTION file of base to completely avoid any R process execution for it |
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Looks great just one minor comment
| use oak_package_metadata::description::Description; | ||
|
|
||
| pub const MIN_R_MAJOR: u32 = 4; | ||
| pub const MIN_R_MINOR: u32 = 2; |
There was a problem hiding this comment.
You may as well throw in MIN_R_PATCH: u32: = 0
I think it makes the Ark requires R >= {}.{}.0 message clearer and we may need it one day
Don't forget to update is_supported
3 participants
Fix #696
This PR inserts a version check in the start method, right after R detection. It does not dive into checking R package versions as suggested since these are not hard requirements AFAICT and Ark gracefully handles it already