Bug 226488 - Merge Review: texinfo
Merge Review: texinfo
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:
Blocks: F9MergeReviewTarget
  Show dependency treegraph
 
Reported: 2007-01-31 16:10 EST by Nobody's working on this, feel free to take it
Modified: 2008-02-24 13:40 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-24 13:40:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pertusus: fedora‑review+


Attachments (Terms of Use)
fixes for the spec file (3.05 KB, patch)
2008-01-11 17:44 EST, Patrice Dumas
no flags Details | Diff
new spec file diff (2.17 KB, application/octet-stream)
2008-01-22 05:04 EST, Vitezslav Crhonek
no flags Details
install tex files (2.11 KB, patch)
2008-01-24 18:13 EST, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:10:13 EST
Fedora Merge Review: texinfo

http://cvs.fedora.redhat.com/viewcvs/devel/texinfo/
Initial Owner: mitr@redhat.com
Comment 1 Patrice Dumas 2008-01-11 17:34:51 EST
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.

Comment 2 Patrice Dumas 2008-01-11 17:44:40 EST
Created attachment 291429 [details]
fixes for the spec file
Comment 3 Patrice Dumas 2008-01-11 17:46:24 EST
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.
Comment 4 Patrice Dumas 2008-01-11 17:49:11 EST
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
Comment 5 Jason Tibbitts 2008-01-13 11:07:36 EST
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.
Comment 6 Patrice Dumas 2008-01-13 11:25:42 EST
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...
Comment 7 Vitezslav Crhonek 2008-01-22 04:50:34 EST
(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?
Comment 8 Vitezslav Crhonek 2008-01-22 04:59:00 EST
(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.


Comment 9 Vitezslav Crhonek 2008-01-22 05:01:53 EST
(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.
Comment 10 Vitezslav Crhonek 2008-01-22 05:04:10 EST
Created attachment 292489 [details]
new spec file diff
Comment 11 Patrice Dumas 2008-01-22 05:22:46 EST
(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).
Comment 12 Patrice Dumas 2008-01-22 05:27:05 EST
(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.
Comment 13 Vitezslav Crhonek 2008-01-22 05:40:44 EST
(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.
Comment 14 Patrice Dumas 2008-01-22 05:51:24 EST
Ok. Aside from the Requires which seems unusefull to me
(and should be a Requires(post) if ever present), looks good.
Comment 15 Patrice Dumas 2008-01-24 18:13:27 EST
Created attachment 292880 [details]
install tex files

also simplify iconv and keep timestamp.
ship xsl and dtd files.
Comment 16 Patrice Dumas 2008-01-24 18:20:38 EST
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...
Comment 17 Patrice Dumas 2008-01-24 18:21:45 EST
Jindrich, could you please remove the  /usr/share/texmf/tex/texinfo/
directory from texlive?
Comment 18 Jindrich Novy 2008-01-25 03:27:11 EST
It is now removed from texlive.
Comment 19 Patrice Dumas 2008-01-25 18:05:41 EST
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.
Comment 20 Patrice Dumas 2008-02-03 14:05:36 EST
Please rebuild texinfo as soon as possible, currently the texinfo tex
files are in the void.
Comment 21 Patrice Dumas 2008-02-23 16:55:14 EST
This should certainly be closed.

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