Bug 225633
Summary: | Merge Review: bzip2 | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nobody's working on this, feel free to take it <nobody> |
Component: | Package Review | Assignee: | Ruben Kerkhof <ruben> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | pertusus, redhat-bugzilla, ruben, varekova |
Target Milestone: | --- | Flags: | ruben:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-06-17 06:40:05 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Nobody's working on this, feel free to take it
2007-01-31 17:48:49 UTC
* 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? |