Bug 519282
Summary: | Review Request: calibre - e-book converter and library manager | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ionuț Arțăriși <mapleoin> |
Component: | Package Review | Assignee: | José Matos <jamatos> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | 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-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! ;) 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. 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 Your spec file has missing BuildRequires of: python-lxml python-dateutil Directory %{python_sitelib}/%{name}/ is not included. https://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership 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. :-) 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. Please ignore my last remark about changing the bash completion place. I see now how and why it works. 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 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 ? [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 ? (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. (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. (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. 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. 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. 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! 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 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) 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! 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 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. :-) 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. 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". :-) FWIW your next steps are described in https://fedoraproject.org/wiki/Package_Review_Process 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. 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. New Package CVS Request ======================= Package Name: calibre Short Description: E-book converter and library manager Owners: mapleoin Branches: F-12 InitialCC: cvs done. |