krlmlr / r-appveyor

Tools for using R with AppVeyor (https://appveyor.com)
132 stars 60 forks source link

Explicitly check the value of USE_RTOOLS. #132

Closed jdblischak closed 5 years ago

jdblischak commented 5 years ago

Close #128.

cc: @krlmlr @LiNk-NY

I tested the proposed behavior using the following function:

Function Test_RTOOLS
{
  if ($env:USE_RTOOLS -eq "true") {
    Write-Host "RTOOLS will be installed"
  }
  Else {
    Write-Host "RTOOLS will NOT be installed"
  }
}

And it appears to work correctly:

PS C:\Users\john> $env:USE_RTOOLS = "true"
PS C:\Users\john> Test_RTOOLS
RTOOLS will be installed

PS C:\Users\john> $env:USE_RTOOLS = "false"
PS C:\Users\john> Test_RTOOLS
RTOOLS will NOT be installed

PS C:\Users\john> $env:USE_RTOOLS = "bananas"
PS C:\Users\john> Test_RTOOLS
RTOOLS will NOT be installed

PS C:\Users\john> $env:USE_RTOOLS = ""
PS C:\Users\john> Test_RTOOLS
RTOOLS will NOT be installed
LiNk-NY commented 5 years ago

Hi John, thanks for testing this !

I wonder if this is robust to inputs like TRUE and True? These are variations of what could be found in the appveyor.yml file (esp. for True).

Regards, Marcel

jdblischak commented 5 years ago

I wonder if this is robust to inputs like TRUE and True? These are variations of what could be found in the appveyor.yml file (esp. for True).

@LiNk-NY It's not. But the instructions in the README are quite explicit:

Set USE_RTOOLS: true if Rtools needs to be installed

Also note that the environment variable NOT_CRAN is just as strict. testthat::skip_on_cran() requires it be be set to "true".

https://github.com/r-lib/testthat/blob/8216ebe2d84028d1a7a315c5c17c15f93cad3d65/R/skip.R#L118

jdblischak commented 5 years ago

One job failed because of an error trying to compile rlang.

jdblischak commented 5 years ago

I amended the commit to re-trigger the CI builds.

jdblischak commented 5 years ago

Same rlang error as last time. Seems unrelated.

@krlmlr Could you please review this minor PR?

krlmlr commented 5 years ago

Thanks. I've looked for usages of USE_RTOOLS on GitHub, saw quite a few instances of USE_RTOOLS: yes (in addition to USE_RTOOLS: true which is the vast majority). The path of the least resistance here would be to support USE_RTOOLS: yes as well, we could also ping the owners of the affected repos. What do you think?

https://github.com/search?q=%22USE_RTOOLS%3A+yes%22&type=Code

krlmlr commented 5 years ago

Also, @msberends suggests in #133 to install Rtools also when PKGTYPE is not set to win.binary. I'm not sure about this (fixes one set of problems but opens up another one). Perhaps change both -- set the default for PKGTYPE to win.binary and install RTOOLS if it's set to anything else?

jdblischak commented 5 years ago

The path of the least resistance here would be to support USE_RTOOLS: yes as well

OK. I can add that.

we could also ping the owners of the affected repos.

I can write a script to open an Issue on each of these repositories. Though the behavior won't change for them. The users that are going to be surprised are those that currently have RTOOLS installed when setting USE_RTOOLS to some string other than yes or true (unfortunately the GitHub search isn't case-sensitive so it's not trivial to find examples of, e.g. USE_RTOOLS: TRUE or USE_RTOOLS: Yes).

set the default for PKGTYPE to win.binary and install RTOOLS if it's set to anything else?

I like this suggested behavior. Avoiding installing RTOOLS and source packages will be faster and is sufficient for most packages. If a user really needs to the very latest CRAN version of a package for the few days that it is not also available as a binary, then they can adjust the default settings.

However, how do I accomplish this technically? The setting PKGTYPE is defined in travis-tool.sh, but the decision to install RTOOLS is made prior to executing travis-tool.sh.

https://github.com/krlmlr/r-appveyor/blob/master/scripts/appveyor-tool.ps1#L161

jdblischak commented 5 years ago

I added support for USE_RTOOLS: yes. And since I'm not proficient in PowerShell, I confirmed that you can have multiple -or statements chained together by running the following:

if (('a' -eq 'b') -or ('b' -eq 'c') -or ('c' -eq 'c')) {Write-Host true}
krlmlr commented 5 years ago

Thanks!