Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 453569 - Review Request: libmirage - library to provide access to different image formats
Summary: Review Request: libmirage - library to provide access to different image formats
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Debarshi Ray
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-01 12:14 UTC by Jiri Moskovcak
Modified: 2015-02-01 22:48 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-09-25 13:09:40 UTC
debarshir: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Jiri Moskovcak 2008-07-01 12:14:30 UTC
Spec URL: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
SRPM URL: http://people.fedoraproject.org/~jmoskovc/libmirage-1.0.0-2.fc10.src.rpm

Description: 

CD-ROM image access library, and part of the userspace-cdemu suite, a free, GPL CD/DVD-ROM device emulator for linux. It is written in C and based on GLib.  The aim of libMirage is to provide uniform access to the data stored in different image formats, by creating a representation of disc stored in image file, which is based on GObjects.

rpmlint:

libmirage.i686: E: zero-length /usr/share/doc/libmirage-1.0.0/NEWS
libmirage.i686: E: zero-length /usr/share/doc/libmirage-1.0.0/ChangeLog

These files are really empty, but will be probably used in future.

Comment 1 Debarshi Ray 2008-07-11 20:23:58 UTC
MUST Items: 

xx - rpmlint is unclean on RPM
    + [rishi@ginger x86_64]$ rpmlint libmirage-1.0.0-2.fc8.x86_64.rpm 
      libmirage.x86_64: E: zero-length /usr/share/doc/libmirage-1.0.0/ChangeLog
      libmirage.x86_64: E: zero-length /usr/share/doc/libmirage-1.0.0/NEWS
      [rishi@ginger x86_64]$

OK - follows Naming Guidelines
OK - spec file is named as %{name}.spec

xx - package does not meet Packaging Guidelines
    + http://cdemu.sourceforge.net/pkg_libmirage.php looks to be a more
appropriate choice for URL.
    + Did you try to get the patch accepted upstream? It might affect SPARC too.
    + The versioned dependencies on pkgconfig, flex and glib2-devel are not
needed. According to
https://fedoraproject.org/wiki/Packaging/Guidelines#Requires: "if the lowest
possible requirement is so old that nobody has a version older than that
installed on any target distribution release, there's no need to include the
version in the dependency at all. ... As a rule of thumb, if the version is not
required, don't add it just for fun."
    + No need to delete %{_libdir}/libmirage/*.a in %install since
--disable-static was passed to %configure.
    + Remove the empty ChangeLog and NEWS from %doc.

OK - license meets Licensing Guidelines

xx - License field meets actual license
    + Should be GPLv2+ instead of LGPLv2+. See
http://fedoraproject.org/wiki/Licensing#SoftwareLicenses

OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible

xx - sources match upstream sources
    + The gzipped tarball is no longer available at the given Source location.
You could use the bzipped tarball instead. However it looks like the bzipped
tarball has problems with parallel builds.

OK - package builds successfully
OK - ExcludeArch not needed

xx - missing build dependencies
    + In order to build the documentation 'BuildRequires: gtk-doc' is needed.

OK - no locales
OK - %post and %postun invoke ldconfig
OK - package is not relocatable

xx - missing dependency on package that creates directory
    + The -devel subpackage should have 'Requires: gtk-doc' since it puts files
in a sub-directory within /usr/share/gtk-doc.

OK - no duplicates in %file
OK - file permissions set properly
OK - %clean present

OK - macros used consistently
    + While %{name} is used in Source, libmirage is used in the rest of the
cases. You can consider using %{name} throughout the Spec file.

OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - header files in -devel
OK - no static libraries
OK - -devel has *.pc file and requires pkgconfig
OK - library files without suffix in -devel
OK - -devel requires base package
OK - no libtool archives
OK - %{name}.desktop file not needed
OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary
OK - package builds in mock successfully
OK - package builds on all supported architectures
OK - package functions as expected
OK - scriptlets are sane
OK - subpackages other than -devel are not needed
OK - pkgconfig files in -devel
OK - no file dependencies

Comment 2 Debarshi Ray 2008-07-23 04:37:31 UTC
Ping?

Comment 3 Jiri Moskovcak 2008-07-28 08:15:53 UTC
Hi,
sorry for the delay,I was on vacation. The following issues you pointed out
should be fixed:

+ http://cdemu.sourceforge.net/pkg_libmirage.php looks to be a more
-fixed
+ Did you try to get the patch accepted upstream? It might affect SPARC too.
 - accepted (should be applied in the latest released version)
+ The versioned dependencies on pkgconfig, flex and glib2-devel are not
 - fixed
+ No need to delete %{_libdir}/libmirage/*.a in %install since
-fixed
+ Remove the empty ChangeLog and NEWS from %doc.
- fixed
+ The gzipped tarball is no longer available at the given Source location.
- a new version has been released during this package review, should I update to it?
+ In order to build the documentation 'BuildRequires: gtk-doc' is needed.
- fixed
+ The -devel subpackage should have 'Requires: gtk-doc' since it puts files
- fixed
+ Should be GPLv2+ instead of LGPLv2+
- fixed


Comment 4 Debarshi Ray 2008-08-07 04:55:55 UTC
(In reply to comment #3)

Please provide URLs to your new Spec/SRPM pair, bump their Release field and update the %changelog, when you make such a series of changes. That helps in tracking the changes and progress made during the course of the review.

> + The versioned dependencies on pkgconfig, flex and glib2-devel are not
>  - fixed

The -devel subpackage still has 'Requires: pkgconfig >= 1:0.14'. It should be 'Requires: pkgconfig'.

> + The gzipped tarball is no longer available at the given Source location.
> - a new version has been released during this package review, should I update to it?

Even then the bzipped tarball of 1.0.0 is still available, while the gzipped counterpart is not. That is enough reason to fix the Source0.

Whether you want to update to a new upstream release is a different matter and is your decision (I think you should).

Also, the 'BuildRequires: pkgconfig' is not needed and should be removed, because those build dependencies which provide *.pc files should themselves bring it in.

Comment 5 Jiri Moskovcak 2008-08-07 09:21:08 UTC
Hi, 
thanks for the tips, I've updated to the latest version and made followong changes to spec:
    - fixed Source url
    - removed pkgconfig from BuildRequires
    - added shared-mime-info to Requires
    - dropped ppc patch
    - add the missing changelog

Updated files:
Spec: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
SRPM: http://people.fedoraproject.org/~jmoskovc/libmirage-1.0.0-2.fc10.src.rpm

Comment 6 Jiri Moskovcak 2008-08-07 09:24:10 UTC
sorry, wrong srpm url ^

Updated files:
Spec: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
SRPM: http://people.fedoraproject.org/~jmoskovc/libmirage-1.1.0-1.fc10.src.rpm

Comment 7 Debarshi Ray 2008-08-09 13:46:58 UTC
(In reply to comment #6)

> Updated files:
> Spec: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
> SRPM: http://people.fedoraproject.org/~jmoskovc/libmirage-1.1.0-1.fc10.src.rpm

Fails to build: http://koji.fedoraproject.org/koji/taskinfo?taskID=768031

Looks like the new version has a dependency on zlib. Please add 'BuildRequires: zlib-devel'.

Comment 8 Debarshi Ray 2008-08-09 14:02:29 UTC
(In reply to comment #6)

+  Since you are installing new mimeinfo, the MIME database needs to be updated. Moreover 'Requires: shared-mime-info' is not needed. See https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo for details.

+  You could also consider using:
   make install INSTALL="%{__install} -p" DESTDIR=%{buildroot}
   ... to preserve timestamps.

Comment 9 Jiri Moskovcak 2008-08-11 09:04:09 UTC
Hi,
I did a following changes to spec file:
 - removed shared-mime-info dependency
 - add script to %post and %postun to update mimeinfo db if s-m-info is installed
 - added zlib-devel to BuildRequires (now it builds in koji)
 - added INSTALL="%{__install} -p" to preserve timestamps

Spec: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
SRPM: http://people.fedoraproject.org/~jmoskovc/libmirage-1.1.0-2.fc10.src.rpm

Comment 10 Debarshi Ray 2008-08-24 11:53:42 UTC
The libmirage.spec that you posted and the one in the SRPM are not the same. The difference is trivial, but still:

[rishi@ginger SRPMS]$ diff -uNp libmirage.spec ../SPECS/libmirage.spec 
--- libmirage.spec      2008-08-11 14:33:55.000000000 +0530
+++ ../SPECS/libmirage.spec     2008-08-11 14:18:21.000000000 +0530
@@ -75,9 +75,8 @@ update-mime-database %{_datadir}/mime &>
 %changelog
 * Mon Aug 11 2008 Jiri Moskovcak <jmoskovc@redhat.com> - 1.1.0-2
 - more spec file fixes:
-    - added INSTALL="%{__install} -p" to preserve timestamps
+    - added INSTALL="%{__install} -p" to preserve timestamp
     - removed shared-mime-info from Requires since it's not needed
-    - added zlib-devel do BuildRequires

Comment 11 Debarshi Ray 2008-08-24 12:13:19 UTC
Rpmlint errors:

[rishi@ginger x86_64]$ rpmlint -i libmirage-1.1.0-2.fc8.x86_64.rpm 
libmirage.x86_64: E: postin-without-ldconfig /usr/lib64/libmirage.so.1.0.0
This package contains a library and its %post scriptlet doesn't call ldconfig.

libmirage.x86_64: E: postun-without-ldconfig /usr/lib64/libmirage.so.1.0.0
This package contains a library and its %postun doesn't call ldconfig.

libmirage.x86_64: E: non-empty-%post /sbin/ldconfig
libmirage.x86_64: E: non-empty-%postun /sbin/ldconfig
[rishi@ginger x86_64]$ 


Since your %post and %postun stanzas contain more than one command, you need to change them to:

%post
/sbin/ldconfig
update-mime-database %{_datadir}/mime &> /dev/null || :

%postun
/sbin/ldconfig
update-mime-database %{_datadir}/mime &> /dev/null || :

Comment 12 Jiri Moskovcak 2008-08-25 13:01:54 UTC
Fixed post and postun, it builds in koji and rpmlint is quiet.

Spec: http://people.fedoraproject.org/~jmoskovc/libmirage.spec
SRPM: http://people.fedoraproject.org/~jmoskovc/libmirage-1.1.0-3.fc10.src.rpm

Comment 13 Debarshi Ray 2008-08-29 04:33:32 UTC
Rpmlint warnings:

[rishi@ginger SPECS]$ rpmlint libmirage
libmirage.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmirage.so.1.0.0 /usr/lib64/libsndfile.so.1
libmirage.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmirage.so.1.0.0 /lib64/libdl.so.2
[rishi@ginger SPECS]$ 

As shown in https://fedoraproject.org/wiki/PackageMaintainers/Common_Rpmlint_Issues#unused-direct-shlib-dependency you can modify your %build stanza as follows:

%build
%configure --enable-gtk-doc --disable-static

# Omit unused direct shared library dependencies.
sed --in-place --expression 's! -shared ! -Wl,--as-needed\0!g' libtool

make %{?_smp_mflags}


Everything else looks fine.

+---------------------------------+
| This package is APPROVED by me. |
+---------------------------------+

Comment 14 Jiri Moskovcak 2008-09-09 13:35:30 UTC
New Package CVS Request
=======================
Package Name: libmirage
Short Description: library for reading various CD/DVD image files
Owners: jmoskovc
Branches: F-9 F-10

Comment 15 Kevin Fenzi 2008-09-09 23:48:26 UTC
cvs done.


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