Bug 225611 - Merge Review: bc
Merge Review: bc
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
: Reopened
Depends On:
Blocks: F9MergeReviewTarget
  Show dependency treegraph
 
Reported: 2007-01-31 12:44 EST by Nobody's working on this, feel free to take it
Modified: 2008-01-31 00:48 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-31 00:48:21 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)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:44:52 EST
Fedora Merge Review: bc

http://cvs.fedora.redhat.com/viewcvs/devel/bc/
Initial Owner: twoerner@redhat.com
Comment 1 Pace Willisson 2007-02-03 15:53:14 EST
spec file says source is bc-1.06.tar.bz2 but gnu ftp site
actually only has bc-1.06.tar.gz   The actual contents of the
file in the rpm package (when uncompressed) does match the gnu site.

grep is in the build requirements because apparantly an old version of
bc inserted a junk line into the "dir" file in the info system.  grep
is used in the %post script to detect the problem, and to clean the
file.  This script also uses "mv", causing rpmlint to complain about
a dangerous command.

This %post script, and the dependency on grep can probably be
removed at this point.

rpmlint complains "e: bc tag-not-utf8 %changelog" due to Trond entries

remove dot from end of Summary
Comment 2 Karsten Hopp 2007-02-23 06:04:00 EST
I've left the grep in the %post section, but fixed the requirements. Other
changes are:
- fix buildroot
- remove trailing dot from summary
- fix post/preun requirements
- use make install DESTDIR=...
- convert changelog to utf-8
- use smp flags
Comment 3 Thomas Woerner 2007-02-26 09:40:01 EST
- removed grep and mktemp usage from post script, also the requires
Comment 4 Patrice Dumas 2007-02-26 17:55:02 EST
Issues:

* Why is there an autotools call?

* use %{_bindir} in %files.

Suggestions:

* use the "official" scriptlet for install-info and remove the .gz.
It is just for consistency since this scriptlet is fine.

* remove / in  $RPM_BUILD_ROOT/%{_infodir}

* use the dist tag
Comment 5 Parag AN(पराग) 2007-12-06 23:54:06 EST
zprikryl,
  will you please fix above issues reported in comment#4?
Comment 6 Zdenek Prikryl 2007-12-07 03:59:49 EST
(In reply to comment #4)
> * Why is there an autotools call?

Because a tarball doesn't contain all needed files. For example a file depcomp
is missing. So one has to create it for successfully compilation.

> * use %{_bindir} in %files.

Fixed

> * use the "official" scriptlet for install-info and remove the .gz.
> It is just for consistency since this scriptlet is fine.

Fixed and added bc info pakges

> * remove / in  $RPM_BUILD_ROOT/%{_infodir}

Since "//" is interpreted like "/", I think that $RPM_BUILD_ROOT/%{_infodir} is
more readable than $RPM_BUILD_ROOT%{_infodir}, so I will not change it.

> * use the dist tag

Fixed
Comment 7 Parag AN(पराग) 2007-12-07 05:51:44 EST
I guess I have not yet APPROVED this review. So this bugzilla cannot be closed.
Status of fedora-review flag is still ? not +

Where can I see fixed package? Is the fixed SPEC built for rawhide?
I need to do further review so provide updated SRPM.

Remember this is Merge Review not any bugzilla problem/RFE.
Comment 8 Parag AN(पराग) 2007-12-07 06:03:49 EST
Ok, I saw you committed some changes in SPEC.
Still missing correct licesne tag see =>
bc.src: W: invalid-license GPL
The value of the License tag was not recognized. 

Comment 9 Patrice Dumas 2007-12-07 08:30:16 EST
(In reply to comment #6)
> (In reply to comment #4)
> > * Why is there an autotools call?
> 
> Because a tarball doesn't contain all needed files. For example a file depcomp
> is missing. So one has to create it for successfully compilation.

This is not a good reason. If depcomp is missing, just copy it.

> more readable than $RPM_BUILD_ROOT%{_infodir}, so I will not change it.

Right.

Comment 10 Stepan Kasal 2007-12-14 05:00:15 EST
(In reply to comment #6)
> (In reply to comment #4)
> > * Why is there an autotools call?
> 
> Because a tarball doesn't contain all needed files. [...]

The situation was a bit more tricky, actually:
One of the old patches touched configure.in, so the make called autotools to
refresh all generated files.  This led to problems, like the complaint about
missing "depcomp" script, since the tarball was created using Automake 1.4.

It turns out that the change to configure.in is no longer needed so the patch
can be removed.  Then the autotools call in spec file can be removed, too.
Comment 11 Patrice Dumas 2007-12-14 05:27:11 EST
(In reply to comment #10)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > * Why is there an autotools call?
> > 
> > Because a tarball doesn't contain all needed files. [...]
> 
> The situation was a bit more tricky, actually:
> One of the old patches touched configure.in, so the make called autotools to
> refresh all generated files.  This led to problems, like the complaint about
> missing "depcomp" script, since the tarball was created using Automake 1.4.
> 
> It turns out that the change to configure.in is no longer needed so the patch
> can be removed.  Then the autotools call in spec file can be removed, too.

Even in this case there are better way to do:
* if configure can be patched (which is the case here for the 
  bc-1.06-flex.patch) configure should be patched and autotools not run
* if the changes are more complicated and patching Makefile.in or 
  configure is not possible, the automake/autoconf used to generate the
  tarball (or a compatible one) should be used instead of the current
  autoconf/automake.
Comment 12 Patrice Dumas 2007-12-14 05:47:13 EST
This is fixed now anyway. 

I also propose adding the Examples directory do %doc

The source timestamp is not kept, but it is too late for this 
release don't forget to keep it next time:
-rw-rw-r-- 1 dumas dumas 278926 nov 16  2000 bc-1.06.tar.gz
-rw-rw-r-- 1 dumas dumas 278926 jun 25  2004 bc-1.06.tar.gz-rh

I tried to run the tests, but it doesn't work simply, it 
is not obvious that it is useful and it takes very long 
time to run.

License seems to be GPLv2+
Comment 13 Stepan Kasal 2007-12-14 06:32:45 EST
(In reply to comment #11)
> Even in this case there are better way to do:
> * if configure can be patched (which is the case here for the 
>   bc-1.06-flex.patch) configure should be patched and autotools not run

agreed, this is a good solution for this particular case, provided you preserve
the timestamp of configure.ac (configure.in, in this particular case), so that
make knows that all files depending on the patched source are up-to-date.

> * if the changes are more complicated and patching Makefile.in or 
>   configure is not possible, the automake/autoconf used to generate the
>   tarball (or a compatible one) should be used instead of the current
>   autoconf/automake.

Again this is a good advice for this package, since it uses ancient autotools
(Automake 1.4, for example).

But if some bigger changes are done to a not-so-ancient tarball (using say
Autoconf >= 2.59 and Automake >= 1.8) it is IMHO better to run autoreconf on the
patched tree.  Autotools are much better wrt backward compatibility these days.

But I know that Fedora Packaging Guidelines generally advice against running
autotools from the spec file.
Comment 14 Stepan Kasal 2007-12-14 06:45:42 EST
(In reply to comment #12)
> License seems to be GPLv2+

I have committed this, too.
rpmlint is now silent on both binary and source rpm

Comment 15 Stepan Kasal 2007-12-14 06:48:27 EST
back needinfo->assigned
Comment 16 Patrice Dumas 2007-12-14 07:54:40 EST
(In reply to comment #13)

> But if some bigger changes are done to a not-so-ancient tarball (using say
> Autoconf >= 2.59 and Automake >= 1.8) it is IMHO better to run autoreconf on the
> patched tree.  Autotools are much better wrt backward compatibility these days.

This is also already taken into account, since there is no automake18
package, there is only automake14 to automake17, and there is only
autoconf213.



Anyway I still think that having Examples in %doc would be better,
but this is definitively not a blocker, to me this package is 
approvable, now it is up to Parag.
Comment 17 Zdenek Prikryl 2007-12-14 07:56:17 EST
(In reply to comment #12)
> This is fixed now anyway. 
> 
> I also propose adding the Examples directory do %doc

Added.

> I tried to run the tests, but it doesn't work simply, it 
> is not obvious that it is useful and it takes very long 
> time to run.

Hmmm I thing that samples, which are in the Example directory, are sufficient
for users. And samples in the Test directory are useful only for developers. For
example, I check a functionality after I patched something with these test samples.

I have committed the spec file with Examples added to %doc. I'll wait with
rebuild so you can add some additional comments. 
Comment 18 Patrice Dumas 2007-12-20 17:47:12 EST
(In reply to comment #17)
> > I tried to run the tests, but it doesn't work simply, it 
> > is not obvious that it is useful and it takes very long 
> > time to run.
> 
> Hmmm I thing that samples, which are in the Example directory, are sufficient
> for users. And samples in the Test directory are useful only 
> for developers. For

Not only. It is useful to run tests in %check in general.

> example, I check a functionality after I patched something with 
> these test samples.

Indeed, but it can also be useful each time you build a rpm.

But in that case I don't think it is worth bothering with it,
especially if you run them manually.

> I have committed the spec file with Examples added to %doc. I'll wait with
> rebuild so you can add some additional comments. 

One more comment, is the --entry= switch to /sbin/install-info --delete?
Comment 19 Zdenek Prikryl 2007-12-21 03:06:41 EST
--entry is needed, because info files does not contain definition of a directory
entry. Which is something like a key. If you want to install an info file, you
need to specify the directory entry. One way is to patch the info file, but I
think this is unneeded, if you specify this directory entry as a command line
parameter, it is sufficient.
Comment 20 Patrice Dumas 2007-12-21 05:21:10 EST
Ok, in that case the file is not used at all to change the dir file,
but (of course) installed.
Comment 21 Parag AN(पराग) 2008-01-15 23:24:08 EST
Patrice, Please feel free to review this officially as I got involved with some
other reviews.
Comment 22 Patrice Dumas 2008-01-16 03:43:59 EST
I only have to say that it is 
APPROVED.
Comment 23 Parag AN(पराग) 2008-01-16 03:49:15 EST
Thanks, Patrice.
Comment 24 Parag AN(पराग) 2008-01-31 00:48:21 EST
Closing this as this is Merge-Review F9Target ticket.

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