* RPM name is OK
* Source bzip2-1.0.4.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Is it necessary to include static libraries? See wiki: Packaging/Guidelines#Exclusion of Static Libraries

* README.COMPILATION.PROBLEMS don't seems to be useful to me

* A description of the api should be in the -devel package, I propose
  manual.html

* the main and devel package should use a full versioned dependency for 
  bzip2-libs:
Requires: bzip2-libs = %{version}-%{release}

* I don't see why the devel package requires the main package

* you should use the -p to keep timestamp on unmodified files installed 
  like bzlib.h, man pages

* Requires(post) and postun missing for -libs, /sbin/ldconfig

"Source:" is 404 not found.
Here is v1.0.4: http://www.bzip.org/1.0.4/bzip2-1.0.4.tar.gz


> Requires: bzip2-libs = %{version}

There is an automatic dependency on the library sonames already, btw.


> Requires(post) and postun missing for -libs, /sbin/ldconfig

No, they are automatic due to "-p".

(In reply to comment #3)

> > Requires: bzip2-libs = %{version}
> 
> There is an automatic dependency on the library sonames already, btw.

Indeed, but I think it is better if an update of the main package
with, say
yum update bzip2
triggers an update of the -libs if there is a newer version.

> > Requires(post) and postun missing for -libs, /sbin/ldconfig
> 
> No, they are automatic due to "-p".

Oops... 

(In reply to comment #4)
> (In reply to comment #3)
> 
> > > Requires: bzip2-libs = %{version}
> > 
> > There is an automatic dependency on the library sonames already, btw.
> 
> Indeed, but I think it is better if an update of the main package
> with, say 
> yum update bzip2
Please try this with today's bzip2 update!

=> It doesn't work with the current deps.

=> If this is really wanted (which I disagree up on) then the dep must be
Requires: bzip2-libs = %{version}-%{release}

Requires: bzip2-libs = %{version} is non-functional

> triggers an update of the -libs if there is a newer version.
To me this would be an unnecessary restrictive requirement.

The automatic dep on libbzip2.so.1 will make sure a sufficently compatible lib
will be pulled in.

(In reply to comment #5)

> The automatic dep on libbzip2.so.1 will make sure a sufficently compatible lib
> will be pulled in.

Imagine that a user get to know that there is a serious flaw in 
bzip2. If the issue is really in the lib, upon doing yum update bzip2
the lib won't be updated, I think it is unfortunate. It shouldn't
only require a compatible lib, but the implementation associated with
the command, in my opinion, since they come from the same source.

"yum update" alone works. "yum update bzip2*" works, too. We don't add
artificial Requires simply because you think people may be unaware that bzip2
comes in two parts. If they are unaware, they probably use pup or some other
graphical interface which will show both packages scheduled for update.
Following your line of reasoning, we'd need to add similar Requires: for all
packages where -libs subpackage exists.

(In reply to comment #7)
> "yum update" alone works. "yum update bzip2*" works, too. We don't add
> artificial Requires simply because you think people may be unaware that bzip2
> comes in two parts. If they are unaware, they probably use pup or some other

They are not so artificial, since both packages come from the same 
tarball. Would they have been installed with ./configure && make && make install
they would have been together.

> graphical interface which will show both packages scheduled for update.
> Following your line of reasoning, we'd need to add similar Requires: for all
> packages where -libs subpackage exists.

Indeed. But I don't make that a blocker, it is just a suggestion (maybe I 
wasn't clear about that) if the contributor don't like, no problem with me. 
In that case, the unuseful

Requires: bzip2-libs = %{version}

should go away, though.

So, to summarize this discussion, the requester has to

- Change the BuildRoot
- Remove the static libraries or provide reasoning for why they're included.
- Drop README.COMPILATION.PROBLEMS
- Add api docs to the -devel subpackage
- Preserve timestamps when installing files
- Check if the -devel package needs to Require the main package, and if so,
change it to Requires: bzip2 = %{version}-%{release}
- Remove the Requires: bzip2-libs = %{version}

Ivana, can you do that?

Additionally URL and source seems wrong to me. They seem to be
http://www.bzip.org/
and
http://www.bzip.org/1.0.4/bzip2-1.0.4.tar.gz

A minor suggestion is to remove the -f in rm invocation, to have it
fail if the .o aren't generated:
rm *.o

> check if the -devel package needs to Require the main package,
> and if so, change it to Requires: bzip2 = %{version}-%{release}
> - Remove the Requires: bzip2-libs = %{version}

The -devel packages does NOT need the "main package", but the
bzip2-libs package (comment #2):

  Requires: bzip2-libs = %{version}-%{release}

in the -devel package. The %release in there makes sure that -devel
and -libs package are in sync always (think run-time bug-fixes +
%changelog of both packages), also for a "yum install bzip2-devel"
when an older bzip2-libs is installed already. Without the strict
dependency it would be possible to install the latest bzip2-devel
while keeping an older bzip2-libs which e.g. has bugs.

The main package depends on the -libs package through an automatic
dependency on "libbz2.so.1", created by rpmbuild. If there is
reason to not trust the accuracy of that dependency, an explicit
dependency on a specific package %version-%release would be needed.
The simple "Requires: bzip2-libs = 1.0.4" does not add any value
except that it doesn't trust an automatic upgrade from 1.0.3 to 1.0.4.

Thanks for your comments.
All problems should be fixed in bzip2-1.0.4-5.fc7.

The man pages are installed with bad perms. Should be
install -p -m644 bzip2.1 bzdiff.1 bzgrep.1 bzmore.1 
$RPM_BUILD_ROOT/%{_mandir}/man1/

-devel should not require the main package.

There is no static library, the %description should be updated.



Suggestions: 

use %defattr(-,root,root,-) instead of %defattr(-,root,root)

use 
URL: http://www.bzip.org/ 

Fixed in bzip2-1.0.4-6.fc7.

Seems almost right to me, still 2 issues and some suggestions:

* remove 'static library' from the -devel package description
  since there is no static library

* the original soname don't follow the usual convention of a soname
  number with an integer, but I am not certain that it is right to 
  modify it in fedora. It should better be changed upstream. What is
  the reasoning behind this change?


Suggestions:

* move 
chmod 644 bzlib.h 
chmod 644 bzip2.1 bzdiff.1 bzgrep.1 bzmore.1
to %prep, since these perms should be like that in the tarball

* remove the / between $RPM_BUILD_ROOT and macros, like %{_mandir},
  %{_libdir}... since they allready have a leading /

* for %patch6, maybe it could be
%patch6 -p1 -b .bzip2recover

* The changelog entries 'incorporate the next review feedback' are not
  particularly useful, I agree that for simple changes it is right, but
  here there was a complete reorganization of the build between 2 release,
  removal of static lib, this would have, in my opinion, deserved a more 
  verbose changelog


A remark:

* I completely agree with the new organization of the spec with build in
  %build and install in %install, I would have asked for that the next round
  anyway ;-)

I'm not absolutely sure, but doesn't rpm require bzip2 static files, to build 
itself as static one?

(In reply to comment #15)

> * I completely agree with the new organization of the spec with build in
>   %build and install in %install, I would have asked for that the next round
>   anyway ;-)

Seems that I confused reviews, this comment is irrelevant here.

(In reply to comment #15)
> Seems almost right to me, still 2 issues and some suggestions:
> 
> * remove 'static library' from the -devel package description
>   since there is no static library
The static library was put back becouse of rpm package need it so - the
description should remain too.

> * the original soname don't follow the usual convention of a soname
>   number with an integer, but I am not certain that it is right to 
>   modify it in fedora. It should better be changed upstream. What is
>   the reasoning behind this change?
It is the upstream resolution so fedora should accept it 


> * remove the / between $RPM_BUILD_ROOT and macros, like %{_mandir},
>   %{_libdir}... since they allready have a leading /
fixed 

> * for %patch6, maybe it could be
> %patch6 -p1 -b .bzip2recover
changed
  
> A remark:
> 
> * I completely agree with the new organization of the spec with build in
>   %build and install in %install, I would have asked for that the next round
>   anyway ;-)

the last version is bzip2-1.0.4-8.fc7.

I could be wrong, but it looks like rpm links to the dynamic bzip library:

[ruben@odin rpm]$ ldd /bin/rpm | grep bz2
        libbz2.so.1 => /usr/lib/libbz2.so.1 (0x0604a000)

Only if rpm is dynamically built. Behaviour switched over the last time between 
static and dynamic, IIRC.

Oh and you sometimes *need* a static rpm. E.g. on a system you can't reboot 
that often for a rescue system and yum made a boo-boo at the last update and 
broke the dynamic rpm. A static rpm will save your ass then in a normal time.

(In reply to comment #18)
> (In reply to comment #15)

> > * the original soname don't follow the usual convention of a soname
> >   number with an integer, but I am not certain that it is right to 
> >   modify it in fedora. It should better be changed upstream. What is
> >   the reasoning behind this change?
> It is the upstream resolution so fedora should accept it 

But it is not what is done in fedora! the soname is changed in the
bzip2-1.0.4-saneso.patch patch, and I question that change.


Otherwise
* there is no need of -p when installing generated binaries, like
  libbz2.a, bzip2-shared... And the static lib should be 0644. I personally
  like to have the -mxxx option even when it isn't stricly needed, so
  what I would have done is:

install -m644 libbz2.a $RPM_BUILD_ROOT%{_libdir}
install -m755 libbz2.so.%{version} $RPM_BUILD_ROOT%{_libdir}
install -m755 bzip2-shared  $RPM_BUILD_ROOT%{_bindir}/bzip2
install -m755 bzip2recover bzgrep bzdiff bzmore  $RPM_BUILD_ROOT%{_bindir}/

The only mandatory item here is to have -m644 on the libbz2.a install
call.

(In reply to comment #22)
> (In reply to comment #18)
> > (In reply to comment #15)
> 
> > > * the original soname don't follow the usual convention of a soname
> > >   number with an integer, but I am not certain that it is right to 
> > >   modify it in fedora. It should better be changed upstream. What is
> > >   the reasoning behind this change?
> > It is the upstream resolution so fedora should accept it 
> 
> But it is not what is done in fedora! the soname is changed in the
> bzip2-1.0.4-saneso.patch patch, and I question that change.
> 
I'm sorry, I have not understand your previous comment so my reply was confused. 
So I'm not sure what are you asking about. 
The version which is in fedora now is right - if there will be any change then
there would be necessary to rebuild all dependencies. I'm not sure why was this
change done (it was about 6 years ago), but it is ok.
This problem should not be a subject of review.
 
> Otherwise
> * there is no need of -p when installing generated binaries, like
>   libbz2.a, bzip2-shared... And the static lib should be 0644. I personally
>   like to have the -mxxx option even when it isn't stricly needed, so
>   what I would have done is:
> 
> install -m644 libbz2.a $RPM_BUILD_ROOT%{_libdir}
> install -m755 libbz2.so.%{version} $RPM_BUILD_ROOT%{_libdir}
> install -m755 bzip2-shared  $RPM_BUILD_ROOT%{_bindir}/bzip2
> install -m755 bzip2recover bzgrep bzdiff bzmore  $RPM_BUILD_ROOT%{_bindir}/
> 
> The only mandatory item here is to have -m644 on the libbz2.a install
> call.
Thanks. Fixed in bzip2-1.0.4-10.fc7.

Ruben,
could you please look at bzip2-1.0.4-10.fc7 and approved this review request or
if you see any reason why you wdon't want to aproved it here. 
Thanks.

Hi Ivana,

I went throught all the comments above, and I don't see any further blockers, so bzip2-1.0.4-10.fc7 is 
approved.

(In reply to comment #23)

> The version which is in fedora now is right - if there will be any change then
> there would be necessary to rebuild all dependencies. I'm not sure why was this
> change done (it was about 6 years ago), but it is ok.
> This problem should not be a subject of review.

Indeed it should. The reverse is right. Letting pass that item 
without discussing it wouldn't be right. This is not a hard
blocker, indeed. 

It is much too late to change it, indeed, if it has to be
changed it should be changed right after fedora 7 is released.
Also it could be decided not to change it, but use the upstream
soname versionning if there is a new soname.

Still this issue is not clear to me. Do you have an idea about
what would be best (use libbz2.so.1 as soname or libbz2.so.1.0)?
It seems right to me not to change the soname, but this issue
should be settled down now such that it is easy to do the
right thing when the soname is changed upstream, and a comment 
should be added in the spec to explain everything and give 
guidance for the future.

Any comment on future sonames handlings?