-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARKR][BUILD] AppVeyor change to latest R version #18856
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
Conversation
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the tests pass
|
@felixcheung, BTW, I think optionally we could also consider setting it 3.4.0 a bit more conservatively for now. There was a rather minor problem for using the latest version (see #15709) due to sync'ing issue of R version, which was fixed within few hours outside Spark IIRC. Either way is fine to me. |
|
It's the latest but it's not new - 3.4.1 was release a month ago. I think there shouldn't be the sync problem.
|
|
Test build #80291 has finished for PR 18856 at commit
|
|
Ah, I meant, the PR I linked actually describes a case when we were using 3.3.1 but it was broken after 3.3.2 release. The reason was, 3.3.2 was released but 3.3.2 for Windows was not synced (but was synced after few hours). The script we are using checks if it is latest or not and adds
But, the issue was, https://rversions.r-pkg.org/r-release gives 3.3.2 after the release but 3.3.1 for WIndows was not in old repo as the latest. |
|
To cut this short, it was ...
This should be minor anyway and I think usually they should be synced quite soon. |
|
@felixcheung, actually, should we try to replace |
|
Will try to open a separate PR. LGTM. |
|
Merged to master. |
|
Right, this PR isn't changing how you have made the version to test with sticky - we won't still pick up the latest automatically, which is fine for now.
But we should bump (manually) to the later/latest version in one of the test matrix to make sure it works.
|
|
I agree this PR should be okay alone, which fixes what it targets for now and I realised my suggestion is rather an extra bit. I opened another one for replacing |
What changes were proposed in this pull request?
R version update
How was this patch tested?
AppVeyor