Bug 505184 - Review Request: xorriso - ISO 9660 image manipulation tool
Review Request: xorriso - ISO 9660 image manipulation tool
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
http://scdbackup.sourceforge.net/xorr...
:
Depends On: 505735
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-10 18:11 EDT by Juha Tuomala
Modified: 2009-08-04 20:33 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.3.8-6.pl00.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-04 20:31:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Juha Tuomala 2009-06-10 18:11:30 EDT
Spec URL: http://tuju.fi/fedora/11/xorriso.spec
SRPM URL: http://tuju.fi/fedora/11/xorriso-0.3.8-1.fc11.src.rpm
Description: 
xorriso is a program which maps file objects from POSIX compliant
filesystems into Rock Ridge enhanced ISO 9660 filesystems and allows
session-wise manipulation of such filesystems. It can load the management
information of existing ISO images and it writes the session results to
optical media or to filesystem objects. Vice versa xorriso is able to
restore file objects from ISO 9660 filesystems.
Comment 2 Juha Tuomala 2009-06-10 18:18:12 EDT
I was planning to add RMB bindings to dolphin to extract .iso files to 'here' directory if that's possbile, but haven't looked it in detail yet.
http://techbase.kde.org/Development/Tutorials/Creating_Konqueror_Service_Menus
Comment 4 Susi Lehtola 2009-06-11 08:20:57 EDT
Assigning.
Comment 5 Susi Lehtola 2009-06-11 08:36:32 EDT
rpmlint output:
xorriso.x86_64: W: devel-file-in-non-devel-package /usr/lib64/pkgconfig/xorriso.pc
xorriso.x86_64: E: zero-length /usr/share/doc/xorriso-0.3.8/CONTRIBUTORS
3 packages and 0 specfiles checked; 1 errors, 1 warnings.

- I have no idea why a pkgconfig file is provided, usually they're only used in development packages such as libraries.

- Drop the CONTRIBUTORS file and add
 [ -s CONTRIBUTORS ] && exit 1
to %setup so that you will be notified if the file gains content later on.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK

MUST: The package must be named according to the Package Naming Guidelines. NEEDSWORK
- Change
 Release: 1%{?dist}
to
 Release: 1.%{__patchlevel}%{?dist}

MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- License is GPLv2 and GPL+ and (LGPLv2+ or MIT), not GPLv2.
* Most of the files are under GPLv2.
* cleanup is under GPL license (GPL+)
* make_isohybrid_mr is LGPLv2+ or MIT.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSWORK
- No source URL provided. Source matches upstream.

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Time stamps are not preserved, use
 make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. N/A
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A

MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. NEEDSWORK
- Add Requires: pkgconfig.

MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- Not all licenses are included.

SHOULD: The package builds in mock. OK
Comment 6 Juha Tuomala 2009-06-12 12:12:12 EDT
(In reply to comment #5)
> rpmlint output:
> xorriso.x86_64: W: devel-file-in-non-devel-package
> /usr/lib64/pkgconfig/xorriso.pc

Removed during %install and also communicated with upstream to remove it, he agreed.

> xorriso.x86_64: E: zero-length /usr/share/doc/xorriso-0.3.8/CONTRIBUTORS
> 3 packages and 0 specfiles checked; 1 errors, 1 warnings.

> - Drop the CONTRIBUTORS file and add
>  [ -s CONTRIBUTORS ] && exit 1

fixed.

> - Change
>  Release: 1%{?dist}
> to  Release: 1.%{__patchlevel}%{?dist}

fixed.

> NEEDSWORK
> - License is GPLv2 and GPL+ and (LGPLv2+ or MIT), not GPLv2.
> * Most of the files are under GPLv2.
> * cleanup is under GPL license (GPL+)
> * make_isohybrid_mr is LGPLv2+ or MIT.

fixed to: GPLv2 and GPL and LGPLv2+

 
> - No source URL provided. Source matches upstream.

fixed.

> - Time stamps are not preserved, use
>  make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

fixed.

> MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
> NEEDSWORK
> - Add Requires: pkgconfig.

Not needed as file is removed during the install and will
disappear in future releases.

> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. NEEDSWORK
> - Not all licenses are included.

Communicated with upstream. What should i do with this, start
hunting those files myself (not preferred) or wait the next
release and fix the spec to include them?

Spec URL: http://tuju.fi/fedora/11/xorriso.spec
SRPM URL: http://tuju.fi/fedora/11/xorriso-0.3.8-2.pl00.fc10.src.rpm
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1408600
Comment 7 Juha Tuomala 2009-06-12 13:32:52 EDT
Added desktop menu entry for KDE RMB over file.

[Desktop Entry]
Type=Service
ServiceTypes=KonqPopupMenu/Plugin
MimeType=application/x-cd-image;
Actions=xorrisoExtractHere;
X-KDE-Priority=TopLevel
X-KDE-StartupNotify=false

[Desktop Action xorrisoExtractHere]
Name=Xorriso: extract ISO-image here
Name[fi]=Xorriso: Pura ISO tiedosto tähän
Icon=media-optical
Exec=/bin/nice -n 19 ionice -n7 -c3 xorriso -indev "%F" -osirrox on -cp_rx "*" "$(/usr/bin/dirname %F)"

Spec URL: http://tuju.fi/fedora/11/xorriso.spec
SRPM URL: http://tuju.fi/fedora/11/xorriso-0.3.8-3.pl00.fc10.src.rpm
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1408752
Comment 8 Susi Lehtola 2009-06-12 14:16:44 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > rpmlint output:
> > xorriso.x86_64: W: devel-file-in-non-devel-package
> > /usr/lib64/pkgconfig/xorriso.pc
> 
> Removed during %install and also communicated with upstream to remove it, he
> agreed.

OK, please add comment about this to the spec file.

> > NEEDSWORK
> > - License is GPLv2 and GPL+ and (LGPLv2+ or MIT), not GPLv2.
> > * Most of the files are under GPLv2.
> > * cleanup is under GPL license (GPL+)
> > * make_isohybrid_mr is LGPLv2+ or MIT.
> 
> fixed to: GPLv2 and GPL and LGPLv2+

You are missing the + from GPL. Please use my version..

> > SHOULD: If the package does not include license text(s) as separate files from
> > upstream, the packager should query upstream to include it. NEEDSWORK
> > - Not all licenses are included.
> 
> Communicated with upstream. What should i do with this, start
> hunting those files myself (not preferred) or wait the next
> release and fix the spec to include them?

Just wait until the next release. Fedora doesn't have a policy on this unlike Debian, we just don't ship the license files if they're not present upstream.
Comment 9 Susi Lehtola 2009-06-12 14:19:35 EDT
(In reply to comment #7)
> Added desktop menu entry for KDE RMB over file.
> 
> [Desktop Entry]
> Type=Service
> ServiceTypes=KonqPopupMenu/Plugin
> MimeType=application/x-cd-image;
> Actions=xorrisoExtractHere;
> X-KDE-Priority=TopLevel
> X-KDE-StartupNotify=false
> 
> [Desktop Action xorrisoExtractHere]
> Name=Xorriso: extract ISO-image here
> Name[fi]=Xorriso: Pura ISO tiedosto tähän
> Icon=media-optical
> Exec=/bin/nice -n 19 ionice -n7 -c3 xorriso -indev "%F" -osirrox on -cp_rx "*"
> "$(/usr/bin/dirname %F)"

You need to use install the desktop file properly, see the Packaging Guidelines. Also I've never seen this type of thing used before, so I probably need to study it a bit.

And a second thing: use cp -p to keep the time stamps on the files you copy.
Comment 10 Rex Dieter 2009-06-12 14:52:54 EDT
These are service menus, not really appropriate for desktop-file-install procedures.
Comment 11 Juha Tuomala 2009-06-12 15:40:09 EDT
(In reply to comment #9)
> You need to use install the desktop file properly, see the Packaging
> Guidelines. Also I've never seen this type of thing used before, so I
> probably need to study it a bit.

I think Rex is right that this is not the regular menu entry.

> And a second thing: use cp -p to keep the time stamps on the files you copy.  

fixed, also the GPL+ and dropped CONTRIBUTORS file completely.

Probably there would be possibility to add other actions like creating an image from selected files, but it would have to default to some default name (which would cause problems if that would already exist in the given dir) or first file name with .iso extension (which lowers the collision risk, but doesn't remove it). Wouldn't be very keen to add too much of woodoo to Exec line either. Perhaps that needs more thinking and asking advice from upstream.

http://tuju.fi/fedora/11/xorriso.spec
http://tuju.fi/fedora/11/xorriso-0.3.8-4.pl00.fc10.src.rpm
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1408975
Comment 12 Susi Lehtola 2009-06-13 05:44:19 EDT
- Why do you
 rm -f CONTRIBUTORS
as it isn't going anywhere...?

- You need to own
 %{_datadir}/kde4/services/ServiceMenus/
and Requires: kde-filesystem for the desktop file.
Comment 13 Susi Lehtola 2009-06-13 05:56:47 EDT
rpmlint output:
xorriso.x86_64: W: incoherent-version-in-changelog 0.3.8-4 ['0.3.8-4.pl00.fc11', '0.3.8-4.pl00']
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- Add the missing patchlevel suffix to the last item in the changelog.

- Also, you might want to use
 install -D -p -m 644 %{SOURCE1} %{buildroot}/%{_datadir}/kde4/services/ServiceMenus/xorriso_servicemenu.desktop
instead of
 mkdir -p %{buildroot}/%{_datadir}/kde4/services/ServiceMenus
 cp -p %{SOURCE1} %{buildroot}/%{_datadir}/kde4/services/ServiceMenus

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. ~OK
- The MIT bit is still missing, but I guess that's okay.
- In fact, when you think of it, the resulting license of the whole shebang is GPLv2, since it's compatible with every one of the licenses in the package, so you were right from the start :)

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A

MUST: A package must own all directories that it creates or require the package that owns the directory. NEEDSWORK
- You are missing ownership of servicemenu dir as stated in #12.

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. ???
- Must check this.

MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 14 Juha Tuomala 2009-06-13 06:02:59 EDT
(In reply to comment #12)
> - Why do you
>  rm -f CONTRIBUTORS
> as it isn't going anywhere...?

fixed.

> - You need to own
>  %{_datadir}/kde4/services/ServiceMenus/
> and Requires: kde-filesystem for the desktop file.  

I'm not willing to pull desktop packages for CLI tool.
That menu entry is just convenience entry. Even I would
like to, why kde-filesystem? 

$ rpm -ql kde-filesystem|grep -c ServiceMenus;cat /etc/fedora-release
0
Fedora release 10 (Cambridge)

That dir is not owned by any particular kde 'base' pkg, my installed ones:

$ rpm -qf /usr/share/kde4/services/ServiceMenus
kplayer-0.7.0-1.20081211cvs.fc10.x86_64
kdesvn-1.3.0-1.fc10.x86_64
kdeutils-4.2.3-1.fc10.x86_64
kdenetwork-4.2.3-1.fc10.x86_64
kdebase-4.2.3-1.fc10.x86_64
kdemultimedia-4.2.3-1.fc10.x86_64
konq-plugins-4.2.3-1.fc10.x86_64
kdebase-workspace-4.2.3-4.fc10.x86_64

last package only drops its own 
  /usr/share/kde4/services/ServiceMenus/installfont.desktop
entry there, so it falls to same category with others.

http://tuju.fi/fedora/11/xorriso.spec
Comment 15 Mamoru TASAKA 2009-06-13 06:07:33 EDT
Well, about the directory /usr/share/kde4/services/ServiceMenus :

# repoquery --repoid=koji-12 --whatprovides /usr/share/kde4/services/ServiceMenus | sort
kdebase-6:4.2.90-1.fc12.i586
kdebase-workspace-0:4.2.90-2.fc12.i586
kdemultimedia-6:4.2.90-2.fc12.i586
kdenetwork-7:4.2.90-1.fc12.i586
kdesdk-0:4.2.90-1.fc12.i586
kdesvn-0:1.3.0-1.fc12.i586
kdeutils-6:4.2.90-1.fc12.i586
konq-plugins-0:4.2.2-1.fc11.i586

I think kde-filesystem should own this directory and
these other packages should not own it. I will ask
kde people about how they think of this.
Comment 16 Susi Lehtola 2009-06-13 06:17:50 EDT
(In reply to comment #14)
> - You need to own
> >  %{_datadir}/kde4/services/ServiceMenus/
> > and Requires: kde-filesystem for the desktop file.  
> 
> I'm not willing to pull desktop packages for CLI tool.
> That menu entry is just convenience entry. Even I would
> like to, why kde-filesystem? 

kde-filesystem is NOT a desktop package. 

$ rpm -qi kde-filesystem|grep Size
Size        : 3551                             License: Public Domain

$ rpm -qR kde-filesystem
config(kde-filesystem) = 4-25.fc11
filesystem  
rpm  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(VersionedDependencies) <= 3.0.3-1
Comment 17 Susi Lehtola 2009-06-13 08:50:18 EDT
At least kdesdk doesn't use any desktop installs, so I guess the desktop file is OK.
Comment 18 Juha Tuomala 2009-06-13 16:23:41 EDT
Dir ownership issue is now solved.

http://tuju.fi/fedora/11/xorriso.spec
Comment 19 Susi Lehtola 2009-06-13 17:27:28 EDT
Hmm, I don't see the desktop file in the %files section. Did you try building the new spec file..?

Also I think you miscopied the install part:

 install -D -p -m 644 %{SOURCE1}
%{buildroot}/%{_datadir}/kde4/services/ServiceMenus/xorriso_servicemenu.desktop

You have to specify the name of the file in the destination, now it installs the desktop file as the file %{buildroot}/%{_datadir}/kde4/services/ServiceMenus.
Comment 20 Juha Tuomala 2009-06-14 05:49:56 EDT
(In reply to comment #19)
> Hmm, I don't see the desktop file in the %files section. Did you try building
> the new spec file..?

Forgot to copy new file into tuju.fi, reload please.
Comment 21 Susi Lehtola 2009-06-14 15:42:48 EDT
Please build srpms too, as not having them makes having a clean review system a PITA.

Also, you are still missing the latter part of the install command as I said in #19, currently the build fails in

RPM build errors:
    File not found: /builddir/build/BUILDROOT/xorriso-0.3.8-5.pl00.fc11.x86_64/usr/share/kde4/services/ServiceMenus/xorriso_servicemenu.desktop
Comment 22 Susi Lehtola 2009-07-05 06:39:36 EDT
ping?
Comment 23 Juha Tuomala 2009-07-05 11:58:11 EDT
(In reply to comment #21)
> RPM build errors:
>     File not found:
> /builddir/build/BUILDROOT/xorriso-0.3.8-5.pl00.fc11.x86_64/usr/share/kde4/services/ServiceMenus/xorriso_servicemenu.desktop  

fixed. 

Spec URL: http://tuju.fi/fedora/11/xorriso.spec
SRPM URL: http://tuju.fi/fedora/11/xorriso-0.3.8-6.pl00.fc10.src.rpm
f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1455129
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1455136
Comment 24 Susi Lehtola 2009-07-05 13:52:50 EDT
Ok, seems fine.

APPROVED
Comment 25 Juha Tuomala 2009-07-05 18:42:50 EDT
New Package CVS Request
=======================
Package Name: xorriso
Short Description: ISO 9660 image manipulation tool
Owners: tuju
Branches: f10 f11 EL4 EL5
InitialCC:
Comment 26 Kevin Fenzi 2009-07-06 00:06:15 EDT
cvs done.
Comment 27 Fedora Update System 2009-07-06 06:55:07 EDT
xorriso-0.3.8-6.pl00.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xorriso-0.3.8-6.pl00.fc10
Comment 28 Fedora Update System 2009-07-06 06:56:09 EDT
xorriso-0.3.8-6.pl00.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xorriso-0.3.8-6.pl00.fc11
Comment 29 Fedora Update System 2009-07-11 12:58:24 EDT
xorriso-0.3.8-6.pl00.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xorriso'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-7427
Comment 30 Fedora Update System 2009-07-11 13:00:05 EDT
xorriso-0.3.8-6.pl00.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xorriso'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-7433
Comment 31 Fedora Update System 2009-08-04 20:31:05 EDT
xorriso-0.3.8-6.pl00.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 32 Fedora Update System 2009-08-04 20:33:52 EDT
xorriso-0.3.8-6.pl00.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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