[ovs-dev] rhel: Add libcap-ng dependency.
diff mbox

Message ID 1443729035-22348-1-git-send-email-rbryant@redhat.com
State Superseded
Headers show

Commit Message

Russell Bryant Oct. 1, 2015, 7:50 p.m. UTC
Commit e91b927d8966bfcb9768225392324dde4fd7d7f6 added optional usage of
the libcap-ng library.  It's packaged in Fedora, so go ahead and added
it to the Fedora spec file.

Our default systemd unit files don't make use of the --user option that
requires this library, but conceivably someone may want to customize
them and use this option.

Signed-off-by: Russell Bryant <rbryant@redhat.com>
---
 rhel/openvswitch-fedora.spec.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ben Pfaff Oct. 1, 2015, 7:54 p.m. UTC | #1
On Thu, Oct 01, 2015 at 03:50:35PM -0400, Russell Bryant wrote:
> Commit e91b927d8966bfcb9768225392324dde4fd7d7f6 added optional usage of
> the libcap-ng library.  It's packaged in Fedora, so go ahead and added
> it to the Fedora spec file.
> 
> Our default systemd unit files don't make use of the --user option that
> requires this library, but conceivably someone may want to customize
> them and use this option.
> 
> Signed-off-by: Russell Bryant <rbryant@redhat.com>

I'd consider also adding --with-libcapng=yes to the configure command
line.  That way, a build without libcap-ng installed will fail.  That
will both root out problems in whatever RPM autobuilders happen to be
running against OVS and make sure that every RPM build supports
libcap-ng.
Russell Bryant Oct. 1, 2015, 8 p.m. UTC | #2
On 10/01/2015 03:54 PM, Ben Pfaff wrote:
> On Thu, Oct 01, 2015 at 03:50:35PM -0400, Russell Bryant wrote:
>> Commit e91b927d8966bfcb9768225392324dde4fd7d7f6 added optional usage of
>> the libcap-ng library.  It's packaged in Fedora, so go ahead and added
>> it to the Fedora spec file.
>>
>> Our default systemd unit files don't make use of the --user option that
>> requires this library, but conceivably someone may want to customize
>> them and use this option.
>>
>> Signed-off-by: Russell Bryant <rbryant@redhat.com>
> 
> I'd consider also adding --with-libcapng=yes to the configure command
> line.  That way, a build without libcap-ng installed will fail.  That
> will both root out problems in whatever RPM autobuilders happen to be
> running against OVS and make sure that every RPM build supports
> libcap-ng.
> 

The rpm build would fail anyway if it's not installed at all.  I tested
that.

It would not catch possible failures due to distro or package version
differences.  For example, if the header file was installed was
installed in a different location and the configure script didn't find
it, the package would still happily build now, but without libcap-ng
support.  --with-libcapng=yes would catch that, at least.

Thanks for the suggestion.  I'll send out a v2.
Flavio Leitner Oct. 1, 2015, 8:22 p.m. UTC | #3
On Thu, Oct 01, 2015 at 04:00:09PM -0400, Russell Bryant wrote:
> On 10/01/2015 03:54 PM, Ben Pfaff wrote:
> > On Thu, Oct 01, 2015 at 03:50:35PM -0400, Russell Bryant wrote:
> >> Commit e91b927d8966bfcb9768225392324dde4fd7d7f6 added optional usage of
> >> the libcap-ng library.  It's packaged in Fedora, so go ahead and added
> >> it to the Fedora spec file.
> >>
> >> Our default systemd unit files don't make use of the --user option that
> >> requires this library, but conceivably someone may want to customize
> >> them and use this option.
> >>
> >> Signed-off-by: Russell Bryant <rbryant@redhat.com>
> > 
> > I'd consider also adding --with-libcapng=yes to the configure command
> > line.  That way, a build without libcap-ng installed will fail.  That
> > will both root out problems in whatever RPM autobuilders happen to be
> > running against OVS and make sure that every RPM build supports
> > libcap-ng.
> > 
> 
> The rpm build would fail anyway if it's not installed at all.  I tested
> that.

That's because you are adding Requires and BuildRequires.
The BuildRequires enforces that the libpcap-ng is installed
during the RPM build.

> It would not catch possible failures due to distro or package version
> differences.  For example, if the header file was installed was
> installed in a different location and the configure script didn't find
> it, the package would still happily build now, but without libcap-ng
> support.  --with-libcapng=yes would catch that, at least.

The only issue I see is to have one more dependency to carry on which
for some use cases might be a problem.

Since this is an optional feature, worth to look at the %bcond_with
and %bcond_without macros.  We already use that to enable/disable
running the testsuite.  We could use the same approach to build
with (default) or without libpcap-ng.

Does that make sense?

fbl

> Thanks for the suggestion.  I'll send out a v2.
Flavio Leitner Oct. 1, 2015, 10:35 p.m. UTC | #4
On Thu, Oct 01, 2015 at 05:22:39PM -0300, Flavio Leitner wrote:
> On Thu, Oct 01, 2015 at 04:00:09PM -0400, Russell Bryant wrote:
> > On 10/01/2015 03:54 PM, Ben Pfaff wrote:
> > > On Thu, Oct 01, 2015 at 03:50:35PM -0400, Russell Bryant wrote:
> > >> Commit e91b927d8966bfcb9768225392324dde4fd7d7f6 added optional usage of
> > >> the libcap-ng library.  It's packaged in Fedora, so go ahead and added
> > >> it to the Fedora spec file.
> > >>
> > >> Our default systemd unit files don't make use of the --user option that
> > >> requires this library, but conceivably someone may want to customize
> > >> them and use this option.
> > >>
> > >> Signed-off-by: Russell Bryant <rbryant@redhat.com>
> > > 
> > > I'd consider also adding --with-libcapng=yes to the configure command
> > > line.  That way, a build without libcap-ng installed will fail.  That
> > > will both root out problems in whatever RPM autobuilders happen to be
> > > running against OVS and make sure that every RPM build supports
> > > libcap-ng.
> 
> > It would not catch possible failures due to distro or package version
> > differences.  For example, if the header file was installed was
> > installed in a different location and the configure script didn't find
> > it, the package would still happily build now, but without libcap-ng
> > support.  --with-libcapng=yes would catch that, at least.
> 
> The only issue I see is to have one more dependency to carry on which
> for some use cases might be a problem.
> 
> Since this is an optional feature, worth to look at the %bcond_with
> and %bcond_without macros.  We already use that to enable/disable
> running the testsuite.  We could use the same approach to build
> with (default) or without libpcap-ng.
> 
> Does that make sense?

I sent out a patch to show what I am saying.
Russel, I based my patch on yours, so if you like the patch, feel free
to signed-off-by too, thanks! :)

BTW, I haven't tested the binaries, just the rpm stuff.

Ah, I removed the Requires to the library since RPM will catch that
if needed. You can use 'rpm -qpR <rpm file>' to see the requirement
list.

Thanks!
fbl

Patch
diff mbox

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 695f1d7..4e46909 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -37,13 +37,14 @@  Source: http://openvswitch.org/releases/%{name}-%{version}.tar.gz
 
 BuildRequires: autoconf automake libtool
 BuildRequires: systemd-units openssl openssl-devel
+BuildRequires: libcap-ng libcap-ng-devel
 BuildRequires: python python-twisted-core python-zope-interface PyQt4
 BuildRequires: desktop-file-utils
 BuildRequires: groff graphviz
 # make check dependencies
 BuildRequires: procps-ng
 
-Requires: openssl iproute module-init-tools
+Requires: openssl iproute module-init-tools libcap-ng
 #Upstream kernel commit 4f647e0a3c37b8d5086214128614a136064110c3
 #Requires: kernel >= 3.15.0-0