-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Try to fix OS X build #3837
Try to fix OS X build #3837
Conversation
@mlpack-jenkins test this please |
The R build is still failing due to the deprecation issues, but the PR itself does its job (fix the regular OS X build) so it's ready for review. |
Did you see the similar hotfix in the r-actions thread I mentioned on Matrix? In this comment Kirill suggests to uninstall as you do but then (as I read it) let the R install take care of the remainder. Anyway, it seems as if the runner image will come back repaired in due course too. |
Yeah, I saw that, but for the jobs I fixed here, they are the general mlpack OS X jobs, so it is not guaranteed that R will even be used. So in that case it makes sense just to reinstall pkg-config on the spot. |
Do you want me to approve this? So that the bot will give a 2nd vote in 24 hours unless someone else preempts? |
If you're comfortable with the change, then yes please :) |
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.
One comment inline
@@ -13,6 +13,8 @@ steps: | |||
- script: | | |||
set -e | |||
sudo xcode-select --switch /Applications/Xcode.app/Contents/Developer | |||
brew uninstall [email protected] | |||
brew install pkg-config |
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 this is a hot-fix for a temporary issue, shall we place a 'reminder' somewhere to see about reverting to normal "in due course" ?
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.
Good idea, done in aeac96e. 👍
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.
Second approval provided automatically after 24 hours. 👍
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 think this is still not working, just to avoid merging
Well, I looked on another PR and everything is working there. So apparently this was a transient problem that doesn't even need a workaround anymore. |
Another day, another strange issue that has nothing to do with mlpack. Sigh.
(Let's see how many iterations it takes to fix.)