Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Aug 6, 2017

What changes were proposed in this pull request?

This PR proposes to use https://rversions.r-pkg.org/r-release-win instead of https://rversions.r-pkg.org/r-release to check R's version for Windows correctly.

We met a syncing problem with Windows release (see #15709) before. To cut this short, it was ...

  • 3.3.2 release was released but not for Windows for few hours.
  • https://rversions.r-pkg.org/r-release returns the latest as 3.3.2 and the download link for 3.3.1 becomes windows/base/old by our script
  • 3.3.2 release for WIndows yet
  • 3.3.1 is still not in windows/base/old but windows/base as the latest
  • Failed to download with windows/base/old link and builds were broken

I believe this problem is not only what we met. Please see krlmlr/r-appveyor@01ce943 and also this r-release-win API came out between 3.3.1 and 3.3.2 (assuming to deal with this issue), please see https://github.com/metacran/rversions.app/issues/2.

Using this API will prevent the problem although it looks quite rare assuming from the commit logs in https://github.com/metacran/rversions.app/commits/master. After 3.3.2, both r-release-win and r-release are being updated together.

How was this patch tested?

AppVeyor tests.

@HyukjinKwon
Copy link
Member Author

cc @felixcheung, trivial but I believe this should be more correct.

@HyukjinKwon HyukjinKwon changed the title [MINOR][R][BUILD] More reliable detection of R version for Windows [MINOR][R][BUILD] More reliable detection of R version for Windows in AppVeyor Aug 6, 2017
@HyukjinKwon HyukjinKwon closed this Aug 6, 2017
@HyukjinKwon HyukjinKwon reopened this Aug 6, 2017

$urlPath = ""
$latestVer = $(ConvertFrom-JSON $(Invoke-WebRequest http://rversions.r-pkg.org/r-release).Content).version
$latestVer = $(ConvertFrom-JSON $(Invoke-WebRequest http://rversions.r-pkg.org/r-release-win).Content).version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw from http://rversions.r-pkg.org/r-release-win, it has a URL field:

"URL": "https://cran.r-project.org/bin/windows/base/R-3.4.1-win.exe"

If we go to use this URL, maybe we don't need to combine rurl below?

Copy link
Member

@viirya viirya Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I saw there is old/ path. Then we still need to manually combine rurl.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I think it'd be a good idea if we introduce something like latest as discussed in #15709 later.

@SparkQA
Copy link

SparkQA commented Aug 6, 2017

Test build #80301 has finished for PR 18859 at commit 655a8ed.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

Merged this to master.

@asfgit asfgit closed this in 08ef7d7 Aug 8, 2017
@HyukjinKwon HyukjinKwon deleted the use-reliable-link branch January 2, 2018 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants