-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] run 'brew update' in macOS jobs (fixes #4160) #4161
Conversation
.ci/setup.sh
Outdated
@@ -1,6 +1,7 @@ | |||
#!/bin/bash | |||
|
|||
if [[ $OS_NAME == "macos" ]]; then | |||
brew update-reset && brew update |
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.
Please move this temp line to
LightGBM/.ci/test_r_package.sh
Line 79 in d517ba1
if [[ $OS_NAME == "macos" ]]; then |
with the aim to not slow down non-R jobs.
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.
oh I don't really think of it as temporary or as R-specific. What is currently happening with basictex could just as easily happen with anything else installed from brew.
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.
What is currently happening with basictex could just as easily happen with anything else installed from brew.
Agree, I remember we had to use brew update
for gcc
for a long time.
But I think we should avoid brew update
when it is possible due to it increases jobs time.
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.
Alright, moved it in f93d7aa since this is blocking all other PRs.
But I disagree with treating it as temporary and only doing this in the R package's tests. I think having every Mac build be 15 seconds longer is worth a reduced risk of problems like #4160 blocking PR merges in LightGBM for multiple days.
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.
Hmm, I'm very surprised to see 15sec for brew update
! I remember it took several minutes...
If this command is really executed within <1min nowadays, I'm totally fine to have brew update
for each build.
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.
I'll merge this for now to unblock other PRs, I can test the timings and submit another PR in the future to move this back to setup.sh
. The update took only a few seconds on my Mac but that's not a fair test of how long it would take in CI.
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! Feel free to move it back to setup.sh
before merging given #4161 (comment).
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. |
As documented in #4160, LightGBM's macOS jobs for the R package started failing two days ago, roughly around the same time as the version of
basictex
was updated at Homebrew/homebrew-cask#103001.I was going to file a bug report with Homebrew, and in that process I was directed to that project's "how to report bugs" guidelines: https://github.com/Homebrew/homebrew-cask#reporting-bugs. That includes the suggestion that you should run
brew update-reset && brew update
. After trying that on my Mac, I was then able tobrew install --cask basictex
.This PR proposes adding a similar to step to LightGBM's CI. This is similar to how this project uses
sudo apt-get update
to update repository information for theapt
package manager on Linux jobs.brew update-reset
: "Fetch and reset Homebrew and all tap repositories (or any specifiedrepository) using git(1) to their latest origin/HEAD."
brew update
: "Fetch the newest version of Homebrew and all formulae from GitHub using git(1)and perform any necessary migrations"
I believe this will fix #4160 and prevent similar disruptions in the future, and that it should add only a few seconds to CI jobs.