Spec URL: http://www.auroralinux.org/people/spot/review/xpdf.spec SRPM URL: http://www.auroralinux.org/people/spot/review/xpdf-3.01-17.fc6.src.rpm Description: Xpdf is an X Window System based viewer for Portable Document Format (PDF) files. Xpdf is a small and efficient program which uses standard X fonts. This package is moving from Fedora Core to Fedora Extras.
Taking this review, will have feedback later tonight.
* I haven't tested, but it seems to me that the configure.in patch and autoconf call could be replaced by --without-Xp-library * Why isn't t1lib used? * There are some rpmlint errors/warning, most of them should be sorted out easily: E: xpdf tag-not-utf8 %changelog E: xpdf non-utf8-spec-file xpdf.spec W: xpdf mixed-use-of-spaces-and-tabs E: xpdf tag-not-utf8 %changelog E: xpdf obsolete-not-provided xpdf-chinese-simplified E: xpdf obsolete-not-provided xpdf-chinese-traditional E: xpdf obsolete-not-provided xpdf-korean E: xpdf obsolete-not-provided xpdf-japanese W: xpdf-utils summary-ended-with-dot Command line utilities for converting PDF files. E: xpdf-utils tag-not-utf8 %changelog E: xpdf-debuginfo tag-not-utf8 %changelog * It seems to me that the .png icon should better be in /usr/share/icons/hicolor/48x48/apps/ with the appropriate calls to the gtk cache snippet. * The calls to update-desktop-database are missing although there is a mimetype entry. * The desktop-file-install vendor should be fedora. * It seems to me that it should be xpdf-utils that requires poppler-utils.
Indeed, every one of these points is valid, and I have adjusted the package accordingly. rpmlint is now clean. I even noticed that it was looking for libpaper, something not in Fedora, so I packaged that up and have it up for review at 207802. (It's a really easy review, should be no trouble to knock out). New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-18.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
More issues: * $RPM_BUILD_ROOT/etc/X11/applnk/Graphics seems to be unneeded * removing the files should better be without -r and, in my opinion even without f. * /etc/xpdfrc in %files should be %{_sysconfdir}. And it should also be nice to have a sed on all the files mentionning /etc/xpdfrc substituted by the patches. Something along for file in doc/*.1 doc/*.5 xpdf-*/README; do sed -i -e 's:/etc/xpdfrc:%{_sysconfdir}/xpdfrc:g' done * similarly /usr/share in the relevant files should be changed to %{_datadir}. Something along for file in xpdf-*/README xpdf-*/add-to-xpdfrc; do sed -i -e 's:/usr/share/:%{_datadir}/:g' done * All the README files that are in /usr/share/xpdf/LANG/README should be in %doc, and I propose to have for them the name README.LANG for each LANG. * The add-to-xpdfrc files should certainly be marked as %config. Also it is not obvious that these files are rightly in /usr/share/xpdf/LANG/. In my opinion they should better be in /etc. What I would suggest would be to add the directory %{_sysconfdir}/xpdf/ and put the add-to-xpdfrc within subdirectories one for each language, or, if you prefer otherwise, something similar. xpdf-3.01-redhat.patch should then be updated (the part corresponding with xpdf-3.00/doc/sample-xpdfrc).
Those BR seems unneeded BuildRequires: fileutils BuildRequires: findutils And there is a duplicate Requires: poppler-utils in the main package.
You sir, are a machine. All items updated in -19. New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-19.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
It is almost right from my point of view, but I still have some comments... * Using the acroread png for xpdf seems quite wrong to me, it may even be a trademark violation (but I don't know that subject a lot). I found an icon which should be much more suitable: http://en.wikipedia.org/wiki/Image:Xpdf-icon.PNG the acroread.png could, however be used for pdf files in my opinion. However I guess such icons are allready shipped with fedora. * Maybe the config files for the different languages in /etc/xpdf should have a %lang() in %files
Yet another, I think that Requires(post): desktop-file-utils Requires(postun): desktop-file-utils should be replaced by BuildRequires: desktop-file-utils
I disagree on two items: - the lang on the config files, the config files are actually in english. - While the BuildRequires: desktop-file-utils is correct, the Requires(post) and (postun) are also correct due to the scriptlet in %post and %postun (update-desktop-database is called). The icon is a definite mustfix, and I've corrected it in -20. New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-20.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
(In reply to comment #9) > I disagree on two items: > > - the lang on the config files, the config files are actually in english. Indeed, but the %lang, if I'm not wrong tells which files are interesting for which language, not the language of the file. Anyway putting %lang here would certainly unnecessarily clutter the spec, so do what you prefer. In fact proposed that because they were previously in such a place that they had a %lang, not because I care a lot about this kind of stuff. > - While the BuildRequires: desktop-file-utils is correct, the Requires(post) and > (postun) are also correct due to the scriptlet in %post and %postun > (update-desktop-database is called). This has changed in the guidelines, to avoid Requires bloat, quoting (you? ;-): "Note: For FC5+, this scriptlet follows the same convention as mimeinfo files and gtk-icon-cache. Namely, the spec file should not Require desktop-file-utils for this."
I have spotted other minor issues (will there be an end? ;-): * there are some cp -rf in my opinion it should better be cp -pr * in xpdf.desktop, there is an hardcoded category X-Red-Hat-Base which certainly shouldn't be there. Last a comment: the Group seems very strange to me for a viewer, but it seems to be the tradition (I do that too), and indeed there is no alternative that looks better. Besides Group isn't important.
OK. Group is a throwaway, it matches Evince and there is nothing better to set it to. Everything else is fixed in -21. New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-21.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
(In reply to comment #12) > OK. Group is a throwaway, it matches Evince and there is nothing better to set > it to. Indeed. I also use that group for example for xchm which is even more wrong in my opinion. Everything seems fine to me now, but I am not the reviewer ;-). I had a look at the gcc warnings, most of them seems not worrying, however I spotted some warnings like: warning: 'tpgrCXPtr1$x' may be used uninitialized in this function certainly not a blocker for inclusion in extras, but maybe something to be reported upstream, if somebody feels like it.
Patrice, if you're happy with the package, feel free to take over the review and approve it. I ran into some time issues and couldn't get to it this weekend.
The sources are not right for the lang packages. It seems that these are not the latest versions that are used. In my opinion it would be much better to have the real files and not the ftp links that points to the versionned files as source files. This would have the added benefit to have different files with different names in the look-aside cache. I made a patch to test the rpm build with the language files really present, and it seems that the patches have to be updated... I'll attach a quick spec file diff with the real source file names, but you'll have to update the patches ;-)
Created attachment 137074 [details] use versionned source files in ftp
Fixed in -22: New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-22.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec Anything else? :)
You have super power to do patches quicker than light... You didn't repatch the xpdf-LANG/README files, though... It isn't a blocker (though it should be better). Moving to the issue of licencing I found that the LANG specific packages are under licences that are not GPL compatible (distribution without modification). Since it is font information (data), it seems to me that it doesn't contradict fedora goals, however it is dubious from a legal point of view to distribute the font information together with GPL code. It isn't obvious since it is code and font information. If the GPL part and distributable parts have to be split, it won't be obvious to do it since a simple split (for example all the xpdf-LANG*.tar.gz in a single package) wouldn't be right: in the xpdf-LANG.tar.gz the encoding files are covered by the GPL, only the CMap/* files are covered by the GPL-incompatible licence...
Having the Free but GPL-incompatible CMap files (which are not compiled) shipped alongside the GPL application (xpdf) is not a problem, since they are not linked together. The missing README fixes are now in the new patch. Here comes -23: New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-23.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
Linking together is not the only reason why GPL and GPL-incompatible soft cannot cooperate. They shouldn't be in the same 'container'. Quoting the GPL: b) You must cause any work that you distribute or publish, that in whole or in part contains or is derived from the Program or any part thereof, to be licensed as a whole at no charge to all third parties under the terms of this License. So, for example it could be argued that xpdf-japanese-2004-jul-27.tar.gz contains the GPL-licenced encoding files, and therefore cause the files in CMap, distributed alongside to be licenced under the GPL which is not possible. It is also explained here: If identifiable sections of that work are not derived from the Program, and can be reasonably considered independent and separate works in themselves, then this License, and its terms, do not apply to those sections when you distribute them as separate works. But when you distribute the same sections as part of a whole which is a work based on the Program, the distribution of the whole must be on the terms of this License, whose permissions for other licensees extend to the entire whole, and thus to each and every part regardless of who wrote it. The first sentence, in my opinion describes what are the CMap files, "independent and separate works in themselves", so the GPL don't apply to them. However, as explained in the second sentence, "when you distribute the same sections as part of a whole which is a work based on the Program", which is the case for CMap files bundled together with encoding files covered by the GPL, "the distribution of the whole must be on the terms of this License, whose permissions for other licensees extend to the entire whole, and thus to each and every part regardless of who wrote it." Since the CMap cannot be distributed under the GPL, we have somewhere a licence violation.
OK. CMap files are gone from source tarballs, patched out of add-to-xpdfrc files. Also noticed that the thai and cyrillic files weren't actually being packaged (thankfully, they don't have CMap files). New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-24.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
Whoops. Forgot to patch the pathing for thai/cyrillic. Fixed now. New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-25.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
It is right now. I looked at all the files to find other licence issues. in goo/, some vms_* files have no licence and the copyright: * vms_directory.c: Patrick L. Mahan * vms_unix_times.c: ? * vms_unlink.c: Thanks to Patrick Moreau (pmoreau.fr). * vms_dirent.h: @(#)dirent.h 1.7 89/06/25 SMI * vms_sys_dirent.h: @(#)dirent.h 1.4 89/06/16 SMI The ones that seem problematic are goo/vms_directory.c and goo/vms_*dirent.h, since they have a copyright and no licence. With a licence, one have: * vms_unix_time.h: 1982, 1986 Berkeley software License Agreement It seems also problematic to me since I believe at that time it was the BSD incompatible with the GPL. The file in splash don't have any copyright nor licence. That is strange, but they can certainly be considered public domain, so no problem here. The goo/vms_* files aren't used at all. I fear the problematic files goo/vms_directory.c and goo/vms_*dirent.h should be removed. And the upstream should certainly be contacted for those issues (and the CMap issues), although there is a comment in the README, about the GPL which seems very strange.
(In reply to comment #23) > The goo/vms_* files aren't used at all. I fear the problematic files > goo/vms_directory.c and goo/vms_*dirent.h should be removed. And goo/vms_unix_time.h also should certainly be removed.
Tarball edited to remove goo/vms_* New SRPM: http://www.auroralinux.org/people/spot/review/xpdf-3.01-26.fc6.src.rpm New SPEC: http://www.auroralinux.org/people/spot/review/xpdf.spec
(In reply to comment #23) > It is right now. I looked at all the files to find other > licence issues. > > * vms_unix_time.h: 1982, 1986 Berkeley software License Agreement > It seems also problematic to me since I believe at that time > it was the BSD incompatible with the GPL. Is this the ORIGINAL BSD license containing the ad-clause? Then, this is a non-issue, because the decan of UCB official announced not to legally enforce and to abandon this offending clause many years ago. Many BSD-derived works (comprising FreeBSD and NetBSD) have been using the decan's statement as justification to remove this clause from their sources.
Thanks, Ralf for the explanation. In fact in the xpdf source there is no licence file, only a BSD copyright, so the clause cannot be removed from the licence, but we can consider that it is not an issue. * rpmlint is silent * package rightly named * after removal of files with licence which seems GPL incompatible the package is covered by the GPL, included * spec legible * upstream source match - the following files match the upstrem source: 7b22f31289ce0812d2ec77014e7b0cdf xpdf-cyrillic-2003-jun-28.tar.gz 96e058c1b0429ae1ba0b50f1784b0985 xpdf-thai-2002-jan-16.tar.gz - the following tarballs were checked manually using a diff against upsteam source to check that only files have been removed: e53ec72546bb1a010fc2a2730f6d80f5 xpdf-3.01-novms.tar.gz ba4b037ab691f8b029ec2b9820a2fb8c xpdf-chinese-simplified-2004-jul-27-NOCMAP.tar.gz 697e7edc09a285115b597ab03f2eddf9 xpdf-chinese-traditional-2004-jul-27-NOCMAP.tar.gz f759b1b9624c7364e5d5a1ab3d146597 xpdf-japanese-2004-jul-27-NOCMAP.tar.gz 276624cddd1b70c29a3ae03ddb20fb3a xpdf-korean-2005-jul-07-NOCMAP.tar.gz * compiles and run in devel * BuildRequires look fine * no library * no translations * directory owning is right * %files section is right * docs don't affect runtime * desktop file correctly packaged It's incredible, but it is APPROVED!
Built for devel, thanks for the thorough review.