Bug 225235

I will review this package. 

Please do address the 203536 bug about splitting out a -devel package. 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
See Below - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
0c8e0c31b08c14f7a7198ce967eb3281  a2ps-4.13b.tar.gz
0c8e0c31b08c14f7a7198ce967eb3281  a2ps-4.13b.tar.gz.1
fee1456d0e6e94af4fc5b5a1bb9687b7  i18n-fonts-0.1.tar.gz
fee1456d0e6e94af4fc5b5a1bb9687b7  i18n-fonts-0.1.tar.gz
See below - Package needs ExcludeArch
OK - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
See below - .a/.la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version
 - check for outstanding bugs on package.

Issues:

1. You use RPM_BUILD_ROOT and %{buildroot}. Would be good to stick to one style?

2. Is there a bug filed for the
# Temp exclude on ppc64 as no emacs there right now
ExcludeArch: ppc64

3. Should fix the buildroot to the standard.

4. Should the .a .la files be shipped?
I suppose if there is a devel package, the .a might be usefull.

5. Our good friend rpmlint says:

E: a2ps-debuginfo tag-not-utf8 %changelog

Not sure where the non utf8 in the changelog is... do you see it?

E: a2ps-debuginfo script-without-shebang /usr/src/debug/a2ps-4.13/lib/basename.c
E: a2ps-debuginfo script-without-shebang /usr/src/debug/a2ps-4.13/lib/xmalloc.c

Permissions wrong on those source files?

W: a2ps summary-ended-with-dot Converts text and other types of files to
PostScript(TM).

Don't end summary with .

E: a2ps tag-not-utf8 %changelog
E: a2ps non-utf8-spec-file a2ps.spec

Ah, the entire spec seems to be non utf8...

W: a2ps prereq-use sed, coreutils


W: a2ps unversioned-explicit-obsoletes a2ps-i18n
W: a2ps unversioned-explicit-provides a2ps-i18n

Perhaps should have versions where that was obsoleted and provide the next version?
Of course that may have been so long ago that we can just remove these now.

W: a2ps macro-in-%changelog files

Thats in one of the very first changelogs from 1998:
- narrower range of %files splats.

W: a2ps mixed-use-of-spaces-and-tabs (spaces: line 169, tab: line 211)

Pick tabs or spaces for cleanness?

E: a2ps file-in-usr-marked-as-conffile /usr/share/a2ps/afm/fonts.map

This looks like it can be ignored.

W: a2ps devel-file-in-non-devel-package /usr/lib/liba2ps.a

Should be removed or moved to devel.

W: a2ps file-not-utf8 /usr/share/info/a2ps.info.gz

Need to run iconv on the info file before install?

W: a2ps devel-file-in-non-devel-package /usr/include/liba2ps.h

Should be removed or moved to devel.

W: a2ps dangerous-command-in-%post mv

Could the ./make_fonts_map.sh be modified to handle the moving the new maps file in
place logic?

6. Instead of 'exit 0' at the end of the scriptlets, perhaps add '|| :'
to the scriplets? Although it's not clear if thats cleaner.

7. You are missing:
Requires(post): /sbin/install-info
Requires(preun): /sbin/install-info

8. I assume upstream is dead and you can't get any patches pushed up?

9. 3 outstanding bugs, might look at that, especially the hebrew support 
and splitting -devel package.

(In reply to comment #2)

> E: a2ps file-in-usr-marked-as-conffile /usr/share/a2ps/afm/fonts.map
> 
> This looks like it can be ignored.

Why can it be ignored? This seems to me that this is a serious
blocker. /usr should be assumed to be read-only and there should
never be any config file in it.

In reply to comment #3: 

Well, It's unclear how hard it would be to move this map file (which is
generated on install). If it's possible to move I agree it would be good to do so. 

The upstream for this package appears to be very dead. :(

Is it really necessary to rerun the autotools?
The
cp /usr/share/aclocal/libtool.m4 m4/
also seems wrong to me.

Why not use %configure? 

%{?_smp_mflags} should be used (or a comment added).

You should preserve timestamps for font files with -p for install.

Why is there a Requires on info?


On devel a2ps doesn't seem to work for ps files:
$ a2ps /usr/share/doc/bash-3.2/bash.ps -o t.ps
[/usr/share/doc/bash-3.2/bash.ps (PostScript): 121 pages on 61 sheets]
[Total: 121 pages on 61 sheets] saved into the file `t.ps'

it seems to render them as ascii text.

Resetting flags and such per the new review guidelines:
https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html

In reply to comment #2:
> W: a2ps file-not-utf8 /usr/share/info/a2ps.info.gz
> Need to run iconv on the info file before install?

I think rpmlint is in error here.

8: yes, upstream is dead as far as I can tell.

In reply to comment #5:
> Is it really necessary to rerun the autotools?
> The
> cp /usr/share/aclocal/libtool.m4 m4/
> also seems wrong to me.

Unfortunately a2ps is *really* temperamental with autotools.  Yes, it is
necessary to re-run them in order to add knowledge about some architectures e.g.
x86_64.  If you can find any way to make this better in a2ps, please send me a
tested patch -- I spent at least two days trying to get it to *build* at all
last time I had to update it.

I've applied all the changes I've seen suggested, and tagged and built 4.13b-61.fc7.

(In reply to comment #7)

> Unfortunately a2ps is *really* temperamental with autotools.  Yes, it is
> necessary to re-run them in order to add knowledge about some architectures e.g.
> x86_64. 

I cannot test because I don't have an x86_64. But if the issue is with
calling internal libtool, I have an untested patch I will attach.

Created attachment 148925 [details]
very simple patch to call external libtool

I don't think it's as simple as that.  We have to run the autotools anyway to
properly incorporate patches that change the .in files (such as the hebrew
patch), and anyway we seem to end up with broken configure scripts if we do that
properly on anything more recent than FC-6.  Feel free to play around with this
yourself -- the brokenness can be seen on i386 as well.

I am not touching the a2ps auto* code any more unless there is a real actual
need (i.e. doesn't build).  Already too much time has been wasted on it, and in
my opinion we shouldn't be shipping a2ps any more anyway but making paps better.

(In reply to comment #10)
> I don't think it's as simple as that.  We have to run the autotools anyway to
> properly incorporate patches that change the .in files (such as the hebrew
> patch), 

This patch cleanly changes configure together with configure.in
and therefore shouldn't need a rerun of autotools. I have checked
all the patch they seem clean. The timestamps are messed up after
patching, but that may be corrected. I'll attach patches.

Created attachment 148967 [details]
simple patch to call external libtool updated

This is the same patch cleaned up.

Created attachment 148968 [details]
spec file patch to avoid running the autotools

In the patch I also removed the dependency on gperf, since
I couldn't find that package.

I have checked that gperf isn't needed when source file 
timestamps are not modified (as why I can't find gperf,
I guess there is something broken somewhere...)

ok. I think the items that I was seeing are all fixed in the latest version. 

rpmlint now says: 

E: a2ps file-in-usr-marked-as-conffile /usr/share/a2ps/afm/fonts.map
W: a2ps file-not-utf8 /usr/share/info/a2ps.info.gz
W: a2ps dangerous-command-in-%post mv

Did we determine anything about the fonts.map file? Is there any way to move
that or should we just ignore it?

As for the auto* patches, I haven't tested them... I do have a x86_64 box if you
would like to do some testing there Patrice? Or would you be willing to try
using them Tim?

In my opinion the autotools issue is a must fix, particularly
since I did a patch.

Regarding 
E: a2ps file-in-usr-marked-as-conffile /usr/share/a2ps/afm/fonts.map
it is clearly wrong in my opinion. There are many ways to override
the defaults. Moreover there is no reason to have that file as %config,
and not, say, encoding.map.

In my opinion what should be done would be to add, in
a2ps-4.13-etc.patch,  $(sysconfdir)/$(PACKAGE) on the
libpath line, in etc/Makefile.in and etc/Makefile.am.
and add %dir %{_sysconfdir}/a2ps to %files.

I can do a patch if desired (and after the auto* issue is solved)


Other issues:

*
gzip %{buildroot}%{_infodir}/* || :
  is unneeded

* %{_sysconfdir}/a2ps.cfg shouldn't be (noreplace) and I even think
that it shouldn't be %config at all.

* missing BuildRequires psutils, gv, tetex-dvips, makeinfo,
tetex-latex.
Missing Requires ImageMagick, texinfo-tex, gv, gzip,
  bzip2,  groff-perl, tetex-dvips, tetex-latex, tetex-fonts

Suggestions:
* for consistency use the info scriptlets from 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db
and also remove the .gz.

* use %defattr(-,root,root,-)

* use sed instead of perl for simple substitution

Another Requires seems to be missing, 
Requires: file

Another suggestion would be to use libpaper, with
--with-medium=libpaper unless _glibc is better.

In reply to comment #14: gperf is checked for during configure.

In reply to comment #17 (buildrequires/requires): for makeinfo I think you mean
texinfo, and I don't know what package gv is so I've left that out.

In reply to comment #19: is libpaper better?  I'd rather keep the behaviour the
same..

New package tagged as a2ps-4.13b-62.fc7.  Build is stuck at the moment.

(In reply to comment #20)
> In reply to comment #14: gperf is checked for during configure.

And? The missing system is used, because it is only needed if
lib/confg.gperf was modified. it is not needed, and not used,
lib/confg.c is newer than lib/confg.gperf.

> In reply to comment #17 (buildrequires/requires): for makeinfo I think you mean
> texinfo, 

indeed.

> and I don't know what package gv is so I've left that out.

gv is in package gv, it is a ps viewer. It is in extras. 

> In reply to comment #19: is libpaper better?  I'd rather keep the behaviour the
> same..

Ok. It is used by xpdf. I don't now if it is better.


The removal of libraries is a bit too much: you remove liba2ps which
is needed by a2ps!  
Maybe you could use
rm -f %{buildroot}%{_libdir}/*.{so,a,la}
and
%{_libdir}/liba2ps.so.*

Do you want a patch for the fonts.map solution I propose above?

Yes please.

Created attachment 149673 [details]
add sysconfdir/a2ps to the path searched for config and data files

In that patch I also add a patch for etc/Makefile.am that
matches the Makefile.in patch.

And also there is the change needed to distribute 
/usr/lib/liba2ps.so.1
/usr/lib/liba2ps.so.1.0.0

otherwise I couldn't test.

The following test files have a license incompatible from the
GPL. (they are below tests/). I found them by looking at the difference
between upstream and the debian source, found at
http://ftp.debian.org/debian/pool/main/a/a2ps/a2ps_4.13b.dfsg.1.orig.tar.gz

gps-ref: Converter.ps
gps-ref: fasttrig.ps
ps-ref: Converter.ps
ps-ref: fasttrig.ps
tstfiles: Converter.java
tstfiles: fasttrig.pas

Unfortunately this is a must fix. You can excerpt them as explained
here:
http://fedoraproject.org/wiki/Packaging/SourceURL

Or maybe you could use the debian source. I have never seen this
done so it is not obvious that it is right...

Thanks.  I've tagged and built 4.13b-63.fc7.

The delegation for html through mozilla seems broken to me.
Using html2ps like in debian would seem better to me. This
implies adding html2ps to fedora but it shouldn't be that hard.

Another issue is how the missing wdiff dependency is treated. 
Instead of being ignored, it should also be packaged in fedora.

That's a good idea. (But I'm not packaging those.)

I begun to have a look at the patches provided by other distros.
Mandriva patchset is a subset of other patchsets. Suse 
patches and debian patches are interesting though. Did you have
a look? 

I have checked that a simple utf-8 encoded (my locale is 
LANG=fr_FR.UTF-8) is displayed as if it was latin1. I have checked
a bit on the web and it seems like this cannot be easily fixed.
What's you thoughts on that subject?

Also did you contact people from the other distros to collaborate
on patches, since upstream is dead?

(In reply to comment #24)

> Unfortunately this is a must fix. You can excerpt them as explained
> here:
> http://fedoraproject.org/wiki/Packaging/SourceURL

Your fix seems right. As a suggestion, I would have called the 
script along 
a2ps-generate-tarball.sh
to avoid the name clashes in the SOURCES directory.

No, I did not look at other patchsets.

Yes, UTF-8 handling is broken.

No, I did not contact others to collaborate on patches.  I am much more
interested in getting additional features into paps than maintaining dead packages.

Tagged and built as 4.13b-64.fc7.

I am willing to look at other patchsets and contact other maintainers.
Would you accept that I comaintain a2ps with you after the merge?

regarding paps, is it really a substitute? It doesn't seems to use
delegations? It could be used by a2ps to render text, though.

Yes, some help would be greatly appreciated. :-)

No, paps does not use delegations, but for print output CUPS has its mime.convs
system.  I think it is a tiny number of people who actually need that
functionality for things like HTML and ROFF.

ok, So where do we stand on this package now?

Looking at the latest version I now see the following from rpmlint: 

W: a2ps file-not-utf8 /usr/share/info/a2ps.info.gz

Run iconv on it? 

W: a2ps non-conffile-in-etc /etc/a2ps.cfg

Shouldn't that be %config? Patrice, you suggested it shouldn't be config? why? 
It's in /etc and it's something that people might modify isn't it?

E: a2ps postin-without-ldconfig /usr/lib/liba2ps.so.1.0.0
E: a2ps library-without-ldconfig-postun /usr/lib/liba2ps.so.1.0.0

Missing postin/postun ldconfig? 

W: a2ps dangerous-command-in-%post mv
W: a2ps strange-permission a2ps-generate-tarball.sh 0755

These can be ignored I think. 

I don't see anything else in my orig items thats not been addressed. 
Patrice? Anything you see holding up approval of this package?

(In reply to comment #33)
> ok, So where do we stand on this package now?
> 
> Looking at the latest version I now see the following from rpmlint: 
> 
> W: a2ps file-not-utf8 /usr/share/info/a2ps.info.gz
> 
> Run iconv on it? 

I have checked, the problematic string is in doc/encoding.texi, it is:

  KOI-8 (+�) is a subset of ISO-IR-111 that can be used in Serbia, Belarus

maybe use iconv -f KOI-8?

> W: a2ps non-conffile-in-etc /etc/a2ps.cfg
> 
> Shouldn't that be %config? Patrice, you suggested it shouldn't be config? why? 
> It's in /etc and it's something that people might modify isn't it?

Normally it shouldn't be modified by the users
At the end there is this comment:
# To avoid that the next installation of a2ps destroys your
# definitions, local customization would be better done in
# a2ps-site.cfg.

Well, maybe it would better be %config. But in  my opinion 
%config(noreplace) isn't right.

> E: a2ps postin-without-ldconfig /usr/lib/liba2ps.so.1.0.0
> E: a2ps library-without-ldconfig-postun /usr/lib/liba2ps.so.1.0.0
> 
> Missing postin/postun ldconfig? 

Certainly, I forgot to readd them when I readed the libraries.

> W: a2ps dangerous-command-in-%post mv
> W: a2ps strange-permission a2ps-generate-tarball.sh 0755
> 
> These can be ignored I think. 
> 
> I don't see anything else in my orig items thats not been addressed. 
> Patrice? Anything you see holding up approval of this package?

No, except missing ldconfig.

Tagged and built a2ps-4.13b-65.fc7.

ok. I don't see any further blockers with this package, so it's APPROVED. 

Feel free to close this rawhide since once the version from comment #35 is
pushed out. 

Package Change Request
======================
Package Name: a2ps
Updated Fedora Owners: twaugh,pertusus

(in reference to comment #31)

cvs done.

Package Change Request
======================
Package Name: a2ps
New Branches: el6
Owners: tremble

Attempted contact with Fedora owner, no response:
https://bugzilla.redhat.com/show_bug.cgi?id=629589

Git done (by process-git-requests).