Bug 710101 - Review Request: mingw-libjpeg-turbo - MinGW Windows Libjpeg-turbo library
Summary: Review Request: mingw-libjpeg-turbo - MinGW Windows Libjpeg-turbo 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: Kalev Lember
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 604702
TreeView+ depends on / blocked
 
Reported: 2011-06-02 13:04 UTC by Erik van Pienbroek
Modified: 2011-06-10 17:26 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-06-10 17:26:20 UTC
Type: ---
Embargoed:
kalevlember: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Erik van Pienbroek 2011-06-02 13:04:29 UTC
Spec URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo.spec
SRPM URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo-1.1.1-1.fc15.src.rpm
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3105772
Description: MinGW Windows cross compiled Libjpeg-turbo library.

This package is intended to replace the mingw32-libjpeg package in F-16

Comment 1 Erik van Pienbroek 2011-06-02 13:12:54 UTC
The jpeg_boolean hacks which were applied in mingw32-libjpeg have been dropped from this package (bug 604702)

The following packages have to be rebuilt/updated once this package gets imported:
mingw32-OpenSceneGraph
mingw32-SDL_image
mingw32-gdk-pixbuf
mingw32-jasper
mingw32-libtiff
mingw32-qt
mingw32-webkitgtk

Comment 2 Kalev Lember 2011-06-02 18:12:46 UTC
Taking for review.

I'm looking at the installed public headers and jmorecfg.h still has:
typedef int boolean;

This is going to cause trouble because Windows headers (in particular rpcndr.h) define boolean as 'unsigned char'.

libjpeg-turbo has two different build systems. In addition to the autotools-based one you've used, there's also a CMake based build system. It would appear that there's at least one important difference for MinGW builds: the cmake build system uses win/jconfig.h.in, which in turn defines:

/* Define "boolean" as unsigned char, not int, per Windows custom */
#ifndef __RPCNDR_H__            /* don't conflict if rpcndr.h already read */
typedef unsigned char boolean;
#endif
#define HAVE_BOOLEAN            /* prevent jmorecfg.h from redefining it */


So in the light of this, I can suggest two options:
 1) Fix up the autotools build system to also typedef boolean as 'unsigned char'
 2) Switch to CMake build system

Comment 3 Erik van Pienbroek 2011-06-02 18:18:00 UTC
That's really surprising! This would mean that the ABI will be different based on the build system used. I don't know if that's really how upstream has meant this to work..

Comment 4 Kalev Lember 2011-06-02 18:26:44 UTC
I think the CMake build system is there mostly for Windows (it can generate Visual Studio projects) and upstream haven't yet properly tested Windows support with the autotools based build system.

Comment 5 Erik van Pienbroek 2011-06-02 18:44:38 UTC
I just updated the package to use the CMake based build system as that looks like the most Win32-friendly method to get this package built

Spec URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo.spec
SRPM URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo-1.1.1-2.fc15.src.rpm
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3106615

* Thu Jun  2 2011 Erik van Pienbroek <epienbro> - 1.1.1-2
- Use CMake to build this package as it creates a more mingw-friendly
  version of the library

Comment 6 Kalev Lember 2011-06-02 19:22:14 UTC
Fedora review mingw-libjpeg-turbo-1.1.1-2.fc15.src.rpm 2011-06-02

+ OK
! needs attention

rpmlint output:
$ rpmlint mingw32-libjpeg-turbo \
          mingw32-libjpeg-turbo-static \
          mingw32-libjpeg-turbo-debuginfo \
          mingw-libjpeg-turbo-1.1.1-2.fc16.src.rpm
mingw32-libjpeg-turbo-static.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libturbojpeg.a
mingw32-libjpeg-turbo-static.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libjpeg.a
mingw32-libjpeg-turbo-static.noarch: W: no-documentation
mingw32-libjpeg-turbo-debuginfo.noarch: E: debuginfo-without-sources
mingw-libjpeg-turbo.src:94: W: macro-in-comment %{_mingw32_libdir}
mingw-libjpeg-turbo.src:96: W: macro-in-comment %{_mingw32_libdir}
4 packages and 0 specfiles checked; 3 errors, 3 warnings.

! rpmlint doesn't like the commented out lines in %files section; I guess
  these can be safely removed to make rpmlint happy. Other errors and
  warnings are harmless.
+ The package is named according to Fedora MinGW packaging guidelines
+ The spec file name matches the package base name
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The stated license is the same as the one for the corresponding
  native Fedora package
! The package doesn't contain the license files
  (LICENSE.txt and LGPL.txt)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  03b9c1406c7bfdc204313c2917ce6962  libjpeg-turbo-1.1.1.tar.gz
  03b9c1406c7bfdc204313c2917ce6962  Download/libjpeg-turbo-1.1.1.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file MUST handle locales properly
n/a ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files should be in -devel
    Fedora MinGW guidelines allow headers in main package
+ Static libraries should be in -static
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Directory ownership sane
+ Filenames are valid UTF-8


Issues:
! Would be nice to clean up the rpmlint warnings about macro-in-comment.
! License files (LICENSE.txt and LGPL.txt) are not packaged up; I guess
  the README file that's duplicating native docs can be removed.
! Obsoletes and Provides should be defined for the mingw32- binary subpackage,
  not for the source package.

Comment 7 Erik van Pienbroek 2011-06-02 19:33:52 UTC
Okay, next round:

Spec URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo.spec
SRPM URL: http://ftd4linux.nl/contrib/mingw-libjpeg-turbo-1.1.1-3.fc15.src.rpm

* Thu Jun  2 2011 Erik van Pienbroek <epienbro> - 1.1.1-3
- Moved the obsoletes/provides to the right location
- Bundle the licence and other %doc's
- Fixed a small rpmlint warning

Comment 8 Kalev Lember 2011-06-02 19:40:24 UTC
(In reply to comment #7)
> - Bundle the licence and other %doc's

Not sure if the "other %doc's" add much value to the mingw package. In other mingw packages we often only package up the license files and leave the rest of the documentation out, with the rationale that they duplicate what's in the native package. But I don't care much either way.


Looks good. APPROVED

Comment 9 Erik van Pienbroek 2011-06-02 19:46:07 UTC
I just copied over the %doc line from the native libjpeg-turbo package, so it contains stuff like readme, the changelog and the license. All other documentation was excluded from this package.

Anyway, thanks for reviewing and approving the package!

New Package SCM Request
=======================
Package Name: mingw-libjpeg-turbo
Short Description: MinGW Windows cross compiled Libjpeg-turbo library
Owners: epienbro kalev rjones
Branches: 
InitialCC:

Comment 10 Jason Tibbitts 2011-06-03 00:52:51 UTC
Git done (by process-git-requests).

Comment 11 Erik van Pienbroek 2011-06-10 17:26:20 UTC
Package has been imported and build for rawhide successfully. Closing ticket


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