-
-
Notifications
You must be signed in to change notification settings - Fork 11.4k
Conversation
depends_on "guile" => :optional | ||
depends_on "aspell" => :optional | ||
depends_on "lua" => :optional | ||
depends_on "curl" => :optional |
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.
This is new; do we need option text making clear this compiles against brewed curl vs system curl (instead of adding curl support)?
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.
Aye it is. There's a curl link and the curl on older OS X versions is getting a little outdated, so it's useful to have the option to go newer in places, so I thought I'd push it in. I guess we could chuck in a with-brewed-curl
option.
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.
Pushed an explicit option in.
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.
If this goes in I think with-curl
is fine; I was thinking if you add this maybe there should additionally be something like option "with-curl", "Build with brewed curl"
.
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.
Sure, Pushed.
Good catch on gettext. |
Thanks! I was impressed upstream had fixed it to be honest, I'm used to finding old Brew-reported issues that just get left open indefinitely 😿. |
depends_on "aspell" => :optional | ||
depends_on "lua" => :optional | ||
depends_on :python => :optional | ||
depends_on "curl" if build.with? "curl" |
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.
This can still be => :optional
so we avoid the class-level conditional; this declaration and the option
declaration together are OK. (sorry to make you commit this three times)
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.
👍
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.
Heh. No worries. Pushed the fix 😉
Devel version, returned the ` gettext ` dep that was removed in #18722 but is now functional again, prunes the redundant ` -DENABLE_GTK=OFF ` option as it’s not a valid option any more and reformats the args.
Devel version, returned the
gettext
dep that was removed in #18722 but is now functional again, prunes the redundant-DENABLE_GTK=OFF
option as it’s not a valid option anymore and reformats the args.