Bug 226671 - Merge Review: zlib
Merge Review: zlib
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: F9MergeReviewTarget
  Show dependency treegraph
 
Reported: 2007-01-31 16:37 EST by Nobody's working on this, feel free to take it
Modified: 2009-01-13 16:13 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-13 16:13:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tibbs: fedora‑review+


Attachments (Terms of Use)
simplify %build and %install, remove redundant Prefix (1.80 KB, patch)
2007-02-16 16:18 EST, Patrice Dumas
no flags Details | Diff
spec file that incorporates all my comments (8.07 KB, text/plain)
2007-04-06 07:47 EDT, Patrice Dumas
no flags Details
keep header and man page timestamps (807 bytes, patch)
2007-04-06 07:55 EDT, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:37:19 EST
Fedora Merge Review: zlib

http://cvs.fedora.redhat.com/viewcvs/devel/zlib/
Initial Owner: varekova@redhat.com
Comment 1 Patrice Dumas 2007-02-16 16:16:57 EST
* 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,-)
Comment 2 Patrice Dumas 2007-02-16 16:18:28 EST
Created attachment 148247 [details]
simplify %build and %install, remove redundant Prefix
Comment 3 Patrice Dumas 2007-02-17 04:43:59 EST
I also suggest adding the dist tag.
Comment 4 Patrice Dumas 2007-02-18 08:46:44 EST
Next the build should be done in build and the install in install.
Comment 5 Ivana Varekova 2007-02-19 07:18:14 EST
(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
Comment 6 Patrice Dumas 2007-02-19 18:22:25 EST
(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.
Comment 7 Ivana Varekova 2007-02-21 07:12:08 EST
Fixed in zlib-1.2.3-7.fc7.
Comment 8 Patrice Dumas 2007-02-21 18:30:53 EST
* 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. 
Comment 9 Ivana Varekova 2007-04-06 05:46:50 EDT
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. 
Comment 10 Patrice Dumas 2007-04-06 07:47:02 EDT
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).
Comment 11 Patrice Dumas 2007-04-06 07:55:50 EDT
Created attachment 151870 [details]
keep header and man page timestamps
Comment 12 Patrice Dumas 2007-04-06 09:43:55 EDT
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.
Comment 13 Patrice Dumas 2007-05-21 06:41:06 EDT
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.
Comment 14 Till Maas 2007-08-31 10:03:54 EDT
This should go in a -static subpackage:
%{_libdir}/*.a
Comment 15 Ivana Varekova 2007-09-06 10:25:38 EDT
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 16 Patrice Dumas 2007-10-27 16:55:32 EDT
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.
Comment 17 Till Maas 2007-10-27 20:31:38 EDT
(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
Comment 18 Patrice Dumas 2007-10-28 05:04:38 EDT
(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

Comment 19 Ivana Varekova 2007-11-14 10:44:52 EST
Hello, thanks for your comments. 
All problems you mentioned here are fixed in zlib-1.2.3-15.fc9.
Comment 20 Patrice Dumas 2007-11-18 05:20:06 EST
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.
Comment 21 Ivana Varekova 2007-11-23 05:46:34 EST
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.
Comment 22 Patrice Dumas 2007-11-23 06:28:11 EST
(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.
Comment 23 Ivana Varekova 2007-11-23 08:12:42 EST
I think they could not be shipped at all. 
Comment 24 Patrice Dumas 2007-11-23 13:14:21 EST
Why? Are they buggy?
Comment 25 Jason Tibbitts 2007-12-21 21:03:20 EST
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.
Comment 26 Jason Tibbitts 2008-01-07 12:25:32 EST
I'll go ahead and take this since nobody else has claimed it.
Comment 27 Patrice Dumas 2008-01-08 04:01:07 EST
(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.
Comment 28 Ivana Varekova 2008-02-11 10:02:06 EST
>   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.
Comment 29 Tom "spot" Callaway 2008-02-12 11:35:41 EST
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.
Comment 30 Ivana Varekova 2008-02-13 03:33:37 EST
Thanks, license tag is fixed in zlib-1.2.3-18.fc9.
Comment 31 Jason Tibbitts 2008-03-03 00:57:20 EST
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?
Comment 32 Robert Scheck 2008-12-20 09:15:17 EST
Ivana, ping?
Comment 33 Ivana Varekova 2009-01-07 06:47:02 EST
Hello, Stepan Kasal will look at this issue.
Comment 34 Stepan Kasal 2009-01-07 07:58:20 EST
(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.
Comment 35 Jason Tibbitts 2009-01-13 16:12:52 EST
> 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

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