Bug 502387 - Review Request: mingw32-hunspell - MinGW Windows spell checker and morphological analyzer library
Review Request: mingw32-hunspell - MinGW Windows spell checker and morphologi...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Thomas Sailer
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 502388
  Show dependency treegraph
 
Reported: 2009-05-24 12:32 EDT by Erik van Pienbroek
Modified: 2009-06-04 17:19 EDT (History)
5 users (show)

See Also:
Fixed In Version: 1.2.8-6.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-04 17:19:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
t.sailer: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Erik van Pienbroek 2009-05-24 12:32:11 EDT
Spec URL: http://www.ftd4linux.nl/contrib/mingw32-hunspell.spec
SRPM URL: http://www.ftd4linux.nl/contrib/mingw32-hunspell-1.2.8-6.fc11.src.rpm
Description:
Hunspell is a spell checker and morphological analyzer library and program
designed for languages with rich morphology and complex word compounding or
character encoding. Hunspell interfaces: Ispell-like terminal interface using
Curses library, Ispell pipe interface, OpenOffice.org UNO module.

This is the MinGW build of hunspell

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

Approved MinGW packaging guidelines are here:
http://fedoraproject.org/wiki/Packaging/MinGW
Comment 1 Thomas Sailer 2009-05-26 06:13:51 EDT
Fedora review
http://www.ftd4linux.nl/contrib/mingw32-libgnurx-2.5.1-1.fc11.src.rpm
2009-05-26

rpmlint output:
$ rpmlint *
mingw32-hunspell.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libparsers.a
mingw32-hunspell-static.noarch: E: arch-independent-package-contains-binary-or-object /usr/i686-pc-mingw32/sys-root/mingw/lib/libhunspell-1.2.a
mingw32-hunspell-static.noarch: W: no-documentation
3 packages and 1 specfiles checked; 2 errors, 1 warnings.

As per Packaging/MinGW, these errors can be ignored.

+ OK
! needs attention


+ rpmlint output
+ Package is named according to Fedora MinGW packaging guidelines
+ Specfile name matches the package base name
+ Package follows the Fedora MinGW packaging guidelines
  version seems to be slightly ahead of native (1.1 vs. 1.0) please try to
  stick to the native version
+ License meets guidelines and is acceptable to Fedora
  LGPLv2+ or GPLv2+ or MPLv1.1
+ License matches the actual package license
+ The package contains the license file (COPYING,COPYING.LGPL,COPYING.MPL)
+ Spec file is written in American English
+ Spec file is legible
  - IMO, you don't need to include the whole history of the native package
    in %changelog, just a reference that it was derived from the native package
    would be sufficient

+ Upstream sources match sources in the srpm
1177af54a09e320d2c24015f29c3a93e  hunspell-1.2.8.tar.gz
1177af54a09e320d2c24015f29c3a93e  x/hunspell-1.2.8.tar.gz

n/a Package builds in mock
n/a ExcludeArch bugs filed
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and
%postun
+ Does not use Prefix: /usr
+ Package owns all directories it creates
+ No duplicate files in %files
+ %files has %defattr
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ Consistent use of macros
+ 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
+ Packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a -devel must require the fully versioned base
n/a Packages should not contain libtool .la files
    Fedora MinGW guidelines allow .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Packages must not own files or directories owned by other packages
+ %install begins with rm -rf $RPM_BUILD_ROOT
+ Filenames must be valid UTF-8

Have you tried to upstream your patches?
Especially hunspell-build-dll.patch seems like very upstream-worthy.
Also, what is the failure mode of AC_FUNC_MALLOC? I guess a bug report with autoconf would be in order.
Comment 2 Erik van Pienbroek 2009-05-26 06:36:02 EDT
(In reply to comment #1)
> Have you tried to upstream your patches?
> Especially hunspell-build-dll.patch seems like very upstream-worthy.

Filed at https://sourceforge.net/tracker/?func=detail&aid=2796772&group_id=143754&atid=756395

> Also, what is the failure mode of AC_FUNC_MALLOC? I guess a bug report with
> autoconf would be in order.  

The AC_FUNC_MALLOC is some kind of fallback for environments which don't have a malloc() function in their libc. If this is the case (apparently it is for MinGW), then a '#define malloc rpl_malloc' is added to config.h.

According to rjones, the rpl_malloc() function is part of gnulib. We don't have gnulib in our MinGW toolchain, so this ends up in a undefined reference (to rpl_malloc). After removing the AC_FUNC_MALLOC from the configure.ac file, the compile completes without problems (and the malloc from the MinGW libraries is used).
Comment 3 Thomas Sailer 2009-05-26 06:56:13 EDT
(In reply to comment #2)

> The AC_FUNC_MALLOC is some kind of fallback for environments which don't have a
> malloc() function in their libc. If this is the case (apparently it is for
> MinGW), then a '#define malloc rpl_malloc' is added to config.h.

I agree we don't want rpl_malloc.

The question is why does it fail to find the malloc in libmsvcrt.a (or newer friends). libmsvcrt.a provides _malloc:

i686-pc-mingw32-nm /usr/i686-pc-mingw32/sys-root/mingw/lib/libmsvcrt.a
dxnmbs00676.o:
00000000 b .bss
00000000 d .data
00000000 i .idata$4
00000000 i .idata$5
00000000 i .idata$6
00000000 i .idata$7
00000000 t .text
         U __head_libmsvcrt_a
00000000 I __imp__malloc
00000000 T _malloc

But that shouldn't hold up packaging.

APPROVED by sailer.
Comment 4 Erik van Pienbroek 2009-05-26 07:24:45 EDT
On the fedora-mingw mailing list Fridrich Strba clarified the AC_FUNC_MALLOC behaviour:

Easy!

The AC_FUNC_MALLOC and AC_FUNC_REALLOC try to execute a program to see
whether malloc(0) will return a NULL pointer. If that is not the case,
it will use the rpl_malloc. Since cross-compiling, binaries cannot be
executed and the test fails even though the malloc and realloc are in MS
C runtime. Those malloc and realloc functions are compliant in the sense
of AC_FUNC_MALLOC and AC_FUNC_REALLOC test, so patching them out is safe
when cross-compiling for win32.

Cheers

Fridrich
Comment 5 Erik van Pienbroek 2009-05-26 07:26:16 EDT
Thomas, thanks for the review!

New Package CVS Request
=======================
Package Name: mingw32-hunspell
Short Description: MinGW Windows spell checker and morphological analyzer library
Owners: epienbro rjones
Branches: F-10 F-11
InitialCC:
Comment 6 Thomas Sailer 2009-05-26 08:09:33 EDT
(In reply to comment #4)

> The AC_FUNC_MALLOC and AC_FUNC_REALLOC try to execute a program to see
> whether malloc(0) will return a NULL pointer. If that is not the case,

So that means these autoconf tests can't be used when crosscompiling to any target, not just mingw. Bad. That should IMO at least be documented (it isn't in autoconf 2.63).

> C runtime. Those malloc and realloc functions are compliant in the sense
> of AC_FUNC_MALLOC and AC_FUNC_REALLOC test, so patching them out is safe
> when cross-compiling for win32.

That's not what I'm worried about. I'm worried about the tediousness to patch these tests out in all packages that use them, vs. fixing the macro in autoconf.
Comment 7 Fridrich Strba 2009-05-26 10:47:01 EDT
OK, I don't see how you want to change that. You need to run a programme to be sure that malloc(0) returns NULL or non-null. You have some other situations where you cannot determine something without actually running a programme, like allignments of types.
The standard way of proceding is either to patch the system for known platform with values that we know to be true (I did this here http://svn.gnome.org/viewvc/evolution-data-server/trunk/configure.in?r1=9959&r2=9961 ) or to pre-populate the compile cache with values for tests that cannot be run. In this case, it is just to pre-fill the cache with: ac_cv_func_malloc_0_nonnull=yes
Comment 8 Jason Tibbitts 2009-05-26 18:48:58 EDT
CVS done.
Comment 9 Fedora Update System 2009-06-02 11:16:20 EDT
mingw32-hunspell-1.2.8-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/mingw32-hunspell-1.2.8-6.fc11
Comment 10 Fedora Update System 2009-06-04 17:18:58 EDT
mingw32-hunspell-1.2.8-6.fc11 has been pushed to the Fedora 11 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.