Bug 697326 - Review Request: libisoburn - Library to enable creation and expansion of ISO-9660 filesystems
Summary: Review Request: libisoburn - Library to enable creation and expansion of ISO-...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 697327
TreeView+ depends on / blocked
 
Reported: 2011-04-17 18:57 UTC by Robert Scheck
Modified: 2015-05-20 11:00 UTC (History)
6 users (show)

Fixed In Version: libisoburn-1.1.6-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-10-24 23:06:29 UTC
Type: ---
Embargoed:
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Robert Scheck 2011-04-17 18:57:36 UTC
Spec URL: http://labs.linuxnetz.de/bugzilla/libisoburn.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/libisoburn-1.0.8-1.src.rpm
Description:
Libisoburn is a front-end for libraries libburn and libisofs which
enables creation and expansion of ISO-9660 filesystems on all CD/
DVD/BD media supported by libburn. This includes media like DVD+RW,
which do not support multi-session management on media level and
even plain disk files or block devices. Price for that is thorough
specialization on data files in ISO-9660 filesystem images. And so
libisoburn is not suitable for audio (CD-DA) or any other CD layout
which does not entirely consist of ISO-9660 sessions.

Comment 1 Michael Schwendt 2011-05-02 08:54:19 UTC
Have you tried to build this yet? Submitting a koji scratch-build is recommended during package review. My local Plague build here failed with

RPM build errors:
error: Installed (but unpackaged) file(s) found:
   /usr/share/info/xorrisofs.info.gz
   /usr/share/man/man1/xorrisofs.1.gz
    Installed (but unpackaged) file(s) found:
   /usr/share/info/xorrisofs.info.gz
   /usr/share/man/man1/xorrisofs.1.gz

Comment 2 Robert Scheck 2011-05-02 08:55:50 UTC
Huh?! Yes, I tried this locally. Which target did you use?

Comment 3 Robert Scheck 2011-05-02 12:05:15 UTC
Ah! I uploaded the wrong files. Thank you for catching this. Updated files:

Spec URL: http://labs.linuxnetz.de/bugzilla/libisoburn.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/libisoburn-1.0.8-2.src.rpm

Comment 4 Robert Scheck 2011-07-10 11:34:47 UTC
New upstream version:

Spec URL: http://labs.linuxnetz.de/bugzilla/libisoburn.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/libisoburn-1.1.2-1.src.rpm

Comment 5 Dennis van Dok 2011-09-13 22:45:40 UTC
This is just an informal review.

rpmlint (minus some spell checks false positives):

libisoburn.src: W: spelling-error Summary(en_US) filesystems -> file systems, file-systems, ecosystems
The value of this tag appears to be misspelled. Please double-check.

libisoburn.x86_64: E: incorrect-fsf-address /usr/share/doc/libisoburn-1.1.2/COPYING
The Free Software Foundation address in this file seems to be outdated or
misspelled.  Ask upstream to update the address, or if this is a license file,
possibly the entire file with a new copy available from the FSF.

libisoburn.x86_64: E: incorrect-fsf-address /usr/share/doc/libisoburn-1.1.2/COPYRIGHT
The Free Software Foundation address in this file seems to be outdated or
misspelled.  Ask upstream to update the address, or if this is a license file,
possibly the entire file with a new copy available from the FSF.

xorriso.x86_64: W: no-manual-page-for-binary xorrecord
Each executable in standard binary directories should have a man page.

xorriso.x86_64: W: no-manual-page-for-binary osirrox
Each executable in standard binary directories should have a man page.

Naming:  OK

Spec file matches base name: OK

Approved license: OK

Spec file license matches actual license: OK

License in %doc: OK

Spec file in English: OK

Spec file legibility: OK

Upstream source matches SRPM: OK

Package builds in mock: OK

ExcludeArch: No excludes

BuildRequires lists build dependencies: OK

Locale handling: N/A

Library package calls ldconfig in %post, %postun: OK

Not bundling system libraries: OK

Relocatable: N/A

Package owns created directories: OK

List files only once: OK

Correct file permissions: OK

Consistent use of macros: OK

Package contains code: OK

Large doc subpackage: N/A

Documentation does not affect runtime: did not test this, but seems unlikely.

Header files in -devel: OK

Static libraries in -static: N/A

Devel package requires base package: OK

Do not install libtool files: OK

Include .desktop file for GUI apps: N/A

Packages must not own files already owned by other packages: OK

Filenames are valid UTF-8: OK


Some additional comments:

The URL in the spec file http://libburnia-project.org/ currently shows a placeholder page.

Although the subpackage xorisso implicitly depends on libisoburn, it doesn't require the
exact base package it was built with. This may not be a problem. It doesn't include the
copyright and license, and strictly speaking this is covered by the base package, but as it
is not named after the base package it might be better to include these files separately
in this package.

After installing libisoburn on a CentOS 5 box, rpmlint reports the following:

libisoburn.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libisoburn.so.1.69.0 /usr/lib64/libz.so.1
libisoburn.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libisoburn.so.1.69.0 /lib64/libacl.so.1

This suggests that upstream should review the dependencies and maybe drop the -lz and -lacl.

Installed xorriso and used it to create a 400 MB .iso file. No actual burn test as the VM
lacked a DVD burner ;-)

Final comment: there seems to be another newer upstream release, 1.1.4.

Comment 6 Michael Schwendt 2011-09-14 07:54:00 UTC
> BuildRequires lists build dependencies: OK

The one missing thing it searches for is "libjte".
That is the library from the project "JTE - Jigdo Template Export".
http://www.einval.com/~steve/software/JTE/

It is not in Fedora yet, and it is okay if this is missing.


> Although the subpackage xorisso implicitly depends on libisoburn,
> it doesn't require the exact base package it was built with. This
> may not be a problem. 

Unfortunately, it leads to problems regularly. Either when users install something without running a full update first. Or when they explicitly apply an update (such as "yum update xorisso") without letting the package installation tool apply *all* available updates. Sometimes the users do that only because broken dependencies make it impossible (or very difficult) to apply all updates.

Since xorisso is a subpackage of libisoburn, this is (sort of) covered by:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %package devel
> ...
> Requires: pkgconfig

A dependency on /usr/bin/pkg-config is is automatic at least since several releases of Fedora.

Comment 7 Robert Scheck 2011-09-14 09:28:05 UTC
(In reply to comment #5)
> Although the subpackage xorisso implicitly depends on libisoburn, it doesn't
> require the exact base package it was built with. This may not be a problem. 
> It doesn't include the copyright and license, and strictly speaking this is 
> covered by the base package, but as it is not named after the base package it 
> might be better to include these files separately in this package.

I can include the copyright/license file(s) into the xorisso subpackage.

> After installing libisoburn on a CentOS 5 box, rpmlint reports the following:
> 
> libisoburn.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libisoburn.so.1.69.0 /usr/lib64/libz.so.1
> libisoburn.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libisoburn.so.1.69.0 /lib64/libacl.so.1
> 
> This suggests that upstream should review the dependencies and maybe drop the
> -lz and -lacl.

I will review this.

> Final comment: there seems to be another newer upstream release, 1.1.4.

Yes, libisoburn releases quite often. As all of the updates were just minor
so far, I'm tempted to wait if somebody is doing a qualified review before I
am it updating once more.


(In reply to comment #6)
> Unfortunately, it leads to problems regularly. Either when users install
> something without running a full update first. Or when they explicitly apply > an update (such as "yum update xorisso") without letting the package 
> installation tool apply *all* available updates. Sometimes the users do that 
> only because broken dependencies make it impossible (or very difficult) to 
> apply all updates.

There is no need to explicitly depend on a specific version at run-time, 
because upstream cares about ABI compatibility and bumps soname if needed.

> A dependency on /usr/bin/pkg-config is is automatic at least since several
> releases of Fedora.

Yes, but not at EPEL - and this package also will go into EPEL.

Comment 8 Michael Schwendt 2011-09-14 10:34:23 UTC
> There is no need to explicitly depend on a specific version at run-time, 
> because upstream cares about ABI compatibility and bumps soname if needed.

Also for added symbols?

Comment 9 Robert Scheck 2011-09-18 22:49:52 UTC
libburn.so.4.65.0 -> libburn.so.4.67.0 (libburn 1.1.0 -> 1.1.4) for example.
If this is not enough, we still can add an explicit versioned requirement, so
let me know. Somebody interested in doing a formal review?

Comment 10 Michael Schwendt 2011-09-19 09:27:27 UTC
> libburn.so.4.65.0 -> libburn.so.4.67.0 (libburn 1.1.0 -> 1.1.4) for example.

The SONAME hasn't changed for that update. It is still 'libburn.so.4'.

[ http://koji.fedoraproject.org/koji/rpminfo?rpmID=2610788 ]
[ http://koji.fedoraproject.org/koji/rpminfo?rpmID=2705359 ]

Similarly, for libisoburn it is still 'libisoburn.so.1', so a base package dep in xorisso subpkg will be needed. Comment 6.

> a formal review?

Dunno what that is, but I've been planning to approve this whenever I find the time to review it properly.

Comment 11 Michael Schwendt 2011-10-05 09:48:48 UTC
* Wed Sep 28 2011 Release 1.1.6 : "libisoburn now comes with a test suite" - that is just a file comparison executable to be run manually. It is not a full suite that could be run in the %check section.

* Upstream offers a detached signature file that could be included
as another %{SOURCEn} file.

* A couple of packages in the distribution explicitly "Requires: util-linux", but I don't think this should be added also for the xorriso kde helper, which uses ionice.


> %global with_kde 1

More flexible would be

  %bcond_without kde

which by default would define %with_kde to 1 and undefine it if building --without kde. The following conditionals,

> %if (%with_kde && 0%{?rhel}%{?fedora} > 5)

would need to be adjusted then, however. There are multiple solutions, such as

  %if %{with kde} && (0%{?rhel}%{?fedora} > 5)

or the more cryptic:

  %if 0%{?with_kde:1} && (0%{?rhel}%{?fedora} > 5)


> %package -n xorriso

I still think it should add an arch-specific base package dependency as discussed before.
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

And just to mention it, yes, it's possible to upgrade arbitrary libraries in Fedora with API additions, (re)build applications with them and produce builds where the automatic SONAME deps are not accurate enough. In a subpackage, one has full control over the base package name and EVR, however, whereas an explicit versioned dependency on external package NEVR bears risks.


* Unless you want to build this library for EPEL 5, you could prune some of the items which are no longer necessary:
 - BuildRoot definition
 - BuildRoot removal at beginning of %install
 - %clean section
 - %defattr lines
 - pkgconfig dependency in -devel package


* APPROVED

Comment 12 Robert Scheck 2011-10-06 06:49:15 UTC
Michael, thank you very much for the review. I will add the arch-specific base
package dependency as suggested after the import.


New Package SCM Request
=======================
Package Name: libisoburn
Short Description: Library to enable creation and expansion of ISO-9660 filesystems
Owners: robert
Branches: f14 f15 f16 el6 el5 el4
InitialCC:

Comment 13 Gwyn Ciesla 2011-10-06 12:26:53 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2011-10-14 19:55:45 UTC
libisoburn-1.1.6-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libisoburn-1.1.6-1.fc16

Comment 15 Fedora Update System 2011-10-14 19:56:32 UTC
libisoburn-1.1.6-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/libisoburn-1.1.6-1.fc15

Comment 16 Fedora Update System 2011-10-14 19:56:57 UTC
libisoburn-1.1.6-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/libisoburn-1.1.6-1.fc14

Comment 17 Fedora Update System 2011-10-15 14:30:27 UTC
libisoburn-1.1.6-1.fc16 has been pushed to the Fedora 16 testing repository.

Comment 18 Fedora Update System 2011-10-16 22:45:44 UTC
libisoburn-1.1.6-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/libisoburn-1.1.6-1.el5

Comment 19 Fedora Update System 2011-10-17 00:24:12 UTC
libisoburn-1.1.6-1.el4 has been submitted as an update for Fedora EPEL 4.
https://admin.fedoraproject.org/updates/libisoburn-1.1.6-1.el4

Comment 20 Fedora Update System 2011-10-24 23:06:29 UTC
libisoburn-1.1.6-1.fc14 has been pushed to the Fedora 14 stable repository.

Comment 21 Fedora Update System 2011-10-24 23:07:05 UTC
libisoburn-1.1.6-1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 22 Fedora Update System 2011-10-25 03:41:27 UTC
libisoburn-1.1.6-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 23 Fedora Update System 2011-11-13 01:33:00 UTC
libisoburn-1.1.6-1.el4 has been pushed to the Fedora EPEL 4 stable repository.

Comment 24 Fedora Update System 2011-11-13 01:33:58 UTC
libisoburn-1.1.6-1.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 25 Robert Scheck 2015-05-19 23:06:01 UTC
Package Change Request
======================
Package Name: libisoburn
New Branches: epel7
Owners: robert

Comment 26 Gwyn Ciesla 2015-05-20 11:00:37 UTC
Git done (by process-git-requests).


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