Fedora Merge Review: zlib http://cvs.fedora.redhat.com/viewcvs/devel/zlib/ Initial Owner: varekova
* the source is not one of those appearing on the home page. Moreover you could use the tar.bz2. * Is Prefix: %{_prefix} needed? * BuildRoot is not the preferred one * The comment in %build is misleading. It should better be something like # prepare Makefile for the static lib and in %install there could be a comment saying # the first make triggers compilation of the object files, linking of the # shared library and installs the library # The second make triggers the linking of the static library and # its installation * I think it would be better to have, in -devel http://www.zlib.net/manual.html and http://www.zlib.net/zlib_how.html * it seems to me that FAQ should be in %doc, and ChangeLog should be in the main package * -devel should Requires: zlib = %{version}-%{release} * It seems to me that there should be a make clean between the 2 make -f invocations, to trigger recompilation with the flags without -fPIC * I'll attach a patch to simplify the build and install, and use more macros. * zutil.h seems to be an internal header that should no be shipped * seems like that spec is not in utf8, certainly because of Glomsrød * remove the dots at the end of the Summaries Suggestion: Change %defattr(-,root,root) to %defattr(-,root,root,-)
Created attachment 148247 [details] simplify %build and %install, remove redundant Prefix
I also suggest adding the dist tag.
Next the build should be done in build and the install in install.
(In reply to comment #1) Thanks for your comment. > * the source is not one of those appearing on the home page. Moreover > you could use the tar.bz2. fixed source tag > * Is Prefix: %{_prefix} needed? I prefer to leave prefix flag set there. > * BuildRoot is not the preferred one changed > * The comment in %build is misleading. It should better be something like > # prepare Makefile for the static lib changed > and in %install there could be a comment saying > # the first make triggers compilation of the object files, linking of the > # shared library and installs the library > # The second make triggers the linking of the static library and > # its installation this comment is not necessary > * I think it would be better to have, in -devel > http://www.zlib.net/manual.html > and > http://www.zlib.net/zlib_how.html These documents are not part of upstream tarball > * it seems to me that FAQ should be in %doc, and ChangeLog should be > in the main package changed and added > * -devel should > Requires: zlib = %{version}-%{release} changed > * It seems to me that there should be a make clean between the 2 > make -f invocations, to trigger recompilation with the flags without -fPIC > * I'll attach a patch to simplify the build and install, and use more > macros. fixed > * zutil.h seems to be an internal header that should no be shipped removed > * seems like that spec is not in utf8, certainly because of Glomsrød fixed > * remove the dots at the end of the Summaries fixed > Change %defattr(-,root,root) to %defattr(-,root,root,-) changed Fixed version is zlib-1.2.3-5.fc7
(In reply to comment #5) > > * Is Prefix: %{_prefix} needed? > I prefer to leave prefix flag set there. Why? It is allready set to that exact value? > > and in %install there could be a comment saying > > # the first make triggers compilation of the object files, linking of the > > # shared library and installs the library > > # The second make triggers the linking of the static library and > > # its installation > this comment is not necessary I disagree. Doing the build in %install is messy and deserves a comment. You should try to do your best such that anybody looking at your spec can understand immediately what you are doing. Doing comments for non standard build procedures is important. > These documents are not part of upstream tarball Why is it an issue? A description of the API is missing, th > > * It seems to me that there should be a make clean between the 2 > > make -f invocations, to trigger recompilation with the flags without -fPIC > > * I'll attach a patch to simplify the build and install, and use more > > macros. > fixed Not completely. I still see some issues with the spec file: * executables are compiled as part of %install and not %build * man pages are not installed in %{_mandir}, libs are not in %{_libdir}, headers are not in %{_includedir} * files compiled with shared libs flags (-fPIC) are used for static libraries.
Fixed in zlib-1.2.3-7.fc7.
* I don't like that much adding autotools support with a patch. Did upstream accept the patch? * zutil.h shouldn't be shipped * timestamp of .h and man files should be kept. * there is no description of the API. Please consider shipping the html pages as I suggest above (or any other description of the API). Suggestion: * in the libtool comment, replace bogus with unuseful, libtool is right in installing .la files.
Patrice, could you please look at the last version zlib-1.2.3-10.fc7 and approved this review request or if you see any reason why you don't want to aproved it here. But at first I want to reply to your comments: * adding autotools is the most clear solution and there is no problem with it * zutil.h is removed * timestamps are kept * the description is easy to get/find/grab it is not a part of zlib package and upstream don't want to add it so I think it is not necessary to add it too Thanks for your comments.
Created attachment 151869 [details] spec file that incorporates all my comments I removed the autotools patch in this spec. I think that such change is for upstream, not in a fedora package. Moreover this is not similar with upstream since the tests are removed. I cleaned the build such that the package is built in the build section and also used more rpm macros. and I added the manual, in my opinion this is a must - don't use the autotools, instead revert to the previous build procedure - ship the manual to have a description of the API - build the libraries in the %%build section rpmlint output is explained in comments in the spec E: zlib configure-without-libdir-spec W: zlib make-check-outside-check-section make check E: zlib configure-without-libdir-spec W: zlib make-check-outside-check-section make check Feel free to modify it again (for example if you like the build but not the manual).
Created attachment 151870 [details] keep header and man page timestamps
I think that the spec I proposed in Comment #10 is right, but if you really want to use the autotools and think my proposal is not right, I could also accept an autotool based package -- although I think it is not right.
Also I won't make shipping the api doc a must but I really can't understand why you don't want it, since the devel package is nearly unuseful without api documentation. It could be in a separated -doc subpackage, though.
This should go in a -static subpackage: %{_libdir}/*.a
Hello Patrice, please could you look to zlib-1.2.3-14, it seems for me to be right now - there is used automake but I think every other parts are clear.
Comment #14 is a must fix. But in any case I have too much disagreement to approve this package (the use of autotools and not shipping the api). I'll keep on commenting on other issues, though. At the first %build line, there is a spurious make %{?_smp_mflags} I don't see other obvious issues.
(In reply to comment #16) > Comment #14 is a must fix. But in any case I have too much The devel spec contains a -static subpackage: http://cvs.fedoraproject.org/viewcvs/rpms/libpng/devel/libpng.spec?view=markup btw. %makeinstall should not be used, better is to use (when it works, which is afaik almost always): make DESTDIR=$RPM_BUILD_ROOT install See: http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002
(In reply to comment #17) > (In reply to comment #16) > > Comment #14 is a must fix. But in any case I have too much > > The devel spec contains a -static subpackage: > http://cvs.fedoraproject.org/viewcvs/rpms/libpng/devel/libpng.spec?view=markup You seem to be mixing with the libpng review ;-) > btw. > %makeinstall > should not be used, better is to use (when it works, which is afaik almost always): > make DESTDIR=$RPM_BUILD_ROOT install > > See: > http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002
Hello, thanks for your comments. All problems you mentioned here are fixed in zlib-1.2.3-15.fc9.
The timestamps of the minizip headers are not kept. Using make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' should do the trick. The minizip headers should be in minizip-devel. in the minizip.pc file, either includedir should be /usr/include/minizip or the Cflags should have minizip subdir. A dot is missing at the end of the minizip description. Why aren't the minizip and miniunzip executables shipped? minizip-devel should Requires pkgconfig.
Thanks Patrice, fixed (zlib-1.2.3-16.fc9). There is no need to have minizip and miniunzip executables in this package it should only be use as a library.
(In reply to comment #21) > Thanks Patrice, fixed (zlib-1.2.3-16.fc9). > There is no need to have minizip and miniunzip executables in this package it > should only be use as a library. They can be in a subpackage, but they should be shipped. If not in this package, which package should they be part of? In fact it seems to me that the most common setup, when you want to separate executables from libraries is to have a minizip-libs package with the libraries (which will be automatically installed, or as a dependency of minizip-devel), and a minizip package with the executables.
I think they could not be shipped at all.
Why? Are they buggy?
Is anyone actually reviewing this? fedora-review is set to '?' but I dont' see anyone assigned to the package. I'm happy to review this if nobody else is doing so. I note a coupe of minor things: zlib.x86_64: W: file-not-utf8 /usr/share/doc/zlib-1.2.3/ChangeLog The ChangeLog file could use a trip through iconv. The License: tag says BSD, but I would find it odd if the zlib package isn't under the zlib license. I believe the License: tag should be "zlib". I can attach a patch or just fix these in CVS if you like. There are also several undefined-non-weak-symbol warnings like: minizip.x86_64: W: undefined-non-weak-symbol /usr/lib64/libminizip.so.1.0.1 inflateEnd and also for these symbols: deflate inflateInit2_ inflate crc32 deflateEnd deflateInit2_ get_crc_table I think these are bad practice but OK because the minizip.pc file specifies that packages which use it always link with libz, which will provide the symbols. I don't have any particular opinion on the minizip executables. I don't see why it would be mandatory to ship them if the maintainer doesn't want to, however. They're just source in the contrib directory of upstream's tarball, and it's not really common for that kind of thing to be shipped as part of our packages unless the maintainer feels some need to include it. I note that there's a small test suite; generally it's a good thing to call test suites even if they don't do all that much because they serve as a useful sanity check. But I added a %check section and it seems that "make check" doesn't actually do anything for some reason. When I call it manually I get some output ending with "*** zlib test OK ***". I'm not really sure what the problem is.
I'll go ahead and take this since nobody else has claimed it.
(In reply to comment #25) > Is anyone actually reviewing this? fedora-review is set to '?' but I dont' see > anyone assigned to the package. I'm happy to review this if nobody else is > doing so. I did some comments, but I disagree to much on the use of autotools without upstream consent. Especially when this is not really necessary and the tests aren't run anymore. > I don't have any particular opinion on the minizip executables. I don't see why > it would be mandatory to ship them if the maintainer doesn't want to, however. > They're just source in the contrib directory of upstream's tarball, and it's not > really common for that kind of thing to be shipped as part of our packages > unless the maintainer feels some need to include it. I don't think it is mandatory, but I haven't seen a good reason not to ship them.
> zlib.x86_64: W: file-not-utf8 /usr/share/doc/zlib-1.2.3/ChangeLog > The ChangeLog file could use a trip through iconv. Thanks - fixed > > The License: tag says BSD, but I would find it odd if the zlib package isn't > under the zlib license. I believe the License: tag should be "zlib". I have discussed this issue with Tom Callaway and his recommendation was it is "BSD" license so. > I note that there's a small test suite; generally it's a good thing to call test > suites even if they don't do all that much because they serve as a useful sanity > check. But I added a %check section and it seems that "make check" doesn't > actually do anything for some reason. When I call it manually I get some output > ending with "*** zlib test OK ***". I'm not really sure what the problem is. check added. I don't thing there is any reason to add minizip executables to subpackage so I don't like to do it. If I overlook some good reason to do it, write it here please. Thanks a lot. The last version is zlib-1.2.3-17.fc9.
Ivana, Apologies if I told you that zlib should be under the BSD license. Zlib has its own license, you can use License: zlib for the spec file here.
Thanks, license tag is fixed in zlib-1.2.3-18.fc9.
I think this package is quite clean now. Really the only thing that bothers me is the what was bothering Patrice earlier: the autool-ization of the original non-autotools-using source. According to cvs annotate, this was done in February, 2007 by Atam Tkac: * Tue Feb 20 2007 Adam Tkac <atkac redhat com> - 1.2.3-7 - building is now automatized - specfile cleanup However, there's nothing that explains why this is needed or what advantage there is to building it this way versus the way the package normally builds. I know this review is dragging on, but is it possible to at least get a couple of lines of comment in the specfile that explain why we need to do this, what bugs it solves, or what problems it avoids?
Ivana, ping?
Hello, Stepan Kasal will look at this issue.
(In reply to comment #31) > Really the only thing that bothers me is [...] the autool-ization of the > original non-autotools-using source. First, about problems with the original build system of zlib: The same CFLAGS variable is used for static and dynamic library. So using this simple build system is not so simple as using the complex autotools system. You need to do something like: make mv libz.a save-libz.a make clean ./configure -s make mv save-libz.a libz.a To see another variant of this trick, see the spec file just before the autoconfiscation: http://cvs.fedora.redhat.com/viewvc/devel/zlib/zlib.spec?revision=zlib-1_2_3-6_fc7 Second, there is minizip-*-autotools.patch. contrib/minizip/Makefile does not contain any rules for building libminizip.so. Consequently, some hacking is needed to get the library built; using libtool (through Automake) is a sensible way. With these things in mind, I believe that the autoconfiscation incures less maintanance costs than the complicated spec file would. Does this sound fair? If yes, should an excerpt of this explanation go to the spec file? (Text suggestions welcome. ;-) BTW, I'm going to do some cleanup of the autotools patches. But I don't think it would be a good idea to go back to the build system shipped with zlib.
> With these things in mind, I believe that the autoconfiscation incures less > maintanance costs than the complicated spec file would. That seems quite reasonable. I'd suggest just noting that in the specfile with something like: # The following two patches add an autotools build system to work around # problems in the regular zlib build system. just to make things clear, but it's not seriously important. I think that takes care of this review. Thanks for your time. APPROVED