Bug 519282

Summary: Review Request: calibre - e-book converter and library manager
Product: [Fedora] Fedora Reporter: Ionuț Arțăriși <mapleoin>
Component: Package ReviewAssignee: José Matos <jamatos>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: bbaetz, david, fedora-package-review, herrold, ionut, jamatos, manuel.wolfshant, mapleoin, notting
Target Milestone: ---Flags: jamatos: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-11-04 11:15:08 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Ionuț Arțăriși 2009-08-25 21:46:51 UTC
Spec URL: http://mapleoin.fedorapeople.org/calibre/calibre.spec
SRPM URL: http://mapleoin.fedorapeople.org/calibre/calibre-0.6.8-1.fc11.src.rpm
Description: Calibre is meant to be a complete e-library solution. It includes library
management, format conversion, news feeds to ebook conversion as well as
e-book reader sync features.
.
Calibre is primarily a ebook cataloging program. It manages your ebook
collection for you. It is designed around the concept of the logical book,
i.e. a single entry in the database that may correspond to ebooks in several
formats. It also upports conversion from a dozen different ebook formats to
LRF and EPUB. A graphical interface to the conversion software can be
accessed easily by just clicking the "Convert E-books" button.
.
Supported input formats are: MOBI, LIT, PRC, EPUB, ODT, HTML, CBR, CBZ, RTF,
TXT, PDF and LRS.


--
Much of this has been inspired by the debian package. I've added some things and removed others. I don't know if I should give them credit or not (ie. what the fedora way of handling this is).

I'm not too sure how I should list the man pages in the %files section since that directory is under %{python_sitelib}/%{name} and it would be hard to pick every subdirectory of that and every file instead. I've searched for a way to get rid of the `file listed twice` error, but with no luck. 

I also don't know what to do about these errors: `calibre.i386: W: unstripped-binary-or-object`. Since those files are plugins, can I just ignore them?
Here's a full rpmlint output: http://fpaste.org/wMBy/


I have never had a package accepted into Fedora, though this isn't my first try. As such, I am looking for a sponsor.

Thank you!

Comment 1 Ionuț Arțăriși 2009-08-28 21:06:13 UTC
Spec URL: http://mapleoin.fedorapeople.org/calibre-0.8.10/calibre.spec
SRPM URL: http://mapleoin.fedorapeople.org/calibre-0.8.10/calibre-0.6.10-1.fc11.src.rpm

I fixed a few issues: the man page duplication, and man page installation, .desktop files, lrfviewer icon file and a new upstream release.

rpmlint output: http://fpaste.org/Z7qg/

I've gotten rid of anything but these two new warnings and all the old non-executable-script errors.
calibre.i586: W: non-conffile-in-etc /etc/bash_completion.d/calibre
calibre.i586: W: non-conffile-in-etc /etc/udev/rules.d/95-calibre.rules

Feedback welcome! ;)

Comment 2 José Matos 2009-09-03 21:20:58 UTC
Hi Ionuț,
  I am a sponsor and I am ready to start the sponsorship process. :-)

I'm sorry for taking so long, I saw this submission on Saturday, but I have been busy with real work.

I will postpone the review of this package until next Saturday, but meanwhile in order to access your understanding of the packaging guidelines I suggest you to start reviewing other packages as described in 

https://fedoraproject.org/wiki/PackageMaintainers
https://fedoraproject.org/wiki/PackageMaintainers/Join

You don't have yet the right to approve other packages, please say this explicitly in the reviews you choose. Don't worry I will follow those reviews and I will help if necessary.

Comment 3 Ionuț Arțăriși 2009-09-11 09:27:39 UTC
Hello José,

Thanks for volunteering to sponsor me!

I've started work on a lot of different packages lately and I've also tried to review some, with varying degrees of success. Here are some links to reviews where I could use your guidance:

telepathy-qt4 https://bugzilla.redhat.com/show_bug.cgi?id=520663
qdex          https://bugzilla.redhat.com/show_bug.cgi?id=522239
aspell-ro     https://bugzilla.redhat.com/show_bug.cgi?id=520204
tornado       https://bugzilla.redhat.com/show_bug.cgi?id=522613

django-robots Review: https://bugzilla.redhat.com/show_bug.cgi?id=518647
fossil Review(fail!)  https://bugzilla.redhat.com/show_bug.cgi?id=521730#c1

I have also updated the calibre package:
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.11-1.fc11.src.rpm
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec

Comment 4 David Nalley 2009-09-24 17:37:07 UTC
Your spec file has missing BuildRequires of:

python-lxml
python-dateutil

Comment 5 Michael Schwendt 2009-10-01 19:20:21 UTC
Directory %{python_sitelib}/%{name}/ is not included.

https://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership

Comment 6 José Matos 2009-10-09 19:00:02 UTC
You have proved to show and understanding of the packaging guidelines. That is good. :-)

Now for some small issues with this package:

1) please add the the directory %{python_sitelib}/%{name}/ to files as Michael pointed above. In this case it enough to replace

%{python_sitelib}/%{name}/*

with

%{python_sitelib}/%{name}

2) there is a missing BuildRequires python-mechanize in the spec file

3) I notice that you use lots of versioned (Build)Requires, are those real dependencies, I know that calibre requires python-2.6 but I am not sure about the others. Just asking here.

4) one small suggestion, when you declare the egg-info file I would suggest to use

%{python_sitelib}/%{name}-%{version}-*.egg-info

instead of

%{python_sitelib}/%{name}-%{version}-py2.6.egg-info

In this case this will work for python-2.7 without any need to change to spec file just for this detail.

For the moment this enough. :-)

Comment 7 José Matos 2009-10-09 19:38:38 UTC
FWIW instead of using the sitelib macro you should use the python_sitearch macro because that is where the installer places the calibre module.

Then it is enough to replace all the cases of sitelib with sitearch in the spec file.

The bash completion file is also installed but not declared in the files section.

I would suggest here instead of installing /usr/share/bash-completion/calibre to install that package in /usr/bash-completion.d/ and then to own that directory as other packages that use the same place do.

Comment 8 José Matos 2009-10-09 19:55:22 UTC
Please ignore my last remark about changing the bash completion place. I see now how and why it works.

Comment 9 Ionuț Arțăriși 2009-10-11 18:45:08 UTC
Thank you, Jose! and Michael and David.

Calibre seems to be a very agile project and I had to change most of the
specfile because the installation process changed a lot in recent versions. There are a lot of patches and things I did in the specfile that I'll have to tell upstream about, but I'd like some feedback from here first.

http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec.1
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.17-1.fc11.src.rpm

Comment 10 manuel wolfshant 2009-10-12 14:15:10 UTC
According to mock's build log, Fedora's compile options are not respected, despite the "CFLAGS=%{_optflags}" used in the %build section:
 
 ####### Building extension pdfreflow #######
g++ -O3 -Wall -DNDEBUG -fPIC -fno-strict-aliasing -pipe -pthread -I/usr/include/python2.6 -DPNG_SKIP_SETJMP_CHECK -I/usr/include -I/usr/include/ImageMagick -I/usr/include/poppler -c /builddir/build/BUILD/calibre/src/calibre/ebooks/pdf/reflow.cpp -o /builddir/build/BUILD/calibre/build/objects/pdfreflow/reflow.o
[...] (similar lines repeated)

and:

####### Building extension lzx #######
gcc -O3 -Wall -DNDEBUG -fPIC -fno-strict-aliasing -pipe -pthread -I/usr/include/python2.6 -I/builddir/build/BUILD/calibre/src/calibre/utils/lzx -c /b
uilddir/build/BUILD/calibre/src/calibre/utils/lzx/compressor.c -o /builddir/build/BUILD/calibre/build/objects/lzx/compressor.o

 and so on until :
####### Building extension pictureflow #######

which seems to respect our optimization flags. Unfortunately we can find afterwards:
   rm -f libpictureflow.so.1.0.0 libpictureflow.so libpictureflow.so.1 libpictureflow.so.1.0
   g++ -Wl,-O1 -shared -Wl,-soname,libpictureflow.so.1 -o libpictureflow.so.1.0.0 pictureflow.o moc_pictureflow.o -lQtGui -lQtCore -lpthread
which once again is wrong.

Could you please check and fix this ?

Comment 11 manuel wolfshant 2009-10-12 14:29:36 UTC
 [wolfy@wolfy tmp]$ rpmlint calibre-0.6.17-1.fc12.x86_64.rpm
gives the following warning:
 calibre.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/calibre

Is this really intended ?

Comment 12 José Matos 2009-10-12 17:42:12 UTC
(In reply to comment #11)
>  [wolfy@wolfy tmp]$ rpmlint calibre-0.6.17-1.fc12.x86_64.rpm
> gives the following warning:
>  calibre.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/calibre
> 
> Is this really intended ?  

Yes but that is a issue with the bash_completion package and until the time where that package stores it plugins in other location it is the right thing to do. Note that calibre should own that directory for the case where bash_completion is not installed.

Comment 13 José Matos 2009-10-12 17:43:51 UTC
(In reply to comment #9)
> Thank you, Jose! and Michael and David.
> 
> Calibre seems to be a very agile project and I had to change most of the
> specfile because the installation process changed a lot in recent versions.
> There are a lot of patches and things I did in the specfile that I'll have to
> tell upstream about, but I'd like some feedback from here first.
> 
> http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec.1
> http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.17-1.fc11.src.rpm  

On a shallow review note that most python BuildRequires should also probably be Requires in addition.

Comment 14 manuel wolfshant 2009-10-12 19:14:59 UTC
(In reply to comment #12)
> (In reply to comment #11)
> >  [wolfy@wolfy tmp]$ rpmlint calibre-0.6.17-1.fc12.x86_64.rpm
> > gives the following warning:
> >  calibre.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/calibre
> > 
> > Is this really intended ?  
> 
> Yes but that is a issue with the bash_completion package and until the time
> where that package stores it plugins in other location it is the right thing to
> do. Note that calibre should own that directory for the case where
> bash_completion is not installed.  

I was speaking about the fact that the file is not marked as config. Its mere presence is just fine.

Comment 15 Ionuț Arțăriși 2009-10-14 18:21:17 UTC
Thanks a lot for your comments, Manuel and Jose!

I marked the bash_completion directory as config(noreplace).

For now I've moved the discussion about CFLAGS upstream: http://calibre.kovidgoyal.net/ticket/3770

Things seem to be going well. I'm trying to help fix most of the other issues as well and will hopefully upload a much shorter specfile once that discussion is over.

Comment 16 Bradley 2009-10-19 12:47:22 UTC
Thanks for doing this - I was considering it before I saw that you were working on it.

A couple of comments/bugs, though:

 * It doesn't work when built for x86_64 - /usr/bin/calibre adds /usr/lib/calibre to the libpath, but that should be lib64. Same for the plugin path.

 * I had problems building an unpatched calibre from upstream, because it wouldn't compile. You've added calibre-outputdev.patch to work around this, but I'm not sure that that is correct. calibre is documented as requiring poppler-0.12, which isn't in F11, and that function's signature has changed. It compiles, but it looks like it'll just recurse infinitely - rather than call the parent class's method it calls itself. I haven't worked out how to trigger that code, though.

Comment 17 Bradley 2009-10-19 13:17:13 UTC
Also, it puts the udev rules into /lib64/udev/rules.d but everyone else uses /lib/udev/rules.d, even on 64-bit. It also owns the rules.d directory, but I don't think that it should - udev can presumably be assumed to be present (I'm not a package maintainer, though, but no other package that adds udev rules owns the rules.d directory)

/usr/share/calibre/fonts/liberation should presumably be using the system liberation fonts instead of its own copy? What about /usr/share/calibre/fonts/prs500 ?

Is /usr/lib64/calibre/calibre/trac needed?

The description has a typo - s/It also upports/It also supports/. Plus its not accurate - it supports writing and reading from mostly the same set of formats.

And finally, it still has its own copy of python-genshi

Thanks a lot for doing this!

Comment 18 Ionuț Arțăriși 2009-10-21 11:16:37 UTC
Thanks a lot Bradley.

Sorry for taking so much time to come up with this new version. There were a lot of changes to be made and I also had to clear some issues with upstream.

All of the mentioned issues should be fixed. 

There is one more remaining. The package provides what seem to be three unfree fonts (Swis721 BT, Dutch801 BT and Courier10 BT). The spec deletes them, but I'm not sure if we are allowed to distribute them in the src.rpm. 
Also, I think I'd need to symlink to some equivalent system fonts, but rpmlint complains of:

calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0003m_.ttf /usr/share/fonts/liberation/LiberationSans-Regular.ttf

http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.19-1.fc11.src.rpm

Comment 19 Bradley 2009-10-22 00:53:20 UTC
This now doesn't build on F11 - you now require poppler-qt-devel > 0.12 but that's not available. I guess that means that this will be F12 only? (and shouldn't that be >= rather than >, although it'll still work since 0.12.0 > 0.12)? You may need a similar explicit requires: line if the soname is the same as the earlier versions

Also, looking at the spec file, you removed the internal python-genshi, but there's not requires (and buildrequires?) for it. libwmf also isn't required (only buildreq'd) - is that correct?

And finally, shouldn't:

%{__chmod} -x src/calibre/*/*/*/*py
%{__chmod} -x src/calibre/*/*/*py
%{__chmod} -x src/calibre/*/*py
%{__chmod} -x src/calibre/*py

(and the sed lines above it use *.py rather than *py?

(And again, I'm nt a fedora packager, #include <usual-disclaimers>, etc etc)

Comment 20 Ionuț Arțăriși 2009-10-22 08:03:20 UTC
Indeed it looks like calibre won't be included in F11, as it doesn't build without poppler-qt-devel >= 0.12. That's what was causing the build failures we both ran into before.

Added python-genshi requires. 
The package only seems to need files which are provided by libwmf-lite and that's a dependency for ImageMagick, so I removed all libwmf requires now.

Those chmods and seds are equivalent with or without the dot since there isn't anything in those directories that ends with py and isn't a python file, but I modified them just for readability's sake.

http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.19-2.fc12.src.rpm

Thanks for your feedback!

Comment 21 Ionuț Arțăriși 2009-10-22 19:21:03 UTC
I talked to upstream about the redistribution of the 3 unfree fonts and it seems that he doesn't care[0]. So I followed [1] and removed the fonts from the source tarball. I then replaced them with symlinks to their liberation equivalents.

I get 3 warnings from rpmlint about dangling symlinks now, but they should be ok since they no longer dangle after installation :).

http://mapleoin.fedorapeople.org/pkgs/calibre/calibre.spec
http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.19-3.fc11.src.rpm

[0] http://calibre.kovidgoyal.net/ticket/3832
[1] https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

Comment 22 José Matos 2009-10-26 19:26:23 UTC
Your source rpm above carries the build files. Note the calibre/build and calibre/~/rpmbuild/ directories in the source rpm.

Could you please generate a new source rpm with the source clean?

The script is to regenerate the source file is OK, I have used it and that was I were I noticed the difference.

This is the last issue that I have with this package. You have diligently solved every other issue in a remarkable way. :-)

Comment 23 Ionuț Arțăriși 2009-10-27 06:36:10 UTC
Thank you, Jose!

Here's the new file http://mapleoin.fedorapeople.org/pkgs/calibre/calibre-0.6.19-3.fc11.src.rpm

I don't know how that got in there. My ~/SOURCES/calibre seems to have been unclean.

Comment 24 José Matos 2009-10-28 18:25:10 UTC
Good:

- rpmlint checks returns

calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0419m_.ttf /usr/share/fonts/liberation/LiberationMono-Regular.ttf
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/pictureflow.so 0775
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/podofo.so 0775
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/pdfreflow.so 0775
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/fontconfig.so 0775
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/lzx.so 0775
calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0003m_.ttf /usr/share/fonts/liberation/LiberationSans-Regular.ttf
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/cPalmdoc.so 0775
calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0011m_.ttf /usr/share/fonts/liberation/LiberationSerif-Regular.ttf
calibre.x86_64: E: non-standard-executable-perm /usr/lib64/calibre/calibre/plugins/msdes.so 0775

The dangling links are commented in the spec and there is nothing else to add. :-)

The problem with the non-standard-executable-perm is the following:

$ rpm -qp --provides calibre-0.6.19-3.fc12.x86_64.rpm
cPalmdoc.so()(64bit)
config(calibre) = 0.6.19-3.fc12
fontconfig.so()(64bit)
lzx.so()(64bit)
mimehandler(application/epub+zip)
mimehandler(application/x-sony-bbeb)
msdes.so()(64bit)
pdfreflow.so()(64bit)
pictureflow.so()(64bit)
podofo.so()(64bit)
calibre = 0.6.19-3.fc12
calibre(x86-64) = 0.6.19-3.fc12

Not that all the so's that show up above also appear below, that is the problem. This should be fixed.

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv3) OK, text in %doc, matches source
- spec file legible, in American English
- source matches upstream after the removal of the problematic parts as explained in the spec file.
sha256sum 5f6366327de00adbf2b8bb2e1c4008b8d2620a8a85089aa4930a2a22511a3778

- package compiles and builds on devel (x86_64)
- locales are correctly handled
- owns all directories that it creates
- .desktop files are correctly set
- no missing BR
- no unnecessary BR
- not relocatable
- no duplicate files
- permissions ok (other than the above problem)
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime

With the non-standard-executable-perm issue fixed this package is APPROVED.

There is no need to post the correction here I trust that you will do it on import. Meanwhile as discussed above I will sponsor you. Well done, nice job with this "enfant terrible". :-)

Comment 25 José Matos 2009-10-28 18:27:40 UTC
FWIW your next steps are described in https://fedoraproject.org/wiki/Package_Review_Process

Comment 26 Ionuț Arțăriși 2009-10-29 11:08:32 UTC
Jose, I can't replicate the rpmlint errors you're getting. Here's a koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=1776159


$ rpmlint calibre-0.6.19-3.fc12.x86_64.rpm 
calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0419m_.ttf /usr/share/fonts/liberation/LiberationMono-Regular.ttf
calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0003m_.ttf /usr/share/fonts/liberation/LiberationSans-Regular.ttf
calibre.x86_64: W: dangling-symlink /usr/share/calibre/fonts/prs500/tt0011m_.ttf /usr/share/fonts/liberation/LiberationSerif-Regular.ttf
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 27 José Matos 2009-10-29 15:21:30 UTC
I run mock locally:

$ mock --rebuild -r fedora-devel-x86_64 calibre-0.6.19-3.fc11.src.rpm

Probably this is related with different set of build tools used in the koji building system...

If that is not a problem then proceed.

Comment 28 Ionuț Arțăriși 2009-10-30 08:41:30 UTC
New Package CVS Request
=======================
Package Name: calibre
Short Description: E-book converter and library manager
Owners: mapleoin
Branches: F-12
InitialCC:

Comment 29 Kevin Fenzi 2009-10-31 23:51:41 UTC
cvs done.