Bug 207761 - Review Request: xpdf - A PDF file viewer for the X Window System
Review Request: xpdf - A PDF file viewer for the X Window System
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Package Reviews List
:
Depends On: 207802
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-09-22 17:46 EDT by Tom "spot" Callaway
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-27 13:33:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
use versionned source files in ftp (1.09 KB, patch)
2006-09-25 13:51 EDT, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Tom "spot" Callaway 2006-09-22 17:46:41 EDT
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.
Comment 1 Jesse Keating 2006-09-22 17:53:54 EDT
Taking this review, will have feedback later tonight.
Comment 2 Patrice Dumas 2006-09-23 05:36:46 EDT
* 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.
Comment 3 Tom "spot" Callaway 2006-09-23 10:12:58 EDT
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
Comment 4 Patrice Dumas 2006-09-23 16:10:48 EDT
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).
Comment 5 Patrice Dumas 2006-09-23 16:14:21 EDT
Those BR seems unneeded

BuildRequires: fileutils
BuildRequires: findutils

And there is a duplicate Requires: poppler-utils in the main package.
Comment 6 Tom "spot" Callaway 2006-09-23 21:42:47 EDT
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
Comment 7 Patrice Dumas 2006-09-24 04:32:19 EDT
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
Comment 8 Patrice Dumas 2006-09-24 05:07:28 EDT
Yet another, I think that

Requires(post): desktop-file-utils
Requires(postun): desktop-file-utils

should be replaced by

BuildRequires: desktop-file-utils
Comment 9 Tom "spot" Callaway 2006-09-24 23:09:57 EDT
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
Comment 10 Patrice Dumas 2006-09-25 02:49:23 EDT
(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."

Comment 11 Patrice Dumas 2006-09-25 03:15:47 EDT
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.
Comment 12 Tom "spot" Callaway 2006-09-25 11:14:52 EDT
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
Comment 13 Patrice Dumas 2006-09-25 11:37:01 EDT
(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.
Comment 14 Jesse Keating 2006-09-25 11:48:59 EDT
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.
Comment 15 Patrice Dumas 2006-09-25 13:50:04 EDT
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 ;-)
Comment 16 Patrice Dumas 2006-09-25 13:51:38 EDT
Created attachment 137074 [details]
use versionned source files in ftp
Comment 17 Tom "spot" Callaway 2006-09-25 14:04:43 EDT
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? :)
Comment 18 Patrice Dumas 2006-09-25 14:31:52 EDT
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...
Comment 19 Tom "spot" Callaway 2006-09-25 14:45:20 EDT
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
Comment 20 Patrice Dumas 2006-09-25 15:48:06 EDT
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.
Comment 21 Tom "spot" Callaway 2006-09-25 16:36:45 EDT
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
Comment 22 Tom "spot" Callaway 2006-09-25 16:46:24 EDT
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
Comment 23 Patrice Dumas 2006-09-25 17:49:14 EDT
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@cena.dgac.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.
Comment 24 Patrice Dumas 2006-09-25 18:12:03 EDT
(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.
Comment 25 Tom "spot" Callaway 2006-09-26 00:26:36 EDT
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
Comment 26 Ralf Corsepius 2006-09-26 03:26:20 EDT
(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.
Comment 27 Patrice Dumas 2006-09-26 03:38:14 EDT
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!
Comment 28 Tom "spot" Callaway 2006-09-27 13:33:42 EDT
Built for devel, thanks for the thorough review.

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