Bug 870978 - Review Request: libcdio-paranoia - CD paranoia on top of libcdio
Summary: Review Request: libcdio-paranoia - CD paranoia on top of libcdio
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Frantisek Kluknavsky
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-10-29 10:17 UTC by Adrian Reber
Modified: 2013-01-11 16:29 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-01-11 16:29:19 UTC
Type: ---
Embargoed:
fkluknav: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Adrian Reber 2012-10-29 10:17:04 UTC
Spec URL: http://lisas.de/~adrian/rpm/libcdio-paranoia.spec
SRPM URL: http://lisas.de/~adrian/rpm/libcdio-paranoia-10.2+0.90-1.fc17.src.rpm
Description:
This CDDA reader distribution ('libcdio-cdparanoia') reads audio from the
CDROM directly as data, with no analog step between, and writes the
data to a file or pipe as .wav, .aifc or as raw 16 bit linear PCM.

Split off from libcdio to allow more flexible licensing and to be compatible
with cdparanoia-III-10.2's license. And also, libcdio is just too large.

Fedora Account System Username: adrian

libcdio has been updated t0 0.90 and libcdio-paranoia has been split off. To provide the sane functionality as before the split libcdio-paranoia needs to be imported before libcdio is updated to 0.90.

libcdio-paranoia.src: W: spelling-error %description -l en_US cdparanoia -> paranoiac, paranoia, paranoid
libcdio-paranoia.src: W: spelling-error %description -l en_US wav -> av, wave, wavy
libcdio-paranoia.src: W: spelling-error %description -l en_US aifc -> waif
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US cdparanoia -> paranoiac, paranoia, paranoid
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US wav -> av, wave, wavy
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US aifc -> waif
libcdio-paranoia-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libcdio-paranoia-10.2+0.90/src/getopt.h
4 packages and 1 specfiles checked; 1 errors, 6 warnings.

The spelling warnings can be ignored because cdparanoia is the name of the package.
I will contact upstream about the incorrect-fsf-address.

Comment 1 Adrian Reber 2012-10-30 13:46:23 UTC
Spec URL: http://lisas.de/~adrian/rpm/libcdio-paranoia.spec
SRPM URL: http://lisas.de/~adrian/rpm/libcdio-paranoia-10.2+0.90-2.fc17.src.rpm

* Tue Oct 30 2012 Adrian Reber <adrian> - 10.2+0.90-2
- added missing files from git: COPYING-GPL and COPYING-LGPL
- added patch from git from missing pkgconfig requires
  and fixed FSF address

Comment 2 Frantisek Kluknavsky 2012-10-31 16:03:13 UTC
Upstream versioning scheme is uncommon but acceptable:
  - observing past releases, we can expect linearity in the future.
  - the '+' character can be considered as a part of version tag, not as a (forbidden) delimiter.

rpmlint:
libcdio-paranoia.src: W: spelling-error %description -l en_US cdparanoia -> paranoiac, paranoia, paranoid
libcdio-paranoia.src: W: spelling-error %description -l en_US wav -> av, wave, wavy
libcdio-paranoia.src: W: spelling-error %description -l en_US aifc -> waif
libcdio-paranoia.src: W: file-size-mismatch COPYING-LGPL = 26628, https://raw.github.com/rocky/libcdio-paranoia/master/COPYING-LGPL = 1
libcdio-paranoia.src: W: file-size-mismatch COPYING-GPL = 18153, https://raw.github.com/rocky/libcdio-paranoia/master/COPYING-GPL = 1
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US cdparanoia -> paranoiac, paranoia, paranoid
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US wav -> av, wave, wavy
libcdio-paranoia.x86_64: W: spelling-error %description -l en_US aifc -> waif
All warnings were adressed in previous posts and are acceptable.

Source code is a mix of GPLv2+, GPLv3+, LGPLv2. (Package can NOT be reasonably split, so this is acceptable.) Spec file and license fulltexts do not match!

Comment 3 Adrian Reber 2012-11-02 13:02:57 UTC
I will contact upstream about the license and update the package once I know what the license is. Thanks.

Comment 4 Adrian Reber 2012-11-05 13:51:58 UTC
Spec URL: http://lisas.de/~adrian/rpm/libcdio-paranoia.spec
SRPM URL: http://lisas.de/~adrian/rpm/libcdio-paranoia-10.2+0.90-3.fc17.src.rpm

Upstream confirmed that it is LGPLv2+ and GPLv2+. The GPLv3 occurrences are from the libcdio - libcdio-paranoia split. Included patch from upstream git which changes the license headers to the correct LGPLv2+ and GPLv2+ headers.

Comment 5 Frantisek Kluknavsky 2012-11-08 10:28:09 UTC
Package is fit for Fedora.

Comment 6 Adrian Reber 2012-11-13 10:22:05 UTC
New Package SCM Request
=======================
Package Name: libcdio-paranoia
Short Description: CD paranoia on top of libcdio
Owners: adrian fkluknav
Branches: 
InitialCC:

Comment 7 Gwyn Ciesla 2012-11-13 13:22:00 UTC
Git done (by process-git-requests).

Comment 8 Michael Schwendt 2013-01-11 15:46:31 UTC
I'd thought I might find a bit of time and fix the libcdio broken dep for Audacious before Mon/Tue, but upon taking a look at how to handle this new build requirement, I've run into this:

  $ rpmls -p libcdio-paranoia-devel-10.2+0.90-5.fc19.x86_64.rpm|grep inc
  -rw-r--r--  /usr/include/cdio/cdda.h
  drwxr-xr-x  /usr/include/cdio/paranoia
  -rw-r--r--  /usr/include/cdio/paranoia.h
  -rw-r--r--  /usr/include/cdio/paranoia/cdda.h
  -rw-r--r--  /usr/include/cdio/paranoia/paranoia.h

Why are there copies of the headers in two locations?
The spec doesn't tell:

> # fix wrong installation of header files
> sed -i -e "s,includedir)/cdio,includedir)/cdio/paranoia,g" include/cdio/paranoia/Makefile.in 


There are two more mistakes which should have been caught during review:

> %package devel
> Requires: %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> # another multilib fix; remove the architecture information from version.h
> sed -i -e "s,%{version}.*$,%{version}\\\",g" include/cdio/paranoia/version.h

??? This file is not installed, not packaged, not used. Btw, if the expression didn't match, nothing would be substituted either, the "fix" would be missing. For sed substitutions like this you ought to add a guard to the spec file (e.g. via grep), or use a patch instead.

Comment 9 Adrian Reber 2013-01-11 16:29:19 UTC
(In reply to comment #8)
> I'd thought I might find a bit of time and fix the libcdio broken dep for
> Audacious before Mon/Tue, but upon taking a look at how to handle this new
> build requirement, I've run into this:
> 
>   $ rpmls -p libcdio-paranoia-devel-10.2+0.90-5.fc19.x86_64.rpm|grep inc
>   -rw-r--r--  /usr/include/cdio/cdda.h
>   drwxr-xr-x  /usr/include/cdio/paranoia
>   -rw-r--r--  /usr/include/cdio/paranoia.h
>   -rw-r--r--  /usr/include/cdio/paranoia/cdda.h
>   -rw-r--r--  /usr/include/cdio/paranoia/paranoia.h
> 
> Why are there copies of the headers in two locations?
> The spec doesn't tell:

There is a commit in the upstream git which will move the headers for the next release:

https://github.com/rocky/libcdio-paranoia/commit/b2807f3c7a4126b6078d96adbd37c3760b9f41ab

Therefore I provide the header files in the old location as well as in the new location. For the new location every package requiring it needs to be changed.

The spec has these lines:

# copy include files to an additional directory
# this will probably be the location for future releases see:
# https://github.com/rocky/libcdio-paranoia/commit/b2807f3c7a4126b6078d96adbd37c3760b9f41ab
mkdir -p $RPM_BUILD_ROOT%{_includedir}/cdio/paranoia
cp -a $RPM_BUILD_ROOT%{_includedir}/cdio/*.h $RPM_BUILD_ROOT%{_includedir}/cdio/paranoia


> > # fix wrong installation of header files
> > sed -i -e "s,includedir)/cdio,includedir)/cdio/paranoia,g" include/cdio/paranoia/Makefile.in 
> 
> 
> There are two more mistakes which should have been caught during review:
> 
> > %package devel
> > Requires: %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> 
> 
> > # another multilib fix; remove the architecture information from version.h
> > sed -i -e "s,%{version}.*$,%{version}\\\",g" include/cdio/paranoia/version.h

This is a leftover from the original libcdio.spec on which this is based. I will remove it.
 
> ??? This file is not installed, not packaged, not used. Btw, if the
> expression didn't match, nothing would be substituted either, the "fix"
> would be missing. For sed substitutions like this you ought to add a guard
> to the spec file (e.g. via grep), or use a patch instead.

Thanks for the re-review. I have updated the spec in git.

Also closing the still open bug.


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