Bug 225611

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

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

- removed grep and mktemp usage from post script, also the requires

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

zprikryl,
  will you please fix above issues reported in comment#4?

(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

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.

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. 

(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.

(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.

(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.

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+

(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.

(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

back needinfo->assigned

(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.

(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. 

(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?

--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.

Ok, in that case the file is not used at all to change the dir file,
but (of course) installed.

Patrice, Please feel free to review this officially as I got involved with some
other reviews.

I only have to say that it is 
APPROVED.

Thanks, Patrice.

Closing this as this is Merge-Review F9Target ticket.