Bug 493750 - Review Request: netcf - a library for managing network configuration
Review Request: netcf - a library for managing network configuration
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-02 19:04 EDT by David Lutterkort
Modified: 2013-04-30 19:41 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-09-14 19:08:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Lutterkort 2009-04-02 19:04:39 EDT
Spec URL: http://people.redhat.com/dlutter/netcf/download/netcf.spec
SRPM URL: http://people.redhat.com/dlutter/netcf/download/netcf-0.0.1-1.fc10.src.rpm
Description:

A library for modifying the network configuration of a system. Network
configurations are expresed in a platform-independent XML format, which
netcf translates into changes to the system's 'native' network
configuration files.
Comment 1 Michael Schwendt 2009-04-12 04:25:03 EDT
* Your home page points to a changed "URL: https://fedorahosted.org/netcf/"


* Better summary: Cross-platform network configuration library
(taken from README!)


* Fedora 10 build needs augeas >= 0.5.0 from updates-testing


* Build errors on Fedora 10:
[...]
checking for LIBAUGEAS... yes
checking for LIBXML... 
[...]

 => Missing BuildRequires: libxml2-devel


checking for LIBXSLT... 

 => Missing BuildRequires: libxslt-devel
(this requires libxml2-devel, too)


* In "src" directory, NETCF_CFLAGS duplicates the CFLAGS.


* netcf.pc  file is missing "Requires: augeas" because it contains "-laugeas".
Comment 2 Michael Schwendt 2009-04-12 13:04:25 EDT
> * netcf.pc  file is missing "Requires: augeas" because it
> contains "-laugeas".

Since libnetcf is linked with libaugeas already, it would be even better to drop "-laugeas" from Libs. 

Else, for static linking support in upstream tarball, add "Requires.private: augeas" to the pkgconfig file, move "-laugeas" from Libs to Libs.private, and add "Requires: augeas-devel >= 0.5.0" to the netcf-devel subpackage (for Fedora <= 10 which doesn't create automatic pkgconfig dependencies).
Comment 3 David Lutterkort 2009-04-15 14:10:05 EDT
Sorry for the lag, somehow your review got filed into the wrong mail folder.

(In reply to comment #1)
> * Your home page points to a changed "URL: https://fedorahosted.org/netcf/"

Fixed.

> * Better summary: Cross-platform network configuration library
> (taken from README!)

Thanks; indeed much better.

> * Fedora 10 build needs augeas >= 0.5.0 from updates-testing

Yeah, it just got pushed to updates

> * Build errors on Fedora 10:
> [...]
> checking for LIBAUGEAS... yes
> checking for LIBXML... 
> [...]
> 
>  => Missing BuildRequires: libxml2-devel
> 
> 
> checking for LIBXSLT... 
> 
>  => Missing BuildRequires: libxslt-devel
> (this requires libxml2-devel, too)

Added those BR's

> * In "src" directory, NETCF_CFLAGS duplicates the CFLAGS.

I don't see that - do you mean the duplication of -I/usr/include/libxml2 ? That comes from using cflags from both libxml2 and libxslt; though since netcf uses API's from both, i.e. it doesn't just use libxml2 via libxslt, I'd rather leave the explicit LIBXML_CFLAGS in the Makefile.

> * netcf.pc  file is missing "Requires: augeas" because it contains "-laugeas".  

Ok, I am adding a Requires for augeas, libxslt, and libxml-2.0 to netcf.pc and taking out the -laugeas.

I'll post an updated spec/srpm in a minute.
Comment 5 Michael Schwendt 2009-04-16 07:18:45 EDT
Okay, packaging-wise the 0.0.2-1.fc10 src.rpm is fine:

  APPROVED

[...]

The remaining minor issues are within your upstream tarball:

* Duplicated CFLAGS. Quoting from:
http://koji.fedoraproject.org/koji/getfile?taskID=1300747&name=build.log

>>>
make[3]: Entering directory `/builddir/build/BUILD/netcf-0.0.2/src'
/bin/sh ../libtool --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I..    --std=gnu99 -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fasynchronous-unwind-tables -I ../gnulib/lib -Wall -Wformat -Wformat-security -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wextra -Wshadow -Wcast-align -Wwrite-strings -Waggregate-return -Wstrict-prototypes -Winline -Wredundant-decls -Wno-sign-compare -I/usr/include/libxml2   -I/usr/include/libxml2   -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i586 -mtune=generic -fasynchronous-unwind-tables -MT netcf.lo -MD -MP -MF .deps/netcf.Tpo -c -o netcf.lo netcf.c
<<<

Actually, it's _not_ duplicated CFLAGS as it seemed first, but that you AC_SUBST default NETCF_CFLAGS in the configure script (precisely: -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fasynchronous-unwind-tables), which are added a second time with Fedora's global optflags. I've expected Fedora's CFLAGS to override the defaults except for a hardcoded -fexceptions (which you want for interoperability with netcf API users written in C++).


* pkgconfig file issues. The result of adding the "Requires" to the .pc file is this:

$ pkg-config --cflags netcf
-I/usr/include/libxml2

$ pkg-config --libs netcf
-lnetcf -laugeas -lxslt -lz -lm -lxml2  

This is not needed for shared linking (the default with Fedora), because you don't need all those libs and headers when building with the libnetcf API. You only need them when linking statically (comment 2), and then you would prefer "Requires.private" and "Libs.private" and "pkg-config --static ...". The current pkgconfig inter-package dependencies lead to dependency-bloat.
Comment 6 Michael Schwendt 2009-04-16 07:36:30 EDT
And similar to my earlier comment 2, on Fedora <= 10, where there are no automatic pkgconfig RPM dependencies added by rpmbuild, the netcf.pc file's current dependencies lead to missing RPM Requires in netcf-devel (for the needed -devel pkgs). So, really better switch to "Requires.private" and "Libs.private".
Comment 7 David Lutterkort 2009-04-16 17:30:44 EDT
I'll fix up the pkg-config file to use Requires.private in the next release.

As for the duplicate CFLAGS, I would realy like to keep NETCF_CFLAGS for development builds, and even if I build with CFLAGS then.

Thanks for the thorough review
Comment 8 David Lutterkort 2009-04-16 17:32:21 EDT
New Package CVS Request
=======================
Package Name: netcf
Short Description: Cross-platform network configuration library
Owners: lutter
Branches: F-10 F-11 EL-5
InitialCC:
Comment 9 Kevin Fenzi 2009-04-17 17:31:54 EDT
cvs done.

Note You need to log in before you can comment on or make changes to this bug.