-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[ci] fall back to source installs of R packages if binary missing (fixes #3875) #3876
Conversation
@@ -101,9 +101,9 @@ fi | |||
# to avoid a CI-time dependency on devtools (for devtools::install_deps()) | |||
packages="c('data.table', 'jsonlite', 'Matrix', 'R6', 'testthat')" | |||
if [[ $OS_NAME == "macos" ]]; then | |||
packages+=", type = 'binary'" | |||
packages+=", type = 'both'" |
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.
as a reminder, this flag cannot be used on Linux, and that's why it's in this if
.
From inside a rocker/verse
container:
> install.packages("jsonlite", type = "both")
Installing package into ‘/usr/local/lib/R/site-library’
(as ‘lib’ is unspecified)
Error in install.packages("jsonlite", type = "both") :
type == "both" can only be used on Windows or a CRAN build for macOS
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.
Many thanks!
@StrikerRUS we'll have to use admin rights to merge this, since I haven't fixed #3873 yet. Is that ok? Otherwise I can remove the linting job temporarily from |
And this one #3874 to fix CUDA tests. I think admin rights is better than mess in CI code between PRs. |
I see in macOS R4:
Seems having |
ok seeing that, now I think the issue is that something somewhere in I still think the |
hmmmm interestingly, it DOES look like R tried to install
But it still only looked for the binary. Ok, let me experiment a little. I commented out all other CIs so I don't hurt our capacity too much while testing. |
I agree! |
Ah!!! I can see on CRAN that the I think R is getting confused by this. No amount of trickery with |
This will be reverted after a day or two, right? When version will be in a sync state at CRAN, I mean. |
It probably could be reverted, yes. I don't know a more permanent fix for this problem :/
|
This seems to have worked: https://github.com/microsoft/LightGBM/pull/3876/checks?check_run_id=1785421659 |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
I believe this will fix #3875. Right now in the Mac CI jobs for the R package, we set
type = "binary"
ininstall.packages()
. This can lead to failures shortly after a new package needed in those jobs is uploaded to CRAN, when only source versions are available. Precompiled binaries from CRAN usually take 1-2 days to become available, in my experience.the package
{withr}
was updated yesterday (https://cran.r-project.org/web/packages/withr/index.html), and I think the failures observed in #3875 happened because of this phenomenon.This PR changes the strategy for installation on Mac to "both". From
?install.packages
So as of this PR, Mac builds will look for binaries (which are faster to install), but will install source packages from CRAN if the source package is newer.
This should prevent future issues like this from blocking LightGBM's CI.