Bug 820729 - Review Request: mingw-cximage - MinGW Windows CxImage manipulation library
Review Request: mingw-cximage - MinGW Windows CxImage manipulation library
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Erik van Pienbroek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 15:41 EDT by Marc-Andre Lureau
Modified: 2012-07-25 23:55 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-25 23:55:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
erik-fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marc-Andre Lureau 2012-05-10 15:41:50 EDT
Spec URL: http://elmarco.fedorapeople.org/mingw-cximage.spec
SRPM URL: http://elmarco.fedorapeople.org/mingw-cximage-600-3.fc17.src.rpm
Description: 
CxImage is a C++ image processing library. It can load, save, display,
transform images in a very simple and fast way, with transparency, multiple
layers and selections, support for BMP GIF JPG PNG MNG TIF ICO TGA PCX J2K
JBG RAS PNM RAW PSD.
Comment 1 Erik van Pienbroek 2012-05-22 08:28:51 EDT
Taking for review

- The BuildRoot tag, the entire %clean section and the %defattr tags in the %files sections aren't needed any more with modern RPM and can be removed
- Please remove the use of the %{summary} macro and give the -static subpackages a more proper summary

The unpack procedure in the %prep section is a bit odd, but I guess this is caused by the fact that current RPM doesn't support 7zip archives yet.

The %mingw_debug_install_post call in the %install section shouldn't be necessary. As it already contains a FIXME comment I'll try to investigate this issue here
Comment 2 Marc-Andre Lureau 2012-05-22 09:19:07 EDT
(In reply to comment #1)
> Taking for review
> 
> - The BuildRoot tag, the entire %clean section and the %defattr tags in the
> %files sections aren't needed any more with modern RPM and can be removed
> - Please remove the use of the %{summary} macro and give the -static
> subpackages a more proper summary

thanks, done

> The unpack procedure in the %prep section is a bit odd, but I guess this is
> caused by the fact that current RPM doesn't support 7zip archives yet.

Yes, I know cfergeau@redhat.com has been working on a rpm patch.

> The %mingw_debug_install_post call in the %install section shouldn't be
> necessary. As it already contains a FIXME comment I'll try to investigate
> this issue here

As I say in the spec, I have no idea why we need to call it ourself, I would be glad to get some help, as I don't know how to debug this one.
Comment 3 Christophe Fergeau 2012-05-22 09:35:53 EDT
(In reply to comment #2)
> (In reply to comment #1)

> > The unpack procedure in the %prep section is a bit odd, but I guess this is
> > caused by the fact that current RPM doesn't support 7zip archives yet.
> 
> Yes, I know cfergeau@redhat.com has been working on a rpm patch.

It's merged http://rpm.org/gitweb?p=rpm.git;a=commit;h=185596818f763af1249f19161f38134ee93092d2 , I've asked Panu if we could get this backported to f17 rpm
Comment 4 Erik van Pienbroek 2012-05-22 09:46:46 EDT
Please bump the release tag in the .spec file every time you make a change instead of replacing the old version. This makes it harder to compare revisions

I just took a look at the debug issue. It seems to be caused by the fact that some RPM variables aren't set automatically as you don't have a %setup tag in the %prep section. It is possible to use this tag without having it trying to extract source files. You can try to add these lines just after the 7za command:
cd ..
%setup -q -T -D -n %{name}-%{version}

With this change, some of the 'cd' calls in other sections can be removed as they're unneeded

In your original .spec file you tried to install the .dll files to libdir and name it .dll.a, but that isn't going to work. In your latest .spec file you already seem to have resolved this issue and also made it generate an import library which is good. Your original .spec file also missed an %{?mingw_debug_package} line, but you seem to have added this in your latest spec file. Again, please bump the release tag and publish a new .src.rpm every time you make a change
Comment 5 Marc-Andre Lureau 2012-05-22 10:07:27 EDT
(In reply to comment #4)
> Please bump the release tag in the .spec file every time you make a change
> instead of replacing the old version. This makes it harder to compare
> revisions

ok, didn't know about that

> I just took a look at the debug issue. It seems to be caused by the fact
> that some RPM variables aren't set automatically as you don't have a %setup
> tag in the %prep section. It is possible to use this tag without having it
> trying to extract source files. You can try to add these lines just after
> the 7za command:
> cd ..
> %setup -q -T -D -n %{name}-%{version}
> 
> With this change, some of the 'cd' calls in other sections can be removed as
> they're unneeded

Indeed, works, thanks

> 
> In your original .spec file you tried to install the .dll files to libdir
> and name it .dll.a, but that isn't going to work. In your latest .spec file
> you already seem to have resolved this issue and also made it generate an
> import library which is good. Your original .spec file also missed an
> %{?mingw_debug_package} line, but you seem to have added this in your latest
> spec file. Again, please bump the release tag and publish a new .src.rpm
> every time you make a change

hmm, I guess that was an outdated srpm.

Fixed ones uploaded:

http://elmarco.fedorapeople.org/mingw-cximage.spec
http://elmarco.fedorapeople.org/mingw-cximage-600-4.fc17.src.rpm
Comment 6 Erik van Pienbroek 2012-05-28 11:57:36 EDT
$ rpmlint mingw-cximage.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint mingw-cximage-600-4.fc17.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint mingw32-cximage-600-4.fc17.noarch.rpm mingw32-cximage-static-600-4.fc17.noarch.rpm mingw64-cximage-600-4.fc17.noarch.rpm mingw64-cximage-static-600-4.fc17.noarch.rpm
mingw32-cximage.noarch: W: no-documentation
mingw32-cximage-static.noarch: W: no-documentation
mingw64-cximage.noarch: W: no-documentation
mingw64-cximage-static.noarch: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 4 warnings.

$ rpm --query --requires mingw32-cximage
mingw32(gdi32.dll)  
mingw32(kernel32.dll)  
mingw32(libgcc_s_sjlj-1.dll)  
mingw32(libjasper-1.dll)  
mingw32(libjpeg-62.dll)  
mingw32(libpng15-15.dll)  
mingw32(libstdc++-6.dll)  
mingw32(libtiff-3.dll)  
mingw32(msvcrt.dll)  
mingw32(user32.dll)  
mingw32-crt  
mingw32-filesystem >= 83
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

$ rpm --query --requires mingw64-cximage
mingw64(gdi32.dll)  
mingw64(kernel32.dll)  
mingw64(libgcc_s_sjlj-1.dll)  
mingw64(libjasper-1.dll)  
mingw64(libjpeg-62.dll)  
mingw64(libpng15-15.dll)  
mingw64(libstdc++-6.dll)  
mingw64(libtiff-3.dll)  
mingw64(msvcrt.dll)  
mingw64(user32.dll)  
mingw64-crt  
mingw64-filesystem >= 83
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1


$ rpm --query --provides mingw32-cximage
mingw32(libcximage.dll)  
mingw32-cximage = 600-4.fc17

$ rpm --query --provides mingw64-cximage
mingw64(libcximage.dll)  
mingw64-cximage = 600-4.fc17


$ wget --quiet http://sourceforge.net/projects/cximage/files/6.00/cximage600_full.7z -O - | md5sum
7c5141120cc849308f96702ce8e3c0eb  -
$ md5sum cximage600_full.7z 
7c5141120cc849308f96702ce8e3c0eb  cximage600_full.7z


+ OK
! Needs to be looked into
/ Not applicable

[!] Compliant with generic Fedora Packaging Guidelines
[+] Source package name is prefixed with 'mingw-'
[+] Spec file starts with %{?mingw_package_header}
[+] BuildRequires: mingw32-filesystem >= 95 is in the .spec file
[+] BuildRequires: mingw64-filesystem >= 95 is in the .spec file
[+] Spec file contains %package sections for both mingw32 and mingw64 packages
[+] Binary mingw32 and mingw64 packages are noarch
[+] Spec file contains %{?mingw_debug_package} after the %description section
[!] Uses one of the macros %mingw_configure, %mingw_cmake, or %mingw_cmake_kde4
    to configure the package
[!] Uses the macro %mingw_make to build the package
[!] Uses the macro %mingw_make to install the package
[/] If package contains translations, the %mingw_find_lang macro must be used
[+] No binary package named mingw-$pkgname is generated
[+] Libtool .la files are not bundled
[+] .def files are not bundled
[+] Man pages which duplicate native package are not bundled
[+] Info files which duplicate native package are not bundled
[+] Provides of the binary mingw32 and mingw64 packages are equal
[+] Requires of the binary mingw32 and mingw64 packages are equal

The license isn't bundled with this package yet. Please include this (CxImage/license.txt) and mark this as %doc before importing the package in Fedora
 
The .spec file currently doesn't use one of the RPM macros %mingw_configure, %mingw_cmake, %mingw_cmake_kde4 or %mingw_make.
As the upstream package only provides MSVC based project files and the library itself is quite small it's okay to
use custom gcc calls to build this package

This package is using cximage 6.00 (released in december 2010) while the latest available version is 7.02 (released in february 2011).
You might want to consider to update to this latest version

===================================================
 The package mingw-cximage is APPROVED by epienbro
===================================================
Comment 7 Marc-Andre Lureau 2012-05-28 12:06:46 EDT
New Package SCM Request
=======================
Package Name: mingw-cximage
Short Description: MinGW Windows CxImage manipulation library
Owners: elmarco epienbro cfergeau
Branches: f17
InitialCC:
Comment 8 Gwyn Ciesla 2012-05-29 08:35:33 EDT
Git done (by process-git-requests).

cfergeau not added, not a valid FAS account, please correct and add in
pkgdb.
Comment 9 Fedora Update System 2012-05-29 09:20:46 EDT
mingw-cximage-600-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mingw-cximage-600-5.fc17
Comment 10 Fedora Update System 2012-05-29 12:20:34 EDT
mingw-cximage-600-5.fc17 has been pushed to the Fedora 17 testing repository.
Comment 11 Fedora Update System 2012-06-09 07:02:19 EDT
mingw-cximage-600-6.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mingw-cximage-600-6.fc17
Comment 12 Fedora Update System 2012-07-25 23:55:39 EDT
mingw-cximage-600-6.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

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