Bug 453839 - Review Request: phatch - photo batch processor
Review Request: phatch - photo batch processor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-02 17:14 EDT by Nicoleau Fabien
Modified: 2009-01-07 04:35 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-07 04:18:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nicoleau Fabien 2008-07-02 17:14:37 EDT
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL: http://nicoleau.fabien.free.fr/rpms/srpms.fc9/phatch-0.1.4.bzr538-1.fc9.src.rpm

Description: 
Phatch is a simple to use cross-platform GUI Photo Batch Processor which handles all popular image formats and can duplicate (sub)folder hierarchies. Phatch can batch resize, rotate, apply perspective, shadows, rounded corners, ... and more in minutes instead of hours or days if you do it manually. Phatch allows you to use EXIF and IPTC tags for renaming and data stamping. Phatch also supports a console version to batch photos on webservers.

Rebuild under mock is OK.

rpmlint output :
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.4.bzr538-1.fc9.noarch.rpm 
phatch.noarch: E: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.4.bzr538-1.fc9.src.rpm 
phatch.src:71: W: libdir-macro-in-noarch-package %{_libdir}/nautilus/extensions-1.0/python/%{name}_*
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
[builder@FEDOBOX tmp]$ 

Warning about binaries is because of files that are located in "/usr/lib/nautilus/extensions-1.0/python/" (pkg-config --variable=pythondir nautilus-python used in setup.py).
Comment 1 Jason Tibbitts 2008-07-02 21:05:48 EDT
In case you didn't know, you should be able to avoid all of that nasty %lang
business by using the %find_lang macro.  See
http://fedoraproject.org/wiki/Packaging/Guidelines#Why_do_we_need_to_use_.25find_lang.3F
Comment 2 Nicoleau Fabien 2008-07-03 07:29:29 EDT
Update :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc9/phatch-0.1.4.bzr538-2.fc9.src.rpm

I'm now using %find_lang macro.

Rebuild under mock is OK.

rpmlint output :
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.4.bzr538-2.fc9.noarch.rpm 
phatch.noarch: E: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.4.bzr538-2.fc9.src.rpm 
phatch.src:73: W: libdir-macro-in-noarch-package
%{_libdir}/nautilus/extensions-1.0/python/%{name}_*
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
[builder@FEDOBOX tmp]$ 
Comment 3 Nicoleau Fabien 2008-07-21 13:44:14 EDT
Update for 0.1.5 :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc9/phatch-0.1.5-1.fc9.src.rpm


Rebuild under mock is OK.
rpmlint output :
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.5-1.fc9.noarch.rpm
phatch-0.1.5-1.fc9.src.rpm
phatch.noarch: E: only-non-binary-in-usr-lib
phatch.src:76: W: libdir-macro-in-noarch-package
%{_libdir}/nautilus/extensions-1.0/python/%{name}_*
2 packages and 0 specfiles checked; 1 errors, 1 warnings.
[builder@FEDOBOX tmp]$ 

Comment 4 manuel wolfshant 2008-07-23 14:57:28 EDT
Just took a glimpse at the program's site and it seems that the license is
GPLv3+. Most of the .py files I've seen either have no license or contain the
following:
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version
Comment 5 Nicoleau Fabien 2008-07-23 15:57:03 EDT
Update (licence is now GPLv3+) :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc9/phatch-0.1.5-2.fc9.src.rpm

Rebuild under mock is OK.
rpmlint output :
[builder@FEDOBOX tmp]$ rpmlint phatch-0.1.5-2.fc9.noarch.rpm
phatch-0.1.5-2.fc9.src.rpm
phatch.noarch: E: only-non-binary-in-usr-lib
phatch.src:76: W: libdir-macro-in-noarch-package
%{_libdir}/nautilus/extensions-1.0/python/%{name}_*
2 packages and 0 specfiles checked; 1 errors, 1 warnings.
[builder@FEDOBOX tmp]$ 
Comment 6 Jason Tibbitts 2008-08-10 15:51:46 EDT
This rpmlint complaint:
  phatch.src:76: W: libdir-macro-in-noarch-package 
   %{_libdir}/nautilus/extensions-1.0/python/%{name}_*
is an absolute blocker.

If you build this noarch package on x86_64, you'll get files in /usr/lib64, which doesn't even exist on a 32-bit machine.  I do not know what the proper solution is; if nautilus really has no place to put arch-independent extensions then I suppose this package can't be noarch.
Comment 7 Nicoleau Fabien 2008-11-09 17:10:09 EST
Update for 0.1.6 :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc9/phatch-0.1.6-1.fc9.src.rpm

Its no more a noarch package, has %{_libdir} is used.
No debuginfo generated.

Package build on koji : http://koji.fedoraproject.org/koji/taskinfo?taskID=923733
rpmlint output :

[eponyme@FEDOBOX tmp]$ rpmlint phatch-0.1.6-1.fc9.i386.rpm phatch-0.1.6-1.fc9.src.rpm 
phatch.i386: E: no-binary
phatch.i386: E: only-non-binary-in-usr-lib
2 packages and 0 specfiles checked; 2 errors, 0 warnings.
[eponyme@FEDOBOX tmp]$
Comment 8 Orcan Ogetbil 2008-12-18 01:25:00 EST
Two comments (I'll do a full review today or tomorrow):

* The nautilus extension dependency pulls almost the entire gnome suite. The KDE (or other DE) user don't need that. I think it should go into a subpackage.

* Please preserve the timestamp of the sed'ed file.
Comment 9 Orcan Ogetbil 2008-12-18 02:49:36 EST
OK, here is the full review. In addition to the above two things:

- rpmlint errors can be ignored.

* I don't think you need to BR: wxPython-devel . The package builds fine without it.

* Source0 link is broken. Does this change frequently? If yes, I guess you should just put the filename, not the full URL (but in this case, make a comment in the SPEC file).

* Any reason why you skipped packaging docs/phatch_dev.txt in %doc ?

* Please follow the desktop-database and mimeinfo sections (4.6 and 4.7) at
    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

* The python egg shouldn't be excluded, but it should be built in a proper way. This is a non-setuptools package. So follow:
    http://fedoraproject.org/wiki/Packaging/Python/Eggs#Providing_Eggs_for_non-setuptools_packages
Comment 10 Nicoleau Fabien 2008-12-19 10:33:37 EST
Thank you for taking this review.
Update :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc10/phatch-0.1.6-2.fc10.src.rpm

Changelog :
- subpckage created for nautilus extension
- timestamp preserved for sed'ed file
- wxPython-devel removed from BuildRequires
- Source0 updated
- whole documentation included
- update-desktop-database and update-mime-database added
- egg-info properly builded
- python-devel buildrequries changed to python (whithout that, build uner mock/koji failed)

Source0 is a "stable" URL. The url changed from far but I didn't have update it in my SPEC.
I tried to use scrollkeeper but no additionnal documentation was generated, so I removed it. 
I also contacted upstream about the software version (egg file generated contains "0.1.5" instead of "0.1.6" and the "about" dialog display also "0.1.5" instead of "0.1.6").

Package build on koji :  https://bugzilla.redhat.com/show_bug.cgi?id=453839
repmlint output :
[builder@FEDOBOX ~]$ rpmlint /home/builder/rpmbuild/SRPMS/phatch-0.1.6-2.fc10.src.rpm /home/builder/rpmbuild/RPMS/i386/phatch-0.1.6-2.fc10.i386.rpm /home/builder/rpmbuild/RPMS/i386/phatch-nautilus-extension-0.1.6-2.fc10.i386.rpm
phatch.i386: E: no-binary
phatch-nautilus-extension.i386: W: no-documentation
phatch-nautilus-extension.i386: E: only-non-binary-in-usr-lib
3 packages and 0 specfiles checked; 2 errors, 1 warnings.
[builder@FEDOBOX ~]$
Comment 11 Nicoleau Fabien 2008-12-19 10:35:59 EST
oops, here is the good URL for koji build :
http://koji.fedoraproject.org/koji/taskinfo?taskID=1009427
Comment 12 Orcan Ogetbil 2008-12-19 12:14:59 EST
(In reply to comment #10)
> Thank you for taking this review.

You're welcome

> Changelog :
> - subpckage created for nautilus extension

I am a gnome-ignorant. How do I test this extension? I couldn't find anything in nautilus related to phatch. But it's probably me. I always find gnome too confusing.

   Requires: %{name} = %{version}
should be 
   Requires: %{name} = %{version}-%{release}

Also, please add the license file at the least as a %doc to this package. 

> - whole documentation included
Thanks. But now there is a docs subdirectory in a directory which is already for docs. I don't think this is elegant. Try using "docs/*" in %doc instead of just "docs"

> I tried to use scrollkeeper but no additionnal documentation was generated, so
> I removed it. 

Sorry I meant sections 4.7 or 4.8 (or they just added another section between my review and you seeing it)

> I also contacted upstream about the software version (egg file generated
> contains "0.1.5" instead of "0.1.6" and the "about" dialog display also "0.1.5"
> instead of "0.1.6").
> 

Until they fix it (possibly until next version), you can use:
   sed -e 's|0\.1\.5|0\.1\.6|' -e 's|20080606224435|200811091037|' \
       phatch/data/version.py > phatch/data/version.py.16
   touch -c -r phatch/data/version.py phatch/data/version.py.16
   mv phatch/data/version.py.16 phatch/data/version.py
This will fix both the python egg and the about dialog.
Comment 13 Nicoleau Fabien 2008-12-19 13:45:00 EST
Update :
Thank you for taking this review.
Update :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/phatch.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc10/phatch-0.1.6-3.fc10.src.rpm

Changelog :
- Fix requires for nautilus-extension
- Licence file added to nautilus-extension
- No more docs subdirectory in documentation folder
- Fix version

> I am a gnome-ignorant. How do I test this extension? I couldn't find anything
> in nautilus related to phatch. But it's probably me. I always find gnome too
> confusing.

To test the extension : right-clicking must display 2 entries related to phatch in the context-menu. Uninstalling phatch-nautilus-extension will make those entries disapears (You have to log{on/off} when you (un)install the extension to see differencies).

Package still build on koji : http://koji.fedoraproject.org/koji/taskinfo?taskID=1010031

Rpmlint output :
[builder@FEDOBOX rpmbuild]$ rpmlint /home/builder/rpmbuild/SRPMS/phatch-0.1.6-3.fc10.src.rpm /home/builder/rpmbuild/RPMS/i386/phatch-0.1.6-3.fc10.i386.rpm /home/builder/rpmbuild/RPMS/i386/phatch-nautilus-extension-0.1.6-3.fc10.i386.rpm
phatch.i386: E: no-binary
phatch-nautilus-extension.i386: E: only-non-binary-in-usr-lib
3 packages and 0 specfiles checked; 2 errors, 0 warnings.
[builder@FEDOBOX rpmbuild]$
Comment 14 Orcan Ogetbil 2008-12-19 16:54:14 EST
Everything is good, but running a quick search I found:
 
 $ yum search nautilus |grep extension
 nautilus-actions.x86_64 : Nautilus extension for customizing the context menu
 nautilus-extensions.i386 : Nautilus extensions library
 nautilus-extensions.x86_64 : Nautilus extensions library
 nautilus-image-converter.x86_64 : Nautilus extension to mass resize images
 nautilus-open-terminal.x86_64 : Nautilus extension for an open terminal shortcut
 nautilus-search-tool.x86_64 : A Nautilus extension to put "Search for Files" on
 nautilus-sound-converter.x86_64 : Nautilus extension to convert audio files

So, I guess renaming the subpackage as nautilus-%{name} would be more appropriate. What do you think?

You can do this before you commit.

-----------------------------------------
This package (phatch) is APPROVED by oget
-----------------------------------------
Comment 15 Nicoleau Fabien 2008-12-19 17:10:04 EST
Thank you very much for this review.
I'll change subpackage name to nautilus-%{name} before I commit.

New Package CVS Request
=======================
Package Name: phatch
Short Description: Photo batch processor
Owners: eponyme
Branches: F-9 F-10
InitialCC:
Comment 16 Nicoleau Fabien 2008-12-19 17:49:08 EST
PS : after a test, nautilus-%{name} will, in fact, generate phatch-nautilus-phatch, so I think I must only keep "nautilus" to have "phatch-nautilus".
Comment 17 Orcan Ogetbil 2008-12-19 18:02:01 EST
For that, you'll need to use the "-n" switch
For instance
%package -n nautilus-%{name}
%description -n nautilus-%{name}
%files -n nautilus-%{name}


(Doesn't apply to this package but you can use "-n" switch on %pre(un) %post(un) too)
Comment 18 Kevin Fenzi 2008-12-20 23:16:30 EST
cvs done.
Comment 19 Nicoleau Fabien 2008-12-21 09:15:28 EST
As I'm on hoolydays, I'll import phatch next week :D
Comment 20 Fedora Update System 2008-12-28 14:08:06 EST
phatch-0.1.6-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/phatch-0.1.6-3.fc9
Comment 21 Fedora Update System 2008-12-28 14:10:40 EST
phatch-0.1.6-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/phatch-0.1.6-3.fc10
Comment 22 Fedora Update System 2008-12-30 18:43:28 EST
phatch-0.1.6-3.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 phatch'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11859
Comment 23 Fedora Update System 2008-12-30 18:50:18 EST
phatch-0.1.6-3.fc9 has been pushed to the Fedora 9 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-newkey update phatch'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-11911
Comment 24 Fedora Update System 2009-01-07 04:18:19 EST
phatch-0.1.6-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 25 Fedora Update System 2009-01-07 04:35:54 EST
phatch-0.1.6-3.fc9 has been pushed to the Fedora 9 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.