-
-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
ship 0.36.0 #37925
ship 0.36.0 #37925
Conversation
@BrewTestBot test this please |
I'm going to add
|
@BrewTestBot test this please |
A golang issue has been opened for this compilation segfault: golang/go#30908 |
this avoids issues building on with 1.12 on Sierra
Edit: It's probably better to just require High Sierra for now |
@@ -15,6 +15,8 @@ class Ship < Formula | |||
depends_on "node" => :build | |||
depends_on "yarn" => :build | |||
|
|||
depends_on :macos => :high_sierra # Ship fails to build with Golang 1.12 on Sierra |
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.
Easier but wrong :(
Unless it is explicitly not supported by upstream
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 maintain the upstream, can I say it's not supported?
In all seriousness, there seems to be a compiler bug causing failures with Go 1.12 and MacOS Sierra. I could have builds depend on old versions of Golang, but that has other issues, and it feels wrong to degrade performance on the most commonly used platforms in order to help the least commonly used.
Unless there's a way to conditionally add build dependencies based on the OS?
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.
@fxcoudert would you prefer for me to just require Go 1.10 until the compiler issue is fixed?
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'm going to merge as-is, please try in the next release to remove this requirement :)
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'm not sure the compiler issue will be fixed by then (in fact, I'm sure it won't since I just opened a PR for the next release) but I'll remove the requirement as soon as I can!
Created with
brew bump-formula-pr
.