Bug 461338

Summary: Review Request: dahdi-tools - Userspace tools to configure the DAHDI kernel modules
Product: [Fedora] Fedora Reporter: Jeffrey C. Ollie <jeff>
Component: Package ReviewAssignee: Peter Robinson <pbrobinson>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: amessina, fedora-package-review, notting, pbrobinson, pugbug, wacker
Target Milestone: ---Flags: pbrobinson: fedora-review+
huzaifas: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-10-19 21:10:29 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 465661    

Description Jeffrey C. Ollie 2008-09-06 04:24:19 UTC
Spec URL: http://fedorapeople.org/~jcollie/dahdi/dahdi.spec
SRPM URL: http://fedorapeople.org/~jcollie/dahdi/dahdi-2.0.0-0.1.rc2.fc9.src.rpm
Description:

DAHDI stands for Digium Asterisk Hardware Device Interface. This
package contains the userspace tools to configure the DAHDI kernel
modules.  DAHDI is the replacement for Zaptel, which must be renamed
due to trademark issues.

Once the final version of DAHDI is released, the Zaptel packages will be obsoleted and removed from Fedora.

Comment 1 Peter Robinson 2008-10-07 10:52:27 UTC
version 2 for both -tools and -linux is out now. Once you have updated them to the 2.0 final release I can do a review. Is there a reason the two packages are combined?

I also noticed you add a user as part of the install but dont remove it on uninstall, not sure what the standard for that is though.

Comment 2 Jeffrey C. Ollie 2008-10-08 17:49:13 UTC
Spec URL: http://fedorapeople.org/~jcollie/dahdi/dahdi.spec
SRPM URL: http://fedorapeople.org/~jcollie/dahdi/dahdi-2.0.0-0.4.fc9.src.rpm

(In reply to comment #1)
> version 2 for both -tools and -linux is out now. Once you have updated them to
> the 2.0 final release I can do a review. Is there a reason the two packages are
> combined?

To compile the -tools package you need some header files from the -linux package.  Since the -linux package contains the kernel modules it can't be made available as a package for Fedora so I just included the tarball in the -tools package rather than extracting the individual header files.  I just figured that it would be simpler to keep things in sync if I included the whole tarball.

> I also noticed you add a user as part of the install but dont remove it on
> uninstall, not sure what the standard for that is though.

I'm pretty sure the standard is to not remove users because unless you scan the whole filesystem you don't know if there are any files owned by the user.

Comment 3 Peter Robinson 2008-10-09 09:13:04 UTC
A few small bits that need to be cleaned up.

+ rpmlint output

rpmlint -i dahdi-2.0.0-0.4.fc9.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint -i dahdi*2.0.0-0.4.fc9.x86_64.rpm
dahdi.x86_64: E: no-status-entry /etc/rc.d/init.d/dahdi
In your init script (/etc/rc.d/init.d/your_file), you don't have a 'status'
entry, which is necessary for good functionality.
5 packages and 0 specfiles checked; 1 errors, 0 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license

+ %doc includes license file
  you don't need to include README etc in all subpackages, just the main package
+ spec file written in American English
+ spec file is legible

+ upstream sources match sources in the srpm

c09f880e67305bf5561c8030958b9fb9  dahdi-linux-2.0.0.tar.gz
31e48ed37e43662b0f8fbf146e192b66  dahdi-tools-2.0.0.tar.gz

+ package successfully builds on at least one architecture
  tested with a koji scratch build on all platforms
- BuildRequires list all build dependencies
  the perl BuildReq needs to be moved up with the rest, the BuildReq are for entire package group, not sub packages
n/a %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
- packages containing GUI apps must include %{name}.desktop file
  see gtk2 point below.
+ packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock
+ the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described (basic testing)
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

A couple of other things:

- Why are the modprobe noreplace? If there are new devices or blacklists added between releases this wouldn't be automatically picked up.
%config(noreplace) %{_sysconfdir}/modprobe.d/dahdi
%config(noreplace) %{_sysconfdir}/modprobe.d/dahdi.blacklist
- the main dahdi package requires the perl modules so should depend on it. Are the perl modules usable without the main package installed, if not there is no point of the subpackage
- why does it need gtk2-devel? It builds fine without it and the configure script doesn't check for it.
- The release number should be 2.0.0-1 not 2.0.0-0.4 (ie the final 0.x usually designates a pre or RC release)

Comment 4 Anthony Messina 2008-10-09 09:41:54 UTC
Excellent work Jeff, and thank you!. I've been following your devel process on this as I'm just about prepared to switch over to DAHDI (when you're able to build the asterisk-1.6 RPMs.  In conjunction with that, I'll be building my own kernel modules package and I'm trying to keep it so that it will fit nicely with your official Fedora packages.

On that note, would it be unreasonable to suggest that the resulting package that contains everything except -per -libs and -devel be named "dahdi-tools" to stay inline with the upstream naming?

Then folks would be looking for:

dahdi-tools
dahdi-perl
dahdi-libs
dahdi-devel

from Fedora, and looking for (perhaps):

dahdi-linux-`uname -r`
dahdi-firmware

either by building themselves or from people like me or Axel Thimm.

It's not a big deal, just a suggestion.  Again, thanks for the work. I hope to see this accepted soon so we can move up on asterisk-1.6.1

Comment 5 Peter Robinson 2008-10-09 09:56:29 UTC
It would be dahdi-tools-* for the rest and is probably reasonable as even though it uses -linux for building it is essentially the dahdi-tools package.

Comment 6 Jeffrey C. Ollie 2008-10-09 19:31:33 UTC
Spec URL: http://fedorapeople.org/~jcollie/dahdi/dahdi-tools.spec
SRPM URL: http://fedorapeople.org/~jcollie/dahdi/dahdi-tools-2.0.0-1.fc9.src.rpm

I believe that this addresses all of the points made so far, except:

(In reply to comment #3)
> - Why are the modprobe noreplace? If there are new devices or blacklists added
> between releases this wouldn't be automatically picked up.
> %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi
> %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi.blacklist

I marked them this way so that if the user makes changes to these files they won't get blown away when an upgrade happens.  If the user doesn't make any changes the new files should get overwritten silently.

Comment 7 Anthony Messina 2008-10-09 19:56:20 UTC
I like it, but of course, it's just the opinion of a future user :)

I also agree with you about the:

> %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi
> %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi.blacklist

Those files are where each system gets configured for what cards are in it.  I wouldn't want to start loading other modules than what I told it to load.

How long until Fedora 8 & 9 updates reo? or at least updates-testing?

Comment 8 Peter Robinson 2008-10-09 20:04:50 UTC
(In reply to comment #7)
> I like it, but of course, it's just the opinion of a future user :)
> 
> I also agree with you about the:
> 
> > %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi
> > %config(noreplace) %{_sysconfdir}/modprobe.d/dahdi.blacklist
> 
> Those files are where each system gets configured for what cards are in it.  I
> wouldn't want to start loading other modules than what I told it to load.

It was a query, I'm use to most kernel modules being auto-configured so that what is put in place by the is usually enough so if the distro updated the blacklist etc you'd want it updated. If these cards are different that's fine.

> How long until Fedora 8 & 9 updates reo? or at least updates-testing?

Well the package would need to get into rawhide first, then be pushed to testing from there.

Comment 9 Jeffrey C. Ollie 2008-10-09 20:32:13 UTC
Forgot to mention:

dahdi-tools.i386: W: incoherent-init-script-name dahdi

Is the only remaining rpmlint complaint.

Comment 10 Peter Robinson 2008-10-09 21:02:51 UTC
(In reply to comment #9)
> Forgot to mention:
> 
> dahdi-tools.i386: W: incoherent-init-script-name dahdi
> 
> Is the only remaining rpmlint complaint.

Doing a quick look through /etc/init.d on one of my systems I can see nfs/nfslock are in the nfs-utils package, the bluez-utils has a number of different named init scripts to name a few. 

Just finishing up the review now.

Comment 11 Peter Robinson 2008-10-09 21:06:31 UTC
It looks good now to me. Approved!

+ rpmlint output

$ rpmlint -v dahdi-tools-2.0.0-1.fc9.src.rpm
dahdi-tools.src: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
$ rpmlint -v dahdi-tools*2.0.0-1.fc9.x86_64.rpm
dahdi-tools.x86_64: I: checking
dahdi-tools.x86_64: W: incoherent-init-script-name dahdi
dahdi-tools-debuginfo.x86_64: I: checking
dahdi-tools-devel.x86_64: I: checking
dahdi-tools-libs.x86_64: I: checking
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license

+ %doc includes license file
  you don't need to include README etc in all subpackages, just the main package
+ spec file written in American English
+ spec file is legible

+ upstream sources match sources in the srpm

c09f880e67305bf5561c8030958b9fb9  dahdi-linux-2.0.0.tar.gz
31e48ed37e43662b0f8fbf146e192b66  dahdi-tools-2.0.0.tar.gz

+ package successfully builds on at least one architecture
  tested with a koji scratch build on all platforms
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock
+ the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described (basic testing)
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

Comment 12 Jeffrey C. Ollie 2008-10-09 21:11:21 UTC
Thanks for the review!

New Package CVS Request
=======================
Package Name: dahdi-tools
Short Description: Userspace tools to configure the DAHDI kernel modules
Owners: jcollie
Branches: F-10 F-9 EL-5
InitialCC: jcollie

Comment 13 Jeffrey C. Ollie 2008-10-09 21:12:00 UTC
Oops... add one more branch:

New Package CVS Request
=======================
Package Name: dahdi-tools
Short Description: Userspace tools to configure the DAHDI kernel modules
Owners: jcollie
Branches: F-10 F-9 F-8 EL-5
InitialCC: jcollie

Comment 14 Huzaifa S. Sidhpurwala 2008-10-10 07:30:02 UTC
cvs done

Comment 15 Peter Robinson 2008-10-19 21:10:29 UTC
Closing as its now built and in rawhide.