Bug 454416 - Review Request: mingw32-zlib - MinGW Windows zlib compression library
Summary: Review Request: mingw32-zlib - MinGW Windows zlib compression library
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Adam Tkac
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: mingw32-binutils 454410 mingw32-runtime mingw32-w32api
Blocks: mingw32-openssl mingw32-freetype mingw32-libpng mingw32-libjpeg mingw32-libxml2 mingw32-gnutls mingw32-cairo mingw32-postgresql
TreeView+ depends on / blocked
 
Reported: 2008-07-08 11:54 UTC by Richard W.M. Jones
Modified: 2016-08-14 16:23 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-21 11:29:48 UTC
Type: ---
Embargoed:
atkac: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Richard W.M. Jones 2008-07-08 11:54:48 UTC
Spec URL: http://www.annexia.org/tmp/mingw/mingw-zlib.spec
SRPM URL: http://www.annexia.org/tmp/mingw/mingw-zlib-1.2.3-1.fc10.src.rpm
Description: MinGW Windows zlib compression library

Incomplete packaging guidelines:
https://fedoraproject.org/wiki/PackagingDrafts/MinGW

This is zlib cross-compiled for Windows.  We have packaged quite a few
libraries for Fedora so far, but not submitted them yet in case there
are problems with the packaging guidelines.  ZLib was chosen as a typical
library, simple and with no dependencies.

Comment 2 Adam Tkac 2008-12-10 14:07:04 UTC
SRPM and SPEC links seem incorrect. Could you please update them? I will take care about review.

Comment 3 Richard W.M. Jones 2008-12-10 16:17:39 UTC
Thanks for looking at this.  Here are the correct URLs for this package:

Spec URL: http://hg.et.redhat.com/cgi-bin/hg-misc.cgi/fedora-mingw--devel/file/tip/zlib/mingw32-zlib.spec
SRPM URL: http://www.annexia.org/tmp/mingw/fedora-10/src/SRPMS/mingw32-zlib-1.2.3-10.fc10.src.rpm

Comment 4 Peter Robinson 2008-12-16 15:01:37 UTC
Adam are you still reviewing this?

Comment 5 Adam Tkac 2008-12-17 13:20:18 UTC
Sorry for late response.

Formal review:

source files match upstream: YES
package meets naming and versioning guidelines: YES
specfile is properly named, is cleanly written and uses macros consistently: YES
dist tag is present: YES
build root is correct: YES
license field matches the actual license: YES
latest version is being packaged: YES
BuildRequires are proper: YES
compiler flags are appropriate: NO
%clean is present: YES
package builds in mock (Rawhide/x86_64): YES
-------
rpmlint is silent: NO:

$ rpmlint mingw32-zlib-1.2.3-10.fc11.src.rpm
mingw32-zlib.src:46: W: configure-without-libdir-spec
- wouldn't be better to add "--libdir=%{_mingw32_libdir}" configure parameter?

$ rpmlint mingw32-zlib-1.2.3-10.fc11.noarch.rpm
mingw32-zlib.noarch: W: no-documentation
mingw32-zlib.noarch: W: devel-file-in-non-devel-package
/usr/i686-pc-mingw32/sys-root/mingw/include/zlib.h
mingw32-zlib.noarch: W: devel-file-in-non-devel-package
/usr/i686-pc-mingw32/sys-root/mingw/lib/libz.dll.a
mingw32-zlib.noarch: W: devel-file-in-non-devel-package
/usr/i686-pc-mingw32/sys-root/mingw/include/zconf.h
mingw32-zlib.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libz.dll.a
mingw32-zlib.noarch: W: non-standard-dir-in-usr i686-pc-mingw32
- OK (there is no reason to separate headers to -devel - noone will use
  mingw32-zlib-libs package without -devel on Fedora)
------
final provides and requires look sane: YES
doesn't own any directories it shouldn't: YES
no duplicates in %files: YES
file permissions are appropriate: YES
code, not content: YES
documentation is small, so no -docs subpackage is necessary: YES
----------------------------------------------------------------

Currently I see two problems:
- compiler flags are not correct, it seems you have to add "CFLAGS=%{_mingw32_cflags}" parameter to make

- static version of library is packaged:
$ file /usr/i686-pc-mingw32/sys-root/mingw/lib/libz.dll.a
/usr/i686-pc-mingw32/sys-root/mingw/lib/libz.dll.a: current ar archive

MinGW packaging guidelines says that static libraries should be packaged in -static subpackage (http://fedoraproject.org/wiki/Packaging/MinGW, "Static libraries" statement). Yes, it is pedantry but guidelines should be honoured.

Comment 6 Richard W.M. Jones 2008-12-18 10:19:43 UTC
Just a quick note that libz.dll.a isn't a static library, in
the sense meant by the guidelines.

Under Windows, DLLs are split into two parts:  The foo-1.dll
which is the shared library needed at runtime.  But if you
want to link with the shared library, you need a library of
stub routines, called foo.dll.a (for cross-compilation)
or more normally foo.lib if this was a real Windows machine.

We actually remove the real static libraries from all of
the mingw32-* packages.

I'll take a look at the rest in an hour or two.

Comment 7 Richard W.M. Jones 2008-12-18 11:28:33 UTC
Thanks for looking at this package.  Here is an updated package
that sets CFLAGS correctly (and I've verified that they are passed
to gcc).

Spec URL: http://hg.et.redhat.com/cgi-bin/hg-misc.cgi/fedora-mingw--devel/file/tip/zlib/mingw32-zlib.spec
SRPM URL: http://www.annexia.org/tmp/mingw/fedora-10/src/SRPMS/mingw32-zlib-1.2.3-11.fc10.src.rpm

* Thu Dec 18 2008 Richard W.M. Jones <rjones> - 1.2.3-11
- Pass correct CFLAGS to build.

Comment 8 Adam Tkac 2008-12-18 12:02:08 UTC
(In reply to comment #6)
> Just a quick note that libz.dll.a isn't a static library, in
> the sense meant by the guidelines.
> 
> Under Windows, DLLs are split into two parts:  The foo-1.dll
> which is the shared library needed at runtime.  But if you
> want to link with the shared library, you need a library of
> stub routines, called foo.dll.a (for cross-compilation)
> or more normally foo.lib if this was a real Windows machine.

Ah, I didn't know how things work. Thanks for your explanation.

With correct CFLAGS package is OK => reviewed (formal review is in comment #5)

Comment 9 Richard W.M. Jones 2008-12-18 12:07:31 UTC
New Package CVS Request
=======================
Package Name: mingw32-zlib
Short Description: MinGW Windows zlib compression library
Owners: rjones berrange lfarkas
Branches: EL-5 F-10
InitialCC:

Comment 10 Kevin Fenzi 2008-12-21 03:56:14 UTC
cvs done.

Comment 11 Richard W.M. Jones 2008-12-21 11:29:48 UTC
Thanks Adam and Kevin.

Imported and built.

Comment 12 Thomas Sailer 2009-04-30 16:02:51 UTC
The native zlib package also generates the minizip and minizip-devel packages. These are also libraries and thus eligible for mingw32. Why have they been dropped?


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