Bug 488359 - Review Request: dcbd - daemon and configuration tool for data center bridging
Review Request: dcbd - daemon and configuration tool for data center bridging
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-03 16:46 EST by Andy Gospodarek
Modified: 2014-06-29 19:01 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-23 09:26:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Build failure log (63.57 KB, text/plain)
2009-03-09 06:06 EDT, Jan Zeleny
no flags Details
Makefile.am patch - enable system libconfig (430 bytes, patch)
2009-03-09 11:43 EDT, Jan Zeleny
no flags Details | Diff

  None (edit)
Description Andy Gospodarek 2009-03-03 16:46:38 EST
Spec URL: http://people.redhat.com/agospoda/dcbd/dcbd.spec
SRPM URL: http://people.redhat.com/agospoda/dcbd/dcbd-0.9.7-1.fc10.src.rpm
Description: 

This package contains the Linux user space daemon and configuration tool for
Intel Enhanced Ethernet for the Data Center software.
Comment 1 Dan Horák 2009-03-05 05:29:46 EST
adding future maintainer to CC
Comment 2 Dan Horák 2009-03-05 06:17:30 EST
- upstream download page says that 0.9.4 is the latest version (https://sourceforge.net/project/showfiles.php?group_id=42302) => check with upstream
- wants to build with the included libconfig => BuildRequires: libconfig-devel is missing, but then it fails to build on Rawhide due some undefined symbols during linking
- are the headers alone in -devel subpackage (no library there) useful for any development?
Comment 3 Andy Gospodarek 2009-03-06 09:45:50 EST
(In reply to comment #2)
> - upstream download page says that 0.9.4 is the latest version
> (https://sourceforge.net/project/showfiles.php?group_id=42302) => check with
> upstream

I got this direct from Intel.  I can harass them to update the stuff on their sourceforge page, but this is the latest.

> - wants to build with the included libconfig => BuildRequires: libconfig-devel
> is missing, but then it fails to build on Rawhide due some undefined symbols
> during linking

Can you paste of attach the build failure message?  I was able to build and install it just find on an f11 alpha system.

> - are the headers alone in -devel subpackage (no library there) useful for any
> development?  

They are useful for anyone who might want to write an app that builds on the infrastructure in place and used by dcbtool/dcbd.  I think it could probably be dropped though.
Comment 4 Jan Zeleny 2009-03-09 06:06:33 EDT
Created attachment 334487 [details]
Build failure log

I'm attaching log of failed build. Build failed when I added libconfig-devel to BuildRequires field. I'm currently trying to fix this, the fix should be aviable soon.
Comment 5 Dan Horák 2009-03-09 06:28:30 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > - upstream download page says that 0.9.4 is the latest version
> > (https://sourceforge.net/project/showfiles.php?group_id=42302) => check with
> > upstream
> 
> I got this direct from Intel.  I can harass them to update the stuff on their
> sourceforge page, but this is the latest.

That's what I have expected :-) It's not a blocker, the source URL works, but should be fixed.

> > - wants to build with the included libconfig => BuildRequires: libconfig-devel
> > is missing, but then it fails to build on Rawhide due some undefined symbols
> > during linking
> 
> Can you paste of attach the build failure message?  I was able to build and
> install it just find on an f11 alpha system.

Did you have "libconfig-devel" installed?

> > - are the headers alone in -devel subpackage (no library there) useful for any
> > development?  
> 
> They are useful for anyone who might want to write an app that builds on the
> infrastructure in place and used by dcbtool/dcbd.  I think it could probably be
> dropped though.  

OK

Jan should be already in contact with you, because he was designated as the maintainer of dcbd package in the BaseOS Engineering.
Comment 6 Jan Zeleny 2009-03-09 11:43:35 EDT
Created attachment 334548 [details]
Makefile.am patch - enable system libconfig

This patch adds support for libconfig shared in system.
Comment 7 Andy Gospodarek 2009-03-09 14:21:19 EDT
Sounds like this is under control -- that is good.  I did have libconfig-devel installed since it probably got pulled into my f11 alpha system when I was doing some other builds.

Thanks for taking care of this package, Jan!  :-)
Comment 8 Jan Zeleny 2009-03-10 05:44:53 EDT
Updated package is aviable:
http://jzeleny.fedorapeople.org/packages/dcbd-0.9.7-1.fc10.src.rpm

Dan, could you please review it?
Comment 9 Jan Zeleny 2009-03-10 09:42:04 EDT
Ok, after review and some fixes:
Updated SRPM: http://jzeleny.fedorapeople.org/packages/dcbd-0.9.7-2.fc10.src.rpm
Updated SPEC: http://jzeleny.fedorapeople.org/packages/dcbd-0.9.7-2.fc10.spec
Comment 10 Dan Horák 2009-03-11 04:59:27 EDT
formal review is here, see the notes below:

OK	source files match upstream:
	    2f67e0c17ffef6fbc3de67234a6eb42cd281f449  dcbd-0.9.7.tar.gz
OK	package meets naming and versioning guidelines.
OK*	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (GPLv2). License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
BAD	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	correct scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	not a GUI app.

- use plain dcbd.spec in the "Updated SPEC" URL
- rpmlint complains a bit:
dcbd.x86_64: W: service-default-enabled /etc/rc.d/init.d/dcbd
dcbd.x86_64: W: service-default-enabled /etc/rc.d/init.d/dcbd
    both in chkconfig and LSB sections
dcbd.x86_64: E: missing-mandatory-lsb-keyword Short-Description in /etc/rc.d/init.d/dcbd
 => https://fedoraproject.org/wiki/Packaging/SysVInitScript

- non-standard dir (/etc/sysconfig/dcbd/) is used for application config files
Comment 11 Jan Zeleny 2009-03-11 13:44:39 EDT
Updated SRPM: http://jzeleny.fedorapeople.org/packages/dcbd/dcbd-0.9.7-3.fc10.src.rpm
Updated SPEC: http://jzeleny.fedorapeople.org/packages/dcbd/dcbd.spec

All issues should be fixed now.
Comment 12 Dan Horák 2009-03-12 05:56:20 EDT
few issues still remain:
- the initscript scriptlets (%post/%preun/%postun) doesn't conform to https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscript_packaging (redirection to /dev/null is missing and wrong comparison in %postun)
- rpmlint is not silent, because there are few typos in the initscript patch
dcbd.x86_64: E: unknown-lsb-keyword # Short-description: Data Center Bridging Exchange protocol daemon
dcbd.x86_64: E: missing-mandatory-lsb-keyword Short-Description in /etc/rc.d/init.d/dcbd
 => self-explained
dcbd.x86_64: E: no-chkconfig-line /etc/rc.d/init.d/dcbd
 => your line is "chkconfig 20 80", but it must be "chkconfig - 20 80"
Comment 13 Jan Zeleny 2009-03-13 08:30:03 EDT
Updated SRPM:
http://jzeleny.fedorapeople.org/packages/dcbd/dcbd-0.9.7-3.fc10.src.rpm
Updated SPEC: http://jzeleny.fedorapeople.org/packages/dcbd/dcbd.spec

Tried it with rpmlint myself this time (with no output), so hopefully it's ok now.
Comment 14 Jan Zeleny 2009-03-20 06:32:23 EDT
Hopefully final version:
Updated SRPM: http://jzeleny.fedorapeople.org/packages/dcbd/dcbd-0.9.7-4.fc10.src.rpm
Updated SPEC: http://jzeleny.fedorapeople.org/packages/dcbd/dcbd.spec
Comment 15 Dan Horák 2009-03-20 06:44:18 EDT
All issues are fixed, this package is APPROVED.
Comment 16 Jan Zeleny 2009-03-20 06:52:29 EDT
New Package CVS Request
=======================
Package Name: dcbd
Short Description: Daemon and configuration tool for
Intel Enhanced Ethernet for the Data Center software
Owners: jzeleny
Branches: 
InitialCC:
Comment 17 Kevin Fenzi 2009-03-22 01:30:02 EDT
cvs done.
Comment 18 Jan Zeleny 2009-03-23 09:26:11 EDT
Package has been uploaded to CVS, closing this bug.

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