Bug 230316 - Review Request: jbrout - Photo manager, written in python/pygtk under the GPL licence
Review Request: jbrout - Photo manager, written in python/pygtk under the GPL...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-28 04:28 EST by Matěj Cepl
Modified: 2008-06-27 14:48 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-27 14:48:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matěj Cepl 2007-02-28 04:28:09 EST
Spec URL:http://www.ceplovi.cz/matej/progs/rpms/jbrout.spec 
SRPM URL: http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.114.svn148-1.src.rpm
Description:
It's cross-platform, and has been tested on GNU/linux and windows
XP/2k.
jBrout is able to :
   * manage albums/photos (= folders/files)
   * tag photos with IPTC keywords
   * use internal jpeg thumbnail
   * comment photos (with jpeg comment) and album (textfile in
     folder)
   * rotate loss-less jpeg (and internal jpeg thumbnail)
     use EXIF info (date, size ..)
   * search pictures (tags, comment, date, ...) (not implemented
     yet)
   * use plugins (to export to html/gallery, to act like
     a httpserver, to export pictures to be mailed, ...)
   * work without database ! (just a xmlfile which can be rebuild
     from scratch)
   * handle a lot of photos (me : more than 20000)
   * upload photos to a flickr account
   * ...
Comment 1 Matěj Cepl 2007-03-17 18:17:02 EDT
Upgrade to svn150 plus protection against impact of bug 232785.

SPEC: http://www.ceplovi.cz/matej/progs/rpms/jbrout.spec
SRPMS: http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.114-2.svn150.src.rpm
Comment 2 Matěj Cepl 2007-03-19 08:40:10 EDT
New upstream version (my patches accepted upstream):

SPEC: http://www.ceplovi.cz/matej/progs/rpms/jbrout.spec
SRPMS: http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.114-2.svn153.src.rpm
Comment 3 Bernard Johnson 2007-05-04 22:42:15 EDT
Your last posted SRPM URL does not exist.
Comment 5 Bernard Johnson 2007-05-22 11:52:01 EDT
A couple of suggestions before we get started here.

1) I would remove all remnants of the svn directory structure immediately after
unpacking the archive.  That will relieve you from jumping through extra hoops
like: find plugins -type f -not -regex '.*\.svn\/.*'

2) It would probably be easier to set the file modes and clean up the shebang
and \r from files as the next step.  I see you are setting the mode (sometimes
redundantly) in multiple locations.

3) You makefile should not copy the .po or .pot files to the buildroot.  That
will also make these lines unnecessary:
%{_datadir}/locale/po/fr/LC_MESSAGES/jbrout.po
%{_datadir}/locale/po/jbrout.pot

4) Why no %doc:
#%doc changelog.txt readme.txt SciTE.properties
Comment 6 Matěj Cepl 2007-05-23 08:55:57 EDT
I hope I fixed it. New SRPM is on
http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.114.svn172-2.src.rpm
Comment 7 Bernard Johnson 2007-05-29 18:39:31 EDT
The locale files are still not being processes correctly.  I believe the .po
files are not needed at all.  The .mo files should be picked up by %find_lang
and installed in /usr/share/locale.

Look at the files that are being packages:
rpm -qpl jbrout-0.2.114.svn172-2.fc6.noarch.rpm
Comment 8 Matěj Cepl 2007-06-13 05:53:30 EDT
I have to disagree with you:

http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee

> %find_lang should be run in the %install section of your spec file, after
> all of the files have been installed into the buildroot.
Comment 9 Matěj Cepl 2007-06-13 09:57:41 EDT
Scrap the previous comment -- things are much more complicated than that. Jbrout
apparently doesn't work with .mo files in the standard way, but it has to have
them in the subdirectory po/ (both for the main program and plugins; try to run
LANG=fr_FR jbrout with the *-2 package). I am not quite sure, whether it is a
bug in jbrout, but anyway I would prefer to have functional program, so I have
followed its lead. Hopefully there isn't anything in our guidelines saying that
locales HAVE TO be in /usr/share/locale.

Updated package has the same .spec file and SRPMS is 
http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.114.svn172-3.fc7.src.rpm
Comment 10 Matěj Cepl 2007-06-13 09:59:37 EDT
s/same \.spec file/same URL of the .spec file/
Comment 11 Bernard Johnson 2007-06-14 15:39:56 EDT
I think the locale problem can be fixed up pretty easily, but I have found a
more serious problem:

This program requires python-elementtree and because of changes in
python-elementtree, it will not run on F7+.

https://www.redhat.com/archives/fedora-devel-list/2006-December/msg00250.html
https://www.redhat.com/archives/fedora-devel-list/2006-December/msg00279.html

cElementTree.so module in python-elementtree has been
renamed to _elementtree.so module in python rpm
(F7 python is 2.5) and to use _elementtree, some code change
will be needed.

$ ./jbrout.py 
Traceback (most recent call last):
  File "./jbrout.py", line 67, in <module>
    from db import JBrout,Buffer
  File "/home/bjohnson/package-review/jbrout/jbrout/db.py", line 16, in <module>
    from lxml.etree import Element,ElementTree
ImportError: No module named lxml.etree
Comment 12 Matěj Cepl 2007-06-15 07:55:17 EDT
What's your version of python-lxml package? It works for me with 
python-lxml-1.1.2-1.fc7 (and yes, rpm -q --changelog says that version 1.0.3-3
is "Rebuild for new Python".
Comment 13 Bernard Johnson 2007-06-16 14:51:49 EDT
(In reply to comment #12)
> What's your version of python-lxml package? It works for me with 
> python-lxml-1.1.2-1.fc7 (and yes, rpm -q --changelog says that version 1.0.3-3
> is "Rebuild for new Python".

Sigh.  Bitten by bug #244077.

Ok, looking into the translation issues at the moment.
Comment 14 Bernard Johnson 2007-06-16 16:48:05 EDT
For the translation files, you should be able to:

1) get rid of the obsoleted translation in the .po files (msgattrib --no-obsoletes)
2) msgcat the .po files together to a jbrout.po
3) create the binary catalog from jbrout.po to jbrout.mo
4) install the jbrout.mo file in /usr/share/locale/fr/LC_MESSAGES
5) edit (patch) jbrout.py and jplugins.py to change the GladeApp.bindtextdomain
and createGetText calls to use /usr/share/locale rather than local directories.
Comment 15 Matěj Cepl 2007-07-03 18:53:09 EDT
I fixed the package so that it works on Fedora 7, but I got totally lost in
dealing with translation files. Are there any standard tools how to what you are
saying for the set of all languages supported by the package (I guess, I should
not limit myself to just one translation, right)?

The working SRPMS without any language stuff is
http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.182-2.fc7.src.rpm
and its SPEC is in the same location.

Messed up and unfinished attempt to fix SPEC file is on
http://www.ceplovi.cz/matej/progs/rpms/jbrout-working.spec
Do you have any idea how to fix it?
Comment 16 Matěj Cepl 2007-07-03 18:54:23 EDT
I guess the easiest way would be to write some Python script to do all that
stuff, but wasn't such script already written?
Comment 17 Bernard Johnson 2007-07-04 18:32:43 EDT
(In reply to comment #15)
> I fixed the package so that it works on Fedora 7, but I got totally lost in
> dealing with translation files. Are there any standard tools how to what you are
> saying for the set of all languages supported by the package (I guess, I should
> not limit myself to just one translation, right)?

(In reply to comment #16)
> I guess the easiest way would be to write some Python script to do all that
> stuff, but wasn't such script already written?

I would guess that the author of the program has a Makefile or something that
processes his translation files.  It might be worth pinging upstream so see if
he can include it if such a program exists.

I'm kinda schlepping my way through this too, so I don't think I can help anymore :(
Comment 18 Matěj Cepl 2007-09-17 11:55:18 EDT
Bad attempt (doesn't work well) of fixing gettext stuff (plus a patch against
screwed-by-Picassa IPTC metadata using libiptcdata) is available at

SRPM: http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.182-3.fc7.src.rpm
RPM:  http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.182-3.fc7.noarch.rpm
Spec: http://www.ceplovi.cz/matej/progs/rpms/jbrout.spec
Comment 19 Matěj Cepl 2007-09-18 01:42:13 EDT
The solution in the last package is nonsense -- *.{po,mo} files should be really
scattered in different directories, because that's the way the program and its
plugins are written (using explicit locale dirs). There's nothing illegal in
that, but we have to make %find_lang to work with this. Will ask on
#fedora-devel and fix this.
Comment 20 Matěj Cepl 2007-09-18 01:46:09 EDT
OK, I have finally understood your comment 14 point 5, but the question is
whether we should do this at all? Do we have to have one unified .mo file in
%{_datadir} or is the current scattered solution good enough? (something about
"Don't fix it when it works.")
Comment 21 Bernard Johnson 2007-10-18 17:08:02 EDT
(In reply to comment #20)
> OK, I have finally understood your comment 14 point 5, but the question is
> whether we should do this at all? Do we have to have one unified .mo file in
> %{_datadir} or is the current scattered solution good enough? (something about
> "Don't fix it when it works.")

IMHO, it needs to be done.  The developer docs also seem pretty clear on using
%find_lang to process the translations.  If you don't use %find_lang, any new
translations added to the package will not be automatically added.

If you wish, solicit opinions on the -devel mailing list and let's get some input.
Comment 22 Bernard Johnson 2008-06-07 12:38:06 EDT
I don't have time to properly review this package anymore.
Comment 23 Jason Tibbitts 2008-06-18 20:05:55 EDT
Unfortunately, while the above specfile link works, the link to the src.rpm is
404.  I grabbed
http://www.ceplovi.cz/matej/progs/rpms/jbrout-0.2.187-1.fc7.src.rpm instead; is
that one OK?  Unfortunately it doesn't build; desktop-file-install fails (sorry
for horrible wrapping):

+ desktop-file-install --dir
/var/tmp/jbrout-0.2.187-1.fc10-UhhP3w/usr/share/applications --vendor=fedora
--add-category=X-Fedora --delete-original
/var/tmp/jbrout-0.2.187-1.fc10-UhhP3w/usr/share/applications/jbrout.desktop
/var/tmp/jbrout-0.2.187-1.fc10-UhhP3w/usr/share/applications/fedora-jbrout.desktop:
error: value "0.2" for key "Version" in group "Desktop Entry" is not a known version
Error on file
"/var/tmp/jbrout-0.2.187-1.fc10-UhhP3w/usr/share/applications/jbrout.desktop":
Failed to validate the created desktop file
Comment 24 Matěj Cepl 2008-06-20 10:23:16 EDT
OK, there is new snapshot upstream, so I have created new .src.rpm
(http://mcepl.fedorapeople.org/rpms/jbrout-0.2.201-1.fc9.src.rpm), which builds
in koji http://koji.fedoraproject.org/koji/taskinfo?taskID=672885

I still don't know how to fix the problem with many .mo files, but otherwise
this package should be all right (rpmlint is otherwise silent on both src.rpm
and noarch.rpm).
Comment 25 Matěj Cepl 2008-06-20 10:25:28 EDT
URL for .spec file is still the same
http://mcepl.fedorapeople.org/rpms/jbrout.spec
Comment 26 Jason Tibbitts 2008-06-25 15:40:22 EDT
Seems there's no longer any reason for the svn checkout instructions since you
can directly fetch a tarball.

There's no real point in mentioning the License in the Summary:, is there?  Or
what language the package is written in?  These things really don't matter to
someone who is interested in what the package does.

Similarly for the %description; does any Fedora user particularly care whether
the software works on Windows 2000?  And who is "me" in the description?  Yum or
some package manager will display this to most users, and surely yum isn't going
to be conversing about itself.

rpmlint does indeed complain about the .mo files not being mentioned in %lang. 
You could list each of them separately in the %files list with %lang(foo) but at
most that would allow someone to save a little space by excluding some specific
lang files.  Nice to have, but not absolutely necessary in my opinion, although
it shouldn't be too terribly difficult if you wanted to do that.

You should probably report this issue with the desktop file upstream:
  key "Categories" is a list and does not have a semicolon as trailing 
  character, fixing
Also, there's no need to use "--vendor=fedora" when installing your desktop file.

Not sure if you noticed it, but there's no point to the %find_lang call, since
this package insatlls nothing into /usr/share/locale.  The %{name}.lang file is
empty.  You might as well just remove the %find_lang call and the -f bit from
%files.

* source files match upstream:
   b62b1bbd72400fd352deb8523a61e2b2da4f81c47f2118dd51488bee626fe77c  
   jbrout-0.2.201.sources.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X summary could use some work.
X description could use some work.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
   jbrout = 0.2.201-1.fc10
  =
   /bin/sh
   /usr/bin/env
   fbida
   jhead
   pygtk2 >= 2.6
   python >= 2.4
   python-imaging
   python-lxml

* %check is not present; no test suite upstream.  I installed and ran this 
   package; it seems to work OK.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
X desktop installed with --vendor=fedora.
Comment 27 Matěj Cepl 2008-06-26 12:27:14 EDT
Updated package is available at
http://mcepl.fedorapeople.org/rpms/jbrout-0.2.201-2.fc9.src.rpm
and SPEC file is still at
http://mcepl.fedorapeople.org/rpms/jbrout.spec
Comment 28 Jason Tibbitts 2008-06-26 19:25:00 EDT
Summary and description look good; desktop file installed correctly.  Looks fine
to me.

APPROVED

Only took 16 months!
Comment 29 Matěj Cepl 2008-06-27 08:00:37 EDT
GREAT!!! And thanks a lot for resolving for me the problem with %lang files.
Comment 30 Matěj Cepl 2008-06-27 12:17:03 EDT
New Package CVS Request
=======================
Package Name: jbrout
Short Description: Photo manager, written in pyGTK
Owners:mcepl
Branches: F-8 F-9
InitialCC:
Cvsextras Commits:yes
Comment 31 Kevin Fenzi 2008-06-27 12:49:48 EDT
cvs done.

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