Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Add RDM tests option to OLA #48575

Closed
wants to merge 4 commits into from
Closed

Add RDM tests option to OLA #48575

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
321e1ff
Make libusb and liblo optional
peternewman Jan 29, 2016
ed1b7b7
Add an rdm-tests option
peternewman Jan 29, 2016
fa7a04f
ola: fix PR comments
peternewman Jan 31, 2016
0256134
OLA: Fix the remaining comment
peternewman Mar 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions Library/Formula/ola.rb
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

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.

Copy link
Contributor Author

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.


bottle do
sha256 "48b73cfefda79be164fe9d201f4906bab1bd6b341979646ae346471fd2bc0101" => :el_capitan
Expand All @@ -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"
Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there isn't.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

depends_on "protobuf-c"
depends_on "libmicrohttpd"
depends_on "libusb"
depends_on "liblo"
depends_on "ossp-uuid"
depends_on "libusb" => :optional
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made both these optional, but I'm wondering if they should be recommended? They enable plugins for the specific devices, e.g. for OSC support. I imagine most people don't use OSC, but should we enable it by default (recommeded) so it's there and "it just works"? I know other projects e.g. Debian have policies on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer not to inflict the extra dependencies on people unconditionally unless it's something a lot of people are going to want or need.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although given they were the default already here, :recommended should be fine actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, noted. Changed to recommended.

depends_on "liblo" => :optional
depends_on :python => :optional
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

depends_on "doxygen" => :optional

Expand All @@ -36,6 +38,10 @@ class Ola < Formula
depends_on "libftdi0"
end

if build.with? "rdm-tests"
depends_on :python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

depends_on :python if MacOS.version <= :snow_leopard if build.with? "rdm-tests"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I'll just trust you on that one.

end

def install
ENV.universal_binary if build.universal?

Expand All @@ -47,6 +53,7 @@ def install
]

args << "--enable-python-libs" if build.with? "python"
args << "--enable-rdm-tests" if build.with? "rdm-tests"
args << "--enable-doxygen-man" if build.with? "doxygen"

system "autoreconf", "-fvi" if build.head?
Expand Down