Simplify the lookup process for windows toolchains#1211
Conversation
| # R 4.0 did not include the toolchain in the PATH by default | ||
| # or set the R_TOOLS_SOFT or LOCAL_SOFT config variables, so | ||
| # we use the RTOOLS40_HOME environment variable instead | ||
| if (R.version$major == 4 && R.version$minor < 2) { |
There was a problem hiding this comment.
| if (R.version$major == 4 && R.version$minor < 2) { | |
| if (current_r_version() < "4.2.0") { |
Not a problem anytime soon, but because R.version$major actually returns a character we can get weird things like "10.0" < 2 evaluates to TRUE, so R 4.10 would incorrectly use Rtools40. current_r_version() returns getRversion() and has class numeric_version, which avoids this.
There was a problem hiding this comment.
Ah that is classic R. Thanks for catching!
There was a problem hiding this comment.
I wonder if we could just require R >= 4.2? Or do you think there are still enough older R versions in the wild that we should continue supporting it?
There was a problem hiding this comment.
Yeah I think there might still be a bunch of older systems out there - where I work in gov we only left 4.2 a few months ago 😮💨
Pretty happy to only do the bare minimum for it though!
There was a problem hiding this comment.
Yeah good point. I would guess some companies still use old R versions too, so we should keep supporting it
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1211 +/- ##
==========================================
+ Coverage 90.96% 91.70% +0.74%
==========================================
Files 15 15
Lines 6252 6184 -68
==========================================
- Hits 5687 5671 -16
+ Misses 565 513 -52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@jgabry this one's good to look at! CI is all green for standard RTools and I've also verified locally that we now support the MSYS-packaged R too |
|
Cool, thanks @andrjohns! I'll take a look |
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
This is great, thanks for doing this. I did a little research to try to understand R_TOOLS_SOFT and I tested a couple things. I have a couple of questions/concerns, but not sure how big a deal they are (see review comments).
| r_arch <- ifelse(Sys.getenv("R_ARCH") == "/i386", "mingw32", "mingw64") | ||
| rtools_soft <- file.path(rtools40_home, r_arch) | ||
| } else { | ||
| rtools_soft <- repair_path(tools::Rcmd(c("config", "R_TOOLS_SOFT"), stdout = TRUE)) |
There was a problem hiding this comment.
This will start an R CMD config subprocess every time toolchain_PATH_env_var() is called, right? Since that gets called in a ton of places in the package (every model fit, every CSV read, cpp_opts, etc.), should we compute it once and cache it somewhere (maybe an environment internal to the package)? I'm assuming it can't change within a single session.
That said, maybe it's not worth bothering to do this it if it's pretty fast. Could it add up enough to be noticeable? I defer to your judgement on this, since I'm not a windows users.
There was a problem hiding this comment.
Also, what happens if R_TOOLS_SOFT isn't in Makeconf? Maybe that's not possible with CRAN R >= 4.2 but maybe it's possible for something like conda R?
I tried testing it on my Mac, which kind of simulates a windows distribution that doesn't have it, and R CMD config has a nonzero exit status and system2 throws a "running command ... had status 1" warning on every call to toolchain_PATH_env_var().
What if we do something like this:
out <- suppressWarnings(
tools::Rcmd(c("config", "R_TOOLS_SOFT"), stdout = TRUE)
)
if (is.null(attr(out, "status")) && length(out) == 1) {
rtools_soft <- repair_path(out)
} else {
rtools_soft <- ""
}| # If RTools is installed in a path with spaces or brackets | ||
| # we error as this path is not valid | ||
| if (grepl("\\(|)| ", rtools_path)) { | ||
| stop( | ||
| "\n", rtools_version, " is installed in a path with spaces or brackets, which is not supported.", |
There was a problem hiding this comment.
Should we keep something like this to make sure we error early when rtools is in a directory with spaces? Seems somewhat common
| ) | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
We're deleting hundreds of lines of tests, which is great, but is there anything we should be adding to the tests for the new logic? Makes me slightly nervous to not have anything. Or do you think our other existing tests cover everything we need?
3 participants
Submission Checklist
Summary
The current process for the toolchain on Windows is a little brittle, and requires hard-coding a new conditional on each RTools release. Starting from R4.2 the
R_TOOLS_SOFTconfig variable was introduced to provide the path to the toolchain directory for the current installation - also allowing alternate packagers to specify a different default, which we can query directly.The current PR updates to toolchain resolution to first check for a valid toolchain in the
R_TOOLS_SOFTdirectory. If one is not found, it falls back to searching the PATH (viaSys.which()) as R automatically prepends the toolchain directories to the PATH on startup.For R4.0 (before
R_TOOLS_SOFTexisted or the toolchain was automatically added to the PATH), it uses the original lookup of theRTOOLS40_HOMEenvironment variableCopyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Andrew Johnson
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: