[Bug 825557] Review Request: mingw-clucene - CLucene 2.3.3.4 built for MinGW

[bugzilla at redhat.com]

https://bugzilla.redhat.com/show_bug.cgi?id=825557

--- Comment #3 from greg.hellings at gmail.com ---
(In reply to comment #2)
> The new approved MinGW packaging guidelines can be found at
> https://fedoraproject.org/wiki/PackagingDrafts/MinGWCrossCompiler
> 
> Some initial review comments:
> - Please change %?mingw_package_header to %{?mingw_package_header}

Done.

> - Please remove the use of the _cross_pkg_name global

I changed this to _pkg_name to avoid confusion with the generic cross-compile
system.

> - Please add BuildRequires: mingw32-filesystem >= 95 and BuildRequires:
> mingw64-filesystem >= 95

Done.

> - Why are the binary packages named mingw32-clucene-core and
> mingw-64-clucene-core instead of just mingw32-clucene and mingw64-clucene?

This is in keeping with the standard Fedora packages which are named
"clucene-core" and "clucene-contribs-lib". This package contains only the -core
libraries and not the contribs. Just trying to maintain consistency with the
native packages in Fedora.

> - Please replace the %{description} tags with the complete description. Last
> time I used this tag it didn't produce the expected result

That's a shame. The %{description} usage was copied wholesale from the native
spec file for the existing Fedora packages.

> - Why is multithreading disabled?

The package will not compile in 64-bit with threading enabled, and I ran into
issues with the package's detection of the 32-bit pthread headers. It seems to
be an upstream bug which I'd be happy to look into later, but for the time
being it is (to me) less important than getting the library functioning
properly.

> - Why do you set the CFLAGS _CL_HAVE_WIN32_THREADS=0 ? Is support for win32
> threads disabled by this?

I have found that setting this and the other multi-threading option are
necessary to disable threading in a win32 build.

> - Why is LUCENE_STATIC_CONSTANT_SYNTAX_EXITCODE set to 1 and the other
> defines to 0?

This is inherited from the original SuSE packages that I built my old Ubuntu
mingw clucene package off of. I can only guess it is because this feature is
supported whereas the others are not. CLucene's cross-platform detection does
not seem to be as robust as it ought, so it needs a few of these options
defined manually. Last time I tried building without this option the library
did not build properly.

> - The 'rm -rf $RPM_BUILD_ROOT' can be removed from the %install section as
> it's not needed any more

Done

> - The quotes used in %mingw_make call aren't needed any more and can be
> removed

Done

> - The %defattr tags aren't needed any more with modern RPM and can also be
> removed

Done

> 
> I'm not a Fedora sponsor, so I can't formally approve this package.
> rwmjones: would you be interested in sponsoring Greg?

Thanks for the review. I have uploaded an updated spec and src.rpm file to the
same URLs as above if anyone else wants to take a look. rwmjones said on
Freenode he is overburdened with sponsorees already. I'm still looking for a
package sponsor for this package.

-- 
You are receiving this mail because:
You are on the CC list for the bug.