Bug 787878 - Review Request: mingw-icu - MinGW compilation of International Components for Unicode Tools
Summary: Review Request: mingw-icu - MinGW compilation of International Components for...
Keywords:
Status: CLOSED ERRATA
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:
TreeView+ depends on / blocked
 
Reported: 2012-02-07 00:07 UTC by Paweł Forysiuk
Modified: 2012-02-17 23:48 UTC (History)
4 users (show)

Fixed In Version: mingw-icu-4.8.1.1-2.fc16
Clone Of:
Environment:
Last Closed: 2012-02-17 23:48:43 UTC
Type: ---
Embargoed:
kalevlember: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Paweł Forysiuk 2012-02-07 00:07:34 UTC
Spec URL: http://dl.dropbox.com/u/2448780/mingw32-icu.spec
SRPM URL: http://dl.dropbox.com/u/2448780/mingw32-icu-4.8.1.1-1.fc16.src.rpm
Description:  MinGW compilation of International Components for Unicode Tools

Comment 1 Kalev Lember 2012-02-07 00:21:17 UTC
I believe it would be better to name the source package mingw-icu, like in the example spec file: https://fedoraproject.org/wiki/Packaging/MinGW#Example_Specfile

We are likely to get 64 bit MinGW cross compiler in the future, and naming the source package mingw-icu would help with the transition. That way we'll be able to produce mingw32-icu + mingw64-icu binary packages from a single mingw-icu source package.

Comment 2 Paweł Forysiuk 2012-02-07 21:47:17 UTC
Here are updated files
Spec  URL: http://dl.dropbox.com/u/2448780/mingw-icu.spec
SRPM URL: http://dl.dropbox.com/u/2448780/mingw-icu-4.8.1.1-1.fc16.src.rpm

Comment 3 Michael Cronenworth 2012-02-07 21:58:00 UTC
Hey Paweł nice bug number. :)

This is my unofficial review:
-Bug subject needs to be:
Review Request: mingw-icu - MinGW compilation of International Components for Unicode Tools
-Change mingw32-filesystem to >= 68
-Change your %defines at the top to use %globals such as:
%global __strip %{_mingw32_strip}
%global __objdump %{_mingw32_objdump}
%define __debug_install_post %{_mingw32_debug_install_post}

Everything else looks pretty good, but I might have missed something.

Comment 4 Kalev Lember 2012-02-07 22:20:14 UTC
Michael, thanks for the comments. If you don't mind, I'll assign myself as the official reviewer.


Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3770353

failed with:

i686-pc-mingw32-gcc  -I. -I../i18n   "-DDEFAULT_ICU_PLUGINS=\"/usr/i686-pc-mingw32/sys-root/mingw/lib/icu\" " -DU_ATTRIBUTE_DEPRECATED= -DU_COMMON_IMPLEMENTATION -DU_HAVE_ICUCFG  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions --param=ssp-buffer-size=4 -mms-bitfields -Wall -ansi -pedantic -Wshadow -Wpointer-arith -Wmissing-prototypes -Wwrite-strings -Wno-long-long -mthreads -fvisibility=hidden -c -DPIC  -o putil.o putil.c
In file included from putil.c:65:0:
umutex.h:25:21: fatal error: intrin.h: No such file or directory
compilation terminated.

intrin.h doesn't appear to be available in the mingw.org headers. I know you are using the mingw-w64 testing repo where the header probably exists, but the official Fedora packages are based on the mingw.org toolchain instead and need to build with it.

From a quick look, intrin.h is conditionally included only if U_WINDOWS is defined. One of the patches, icu4c-4_6_1-crossbuild.patch, contains the following hunk that changes the build system to define U_WINDOWS instead of U_MINGW:

--- a/source/configure
+++ b/source/configure
@@ -7736,7 +7736,7 @@
        *-*-hpux*)      platform=U_HPUX ;;
        *-apple-darwin*|*-apple-rhapsody*)      platform=U_DARWIN ;;
        *-*-cygwin*)    platform=U_CYGWIN ;;
-       *-*-mingw*)     platform=U_MINGW ;;
+       *-*-mingw*)     platform=U_WINDOWS ;;
        *-*ibm-openedition*|*-*-os390*) platform=OS390
                        if test "${ICU_ENABLE_ASCII_STRINGS}" != "1"; then
                                ICUDATA_CHAR="e"

Removing this from the patch made it build on koji.

Comment 5 Kalev Lember 2012-02-07 22:41:56 UTC
With the small change to the source package outline in comment #4 to get it
building, rpmlint spews out a huge number of warnings / errors:

rpmlint output:
$ rpmlint mingw-icu-4.8.1.1-1.fc17.noarch.rpm \
          mingw-icu-4.8.1.1-1.fc17.src.rpm \
          mingw32-icu-debuginfo-4.8.1.1-1.fc17.noarch.rpm
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicule.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicuio.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicuuc.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicui18n.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicutu.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicutest.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libicudata.dll.a
mingw-icu.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libiculx.dll.a
mingw-icu.noarch: W: devel-file-in-non-devel-package /usr/i686-pc-mingw32/sys-root/mingw/include/unicode/chariter.h
mingw-icu.noarch: W: devel-file-in-non-devel-package /usr/i686-pc-mingw32/sys-root/mingw/include/unicode/localpointer.h
[snip, some 200 more similar devel-file-in-non-devel-package warnings]
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicule.dll.a
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicuio.dll.a
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicuuc.dll.a
mingw-icu.noarch: W: dangling-relative-symlink /usr/i686-pc-mingw32/sys-root/mingw/lib/icu/pkgdata.inc current/pkgdata.inc
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicui18n.dll.a
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicutu.dll.a
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicutest.dll.a
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libicudata.dll.a
mingw-icu.noarch: E: rpath-in-buildconfig /usr/i686-pc-mingw32/sys-root/mingw/bin/icu-config lines ['100']
mingw-icu.noarch: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libiculx.dll.a
mingw-icu.noarch: W: non-standard-dir-in-usr i686-pc-mingw32

The core issue here is that the binary rpm is named 'mingw-icu', instead of 'mingw32-icu'. With correct naming rpmlint shouldn't output any of the warnings above.

It would appear from the spec that it was the intention to name the binary
package mingw32-icu, but just a small oversight with %files section.

Instead of:
%files
it should be:
%files -n mingw32-icu


mingw-icu.noarch: W: no-documentation

Fedora MinGW packages are supposed to not contain documentation, but they MUST
have license files if the upstream tarballs has them. From a quick look,
upstream tarball has license.html and unicode-license.txt which need to be
packaged as %doc.


mingw-icu.noarch: W: dangling-relative-symlink /usr/i686-pc-mingw32/sys-root/mingw/lib/icu/Makefile.inc current/Makefile.inc

Can probably just remove the dangling /usr/i686-pc-mingw32/sys-root/mingw/lib/icu/Makefile.inc in %install


mingw-icu.src:71: W: configure-without-libdir-spec

This is fine, configure is just used for bootstrapping the mingw build.


mingw-icu.src:78: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 78)

Would be nice to fix this to make rpmlint a bit quieter.


mingw-icu.src: W: invalid-url Source0: http://download.icu-project.org/files/icu4c/${version}/icu4c-4_8_1_1-src.tgz HTTP Error 404: Not Found

Looks like a typo, ${version} -> %{version}


mingw32-icu-debuginfo.noarch: E: debuginfo-without-sources
3 packages and 0 specfiles checked; 10 errors, 205 warnings.

Comment 6 Paweł Forysiuk 2012-02-08 14:50:30 UTC
Michael, Kalev thanks for your input.

Updated files here: 
Spec  URL: http://dl.dropbox.com/u/2448780/mingw-icu.spec
SRPM URL: http://dl.dropbox.com/u/2448780/mingw-icu-4.8.1.1-2.fc16.src.rpm

Comment 7 Kalev Lember 2012-02-08 15:28:14 UTC
Fedora review of mingw-icu-4.8.1.1-2.fc16.src.rpm 2012-02-08

+ OK
! needs attention

rpmlint output:
$ $ rpmlint mingw32-icu \
            mingw32-icu-debuginfo-4.8.1.1-2.fc16.noarch.rpm \
            mingw-icu-4.8.1.1-2.fc16.src.rpm
mingw32-icu.noarch: E: rpath-in-buildconfig /usr/i686-pc-mingw32/sys-root/mingw/bin/icu-config lines ['100']
mingw32-icu-debuginfo.noarch: E: debuginfo-without-sources
mingw-icu.src:72: W: configure-without-libdir-spec
3 packages and 0 specfiles checked; 2 errors, 1 warnings.

+ rpmlint warnings are harmless and can be ignored
+ The package is named according to Fedora MinGW packaging guidelines
+ The spec file name matches the base package 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 contains the license files (license.html, unicode-license.txt)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  ea93970a0275be6b42f56953cd332c17  icu4c-4_8_1_1-src.tgz
  ea93970a0275be6b42f56953cd332c17  Download/icu4c-4_8_1_1-src.tgz
+ 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
n/a 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

Looks very nice overall, considering how difficult ICU is to cross compile.


A remaining issue appears to be with icu-config, which is looking for
nonexisting .lib files:
$ /usr/i686-pc-mingw32/sys-root/mingw/bin/icu-config --cflags
### icu-config: Can't find /usr/i686-pc-mingw32/sys-root/mingw/bin/icuuc48.lib - ICU prefix is wrong.
###      Try the --prefix= option
###      or --detect-prefix
###      (If you want to disable this check, use  the --noverify option)
### icu-config: Exitting.

I think this is caused by the sed snippet: 's,^SO=.*$$,SO="dll",'
which appears to have a typo, should be one $ instead of the two $$ it
currently has. Please fix it before importing the package.

Also, some nitpicking about small issues (not review blockers) that you could
clean up before importing, if you want to:
 - "BuildRequires:  gcc-c++" is not needed, because gcc-c++ is in the default buildroot
 - instead of "%{_mingw32_make} %{?_smp_mflags} || %{_mingw32_make}" I'd personally just use
   "make %{?_smp_mflags}"


APPROVED

Comment 8 Paweł Forysiuk 2012-02-08 16:02:19 UTC
New Package SCM Request
=======================
Package Name: mingw-icu
Short Description: MinGW compilation of International Components for Unicode Tools
Owners: pfor kalev
Branches: f16
InitialCC:

Comment 9 Paweł Forysiuk 2012-02-08 16:12:41 UTC
New Package SCM Request
=======================
Package Name: mingw-icu
Short Description: MinGW compilation of International Components for Unicode
Tools
Owners: pfor kalev
Branches: f16 f17
InitialCC:

Comment 10 Gwyn Ciesla 2012-02-08 16:33:33 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2012-02-08 17:59:08 UTC
mingw-icu-4.8.1.1-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/mingw-icu-4.8.1.1-2.fc16

Comment 12 Fedora Update System 2012-02-10 00:56:42 UTC
mingw-icu-4.8.1.1-2.fc16 has been pushed to the Fedora 16 testing repository.

Comment 13 Fedora Update System 2012-02-17 23:48:43 UTC
mingw-icu-4.8.1.1-2.fc16 has been pushed to the Fedora 16 stable repository.


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