I don't understand exactly why the __spec_install_post is redefined.
It seems to me that brp-compress leaves dir as is. I have tried
to remove the %define and things seems to be right.

Prereq should be Requires(.). At least I think so, although
this is not obvious with trigger/Requires(.) interactions.

The manual gzip are bad. Of course they could be there for the
comment above, that is avoiding gziping the dir file, but couldn't
it be done more cleanly? 

Why are the xsl and dtd files removed?

in the install-info scriptlets, install-info should figure out whether
.gz is needed or not.

Having the dir file %config is not very nice, given that it is not
in sysconfdir. However I can't see any other way.


I suggest using %defattr(-,root,root,-) instead of %defattr(-,root,root)


There is a build dependency loop texinfo -> ncurses-devel -> gpm-devel ->
install-info.

Created attachment 291429 [details]
fixes for the spec file

I have set the license to GPLv3+ in the spec file diff since
it seems to me that everything is under this license.

Also why not install the texinfo.tex and associated files
from texinfo? They should always be more up to date than
what comes from texlive.

rpmlint warning that could be fixed
texinfo.src:36: W: prereq-use bash
texinfo.src: W: mixed-use-of-spaces-and-tabs (spaces: line 90, tab: line 99)
texinfo.i386: W: file-not-utf8 /usr/share/doc/texinfo-4.11/ChangeLog

ignorable:
info.i386: E: info-dir-file /usr/share/info/dir
info.i386: E: file-in-usr-marked-as-conffile /usr/share/info/dir

I was typing up a review but didn't get to finish before the we had to leave the
hackfest.  I have it saved, but if Patrice would like to review this then I'm
more than happy to step aside and work on something else.

Tibbs, when I saw that you assigned yourself to the review, since I 
am interested in texinfo and I guess that you have already plenty 
other things to do I decided to take the time to do as much as possible 
of the review, such that you can do other reviews if you want to. Still 
if you have other comments, feel free to share...

(In reply to comment #1)
> I don't understand exactly why the __spec_install_post is redefined.
> It seems to me that brp-compress leaves dir as is. I have tried
> to remove the %define and things seems to be right.

Agree. I don't know exact purpose of it too (with the exception of what's
written in #16120), but I have got same result as you. I think we can remove it.

> 
> Prereq should be Requires(.). At least I think so, although
> this is not obvious with trigger/Requires(.) interactions.

I know PreReq is deprecated. IIRC Requires is practically PreReq without strict
package order and since there is only one package in PreReq, Requires should do
the same thing. Am I right?

> 
> The manual gzip are bad. Of course they could be there for the
> comment above, that is avoiding gziping the dir file, but couldn't
> it be done more cleanly?

OK.

> 
> Why are the xsl and dtd files removed?

I don't know. There's no reference in Changelog.

> 
> in the install-info scriptlets, install-info should figure out whether
> .gz is needed or not.

OK.

> 
> Having the dir file %config is not very nice, given that it is not
> in sysconfdir. However I can't see any other way.

Agree... with both sentences:)

> 
> I suggest using %defattr(-,root,root,-) instead of %defattr(-,root,root)

OK.

> 
> 
> There is a build dependency loop texinfo -> ncurses-devel -> gpm-devel ->
> install-info.
> 

How do you suggest to change it?

(In reply to comment #4)
> rpmlint warning that could be fixed
> texinfo.src:36: W: prereq-use bash
> texinfo.src: W: mixed-use-of-spaces-and-tabs (spaces: line 90, tab: line 99)
> texinfo.i386: W: file-not-utf8 /usr/share/doc/texinfo-4.11/ChangeLog
> 
> ignorable:
> info.i386: E: info-dir-file /usr/share/info/dir
> info.i386: E: file-in-usr-marked-as-conffile /usr/share/info/dir
> 

OK.

(In reply to comment #3)
> I have set the license to GPLv3+ in the spec file diff since
> it seems to me that everything is under this license.

OK.

> 
> Also why not install the texinfo.tex and associated files
> from texinfo? They should always be more up to date than
> what comes from texlive.

If it will be good for users, I'm game.

Created attachment 292489 [details]
new spec file diff

(In reply to comment #7)

> I know PreReq is deprecated. IIRC Requires is practically PreReq without strict
> package order and since there is only one package in PreReq, Requires should do
> the same thing. Am I right?

I think that you are right in your recalling, but I don't think that
a Requires and a PreReq do the same in a transaction where more than
one package is involved. If one of the packages has a
Requires(post): /sbin/install-info
Then it may be possible that info is installed before bash, thus leading
to info post-scriptlets failing. So it seems that at least there should
be a
Requires(post): bash
But I am not sure that it will work with triggers. It seemsso,
however, since on
http://people.redhat.com/laroche/pyrpm/pyrpm-devel.html
this is explained, and it looks like post is done before the
triggers and so a Requires(post) is enough.

Additionnally, it looks like rpm adds automatically a /bin/sh
Requires(post), as can be seen in the rpmbuild output.
Requires(post): /bin/sh
So nothing should be needed.

I'll ask on the packaging list to be sure.


> > There is a build dependency loop texinfo -> ncurses-devel -> gpm-devel ->
> > install-info.
> >
>
> How do you suggest to change it?

I have no specific idea. I have noted it in the gpm merge review
if I recall well I suggested there to avoid depending on install-info.
I think that you should try coordinate with the ncurses/gpm maintainers.
(as a side note there is a weird ncurses-devel <-> gpm-devel cross
dependency, and also ncurse dlopens gpm, it is very strange but
unrelated).

(In reply to comment #10)
> Created an attachment (id=292489) [edit]
> new spec file diff
> 

Aren't you the texinfo maintainer? Shouldn't you apply own 
patch?

As a side note the mimetype of your patch is wrong. Also
I would suggest using diff -u and using only .patch and 
.diff as file suffix.

(In reply to comment #12)
> (In reply to comment #10)
> > Created an attachment (id=292489) [edit] [edit]
> > new spec file diff
> > 
> 
> Aren't you the texinfo maintainer? Shouldn't you apply own 
> patch?
> 
> As a side note the mimetype of your patch is wrong. Also
> I would suggest using diff -u and using only .patch and 
> .diff as file suffix.

Yes, I'm. This is not patch, it's only spec file diff to show you another
changes I suggest to do and let you comment it. I apologize for the mimetype.

Ok. Aside from the Requires which seems unusefull to me
(and should be a Requires(post) if ever present), looks good.

Created attachment 292880 [details]
install tex files

also simplify iconv and keep timestamp.
ship xsl and dtd files.

It seems to me that the 
PreReq: bash
should be removed. To be on the safe side you can replace it with
something along:

# automatically detected, but to be on the safe side.
# This ensures that other packages which have triggers based on
# info don't run those triggers until the shell is in place as well.
Requires(post): /bin/sh
Requires: /bin/sh

But I think that you could also remove it entirely. If needed in 
%post it will be added, the requires is ensured by the /bin/sh
automatically added for %preun, and I don't think it is very useful 
anyway.

You can contact Erik if you want to...

Jindrich, could you please remove the  /usr/share/texmf/tex/texinfo/
directory from texlive?

It is now removed from texlive.

Everything looks good to me.

Only one thing is that maybe the dependency between texlive
and texinfo-tex could be the reverse, that is texlive 
Requires: texinfo-tex
since it is plain tex. But I'll leave that choice to Jindrich.
Just remove the requires tetex if the dependency is in the
other direction.

APPROVED.

Please rebuild texinfo as soon as possible, currently the texinfo tex
files are in the void.

This should certainly be closed.