Conversation
Sorry, I failed to read the contributing guidelines first, so my commit messages don't have the right prefix. |
@@ -20,14 +21,15 @@ class Ola < Formula | |||
|
|||
option :universal | |||
option "with-ftdi", "Install FTDI USB plugin for OLA." | |||
option "with-rdm-tests", "Install RDM Tests for OLA." | |||
|
|||
depends_on "pkg-config" => :build | |||
depends_on "cppunit" |
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.
Is there a :test dependency flag? We only need cppunit for the tests.
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.
No, there isn't.
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.
Okay thanks. Any plans to do so? If I understand your bottle concept correctly, will that not still drag the test deps in, despite the fact your unlikely to bother testing the binary?
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'd like to add a TODO if there's an issue covering adding a :test dependency flag.
Hmm… I am getting stumped as to how to test this as well. Here is the best I have been able to achieve so far, perhaps one of the maintainers can give us some help? I have gotten lost in the documentation.
|
@@ -3,6 +3,7 @@ class Ola < Formula | |||
homepage "https://www.openlighting.org/ola/" | |||
url "https://github.com/OpenLightingProject/ola/releases/download/0.10.0/ola-0.10.0.tar.gz" | |||
sha256 "cae8131a62f0ff78d399c42e64a51b610d7cf090b606704081ec9dd2cf979883" | |||
revision 2 |
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.
Don't need the revision unless we're making a breaking change here. Also would be revision 1
; default is 0
and unwritten.
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.
My reading of your docs was a revision was needed to force the build to update, or is that not the case? I simply bumped it each time I committed, as I wasn't sure what it did about freshness, or does it just git pull? Anyway removed.
I'd maybe consider a
Did you install |
I should probably clean up many formulas, I don’t think I have ever run I don’t remember making any specific choices when installing
|
My reading of that brew info is it should install with python by default, or have I read that wrong? Separate question, how do I force --with-python on protobuf when people choose the RDM tests option? Currently it's just depending on protobuf-c, which pulls in protobuf. |
I think in the interim @brunchboy you can install protobuf --with-python, then try my Formula again and it will hopefully get further. |
@peternewman you were right, even though I thought my original reading of the |
Is there anything else needed from me? |
Ah, I completely missed the email on the notification last week. Not sure why I didn't get nudged by GitHub there. I'll look this over today, and try to look into the |
Thanks @DomT4 . @brunchboy we shouldn't need anything from you for the moment, apart from to ideally remove protobuf and ola and retest when it's all working. Doing this has reminded me we don't have a Ja Rule option yet, now that's launched, so we should try and do that at some point too. |
All right, sounds good. Having overcome my superstitious fears of uninstalling and reinstalling ola and protobuf once (because they’re so critical to the project devouring my free time 😉) it will be no problem to do it again when it’s all ready. |
Sorry, I haven't forgotten this. Life and Homebrew are both combining to be extremely hectic, which is fun. If I don't post some kind of update here today please feel free to throw 🍅s at me. |
Well, my hunch was wrong. I thought perhaps passing
At the moment, you can't. You can make people build with non-default options, but you can't make people build with default options. Really though, the amount of Homebrew users who deviate from the defaults is a minority of the user base and I can't foresee significant issues here. Hopefully not famous last words there 😉. Need one small change in the formula but otherwise I'm happy: if build.with? "rdm-tests"
depends_on :python if MacOS.version <= :snow_leopard
else
depends_on :python => :optional
end |
Sorry @DomT4 I've been rather sidetracked too. A similar question to the other, is there an issue for adding the feature to force a --with, so I can add a comment and we can improve our fomula when it gets fixed? Can we at least detect which --with option they've chosen and display a warning to the user, or can we only put a comment in the formula? |
You can check the options if you wish to, example. It's pretty rare, we tend to reserve it for cases that are known to be a regular occurrence. |
@peternewman I am still waiting for this PR to be closed before updating the formula to include OLA 0.10.1, so Homebrew is still stuck on 0.10.0. I am sure you are busy preparing for the Google Summer of Code, but is there any hope we can see this finished soon? |
@BrewTestBot test this please |
Merged in 52b37f1. Thank you for your contribution to Homebrew @peternewman; we appreciate it! 😺 |
depends_on "ossp-uuid" | ||
depends_on "libusb" => :recommended | ||
depends_on "liblo" => :recommended | ||
depends_on :python => :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.
Is this line redundant, now its been duplicated on line 44?
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 chopped it in post before merging.
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.
Ah yes, just spotted that. Thanks.
Sorry @brunchboy . Okay based on your comments @DomT4 I won't bother actively checking and we can see how people get on with it. |
Closes Homebrew/legacy-homebrew#48575. Signed-off-by: Dominyk Tiller <dominyktiller@gmail.com>
Great, thanks, guys! I have submitted a PR to update the formula to 0.10.1. |
@brunchboy needs to test this first; this is really just to run Travis against it and start asking some questions regarding some bits.