Skip to content
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

Incorrect Rtools installation behavior when another installation already exists in the default place #289

Closed
Ilia-Kosenkov opened this issue May 5, 2021 · 9 comments
Labels
bug an unexpected problem or unintended behavior

Comments

@Ilia-Kosenkov
Copy link

Describe the bug
The setup-r action installs R and Rtools on Windows.
Due to a number of reasons, it is possible that there already exists an installation of Rtools at the default path (C:\rtools40 for R4.x).
With the recent release of Rtools40v2, a possible bug has surfaced.
If a system image ships with a pre-installed version of Rtools40 found at the default location, the setup-r action will not install a newer version of Rtools40v2 even if asked explicitly using rtools-version: '40v2'.
This happens because the action script relies on a very poor rtools version check, using only the first character in the version string to distinguish between Rtools35 (version "35", tested value is "3") and Rtools40 (version "40", tested values is "4").
When the desired version is Rtools40v2 (version "40v2", tested value is again "4"), it is indistinguishable from the previous version of Rtools40 (or any other future version of Rtools that has a version string starting with "4").
If at the same time Rtools40 is present at the default path, no action is taken and the CI is set up with an older version of Rtools.

This issues potentially blocks migration to the Rtools40v2 (see #287), because even if the correct version is supplied, no action will be taken if an older version is already present in the system.

To Reproduce
To reproduce one needs a CI scenario that relies on the new features added in 40v2 that are absent in 40.
As part of investigating the 40v2, I ran some CI examples (to be honest, a lot before I realised why it is not working).
To be able to debug the setup-r action, I forked this repo and added simple core.info() calls where core.debug() are present in the original code, see for example:
Ilia-Kosenkov@c267817

The important line here is the one that prints "Skipping rtools installation", as it is a sign of setup-r finding an existing installation of the correct (at least according to current logic) version.
The relevant fragment in this repo is here:

// If Rtools is already installed just return, as there is a message box
// which hangs the build otherwise.
if (
(!rtools4 && fs.existsSync("C:\\Rtools")) ||
(rtools4 && fs.existsSync("C:\\rtools40"))
) {
core.debug(
"Skipping Rtools installation as a suitable Rtools is already installed"
);
} else {
let downloadUrl: string = util.format(
"http://cloud.r-project.org/bin/windows/Rtools/%s",
fileName
);

Here is an example CI output that request rtools-version: '40v2':
https://github.com/Ilia-Kosenkov/rextendr/runs/2501639308?check_suite_focus=true#step:2:18
Notice how it outputs desired version of 40v2 yet skips installation because it belives the correct version is already there.

However, here is another example where the downloading and installation do happen:
https://github.com/Ilia-Kosenkov/rextendr/runs/2501698409?check_suite_focus=true#step:3:18

This works because prior to installing R I manually purge system's rtools folder (if present), using e.g. a pwsh script

rm C:\rtools40 -Recurse -Force -ErrorAction SilentlyContinue

Expected behavior
I do not know if there is a way to determine the version of Rtools and differentiate between Rtools40 and Rtools40v2.
If it is very difficult (or impossible), I'd expect that if I provide an explicit version, the script will install a fresh version of Rtools even if a previously installed version exists.
It may do so by installing it to another folder and then setting up an env variable like RTOOLS40_HOME.

Alternatively, a new script parameter can be added that will force re-installation of R and/or Rtools if set to true.

@Ilia-Kosenkov Ilia-Kosenkov added the bug an unexpected problem or unintended behavior label May 5, 2021
@Ilia-Kosenkov
Copy link
Author

cc: @yutannihilation

@jimhester
Copy link
Member

This check was added by @jeroen in #85, who is the maintainer of the Rtools toolchain. Perhaps he can comment on the best way forward.

@jeroen
Copy link
Member

jeroen commented May 5, 2021

Please wait this out for a bit. For CI purposes, the v2 build of the tools40 installer is no different than the original build. It has exactly the same mingw32 and mingw64 toolchains. It is mostly there to prepare for potential future changes, but there is no urgent need to upgrade.

GitHub rebuilds their GHA windows images on a regular basis, I think the preinstalled version of rtools40 will automatically get upgraded to the new build at some point in the upcoming weeks.

Also I have not settled on the name "40v2", perhaps this new build will just replace the rtools40-x86_64.exe at some point, without anyone noticing :-)

@yutannihilation
Copy link
Contributor

without anyone noticing :-)

I doubt this part as we already noticed :) but except for this, it sounds good to me. I closed #287.

@clauswilke
Copy link

I agree it's not super urgent, but just as a reminder: v2 contains a fix that makes building Rust-based projects much simpler. That's why we noticed.

@jeroen
Copy link
Member

jeroen commented May 5, 2021

Yes I know :-) Give it some time, the update is currently pending in chocolately (which I have been told may take up to 2 or 3 weeks): https://community.chocolatey.org/packages/rtools/4.0.0.20210502

After that goes through it should eventually land in the Github actions windows image.

@jeroen
Copy link
Member

jeroen commented May 9, 2021

Here is a proposal for a fix. This will add an option to let rtools40 self-update instead of trying to uninstall and reinstall it: #296

@jimhester
Copy link
Member

Should be fixed by #296

@github-actions
Copy link

github-actions bot commented Nov 6, 2022

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue and include a link to this issue

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants