Bug 825557 - Review Request: mingw-clucene - CLucene 2.3.3.4 built for MinGW
Review Request: mingw-clucene - CLucene 2.3.3.4 built for MinGW
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Kalev Lember
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-27 14:55 EDT by greg.hellings
Modified: 2012-11-28 06:53 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-23 02:46:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kalevlember: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description greg.hellings 2012-05-27 14:55:56 EDT
Spec URL: http://dl.thehellings.com/mingw32/clucene/mingw-clucene.spec
SRPM URL: http://dl.thehellings.com/mingw32/clucene/mingw-clucene-2.3.3.4-0.fc17.src.rpm
Description: The CLucene2 text search engine, packaged for MinGW build. Currently built with threading disabled as there seems to be some problems between this package and the version of pthread.h in Fedora's build. I've never successfully built CLucene2 with threading against mingw64, but have had success against mingw32 in the past.

CLucene is a C++ port of the popular Apache Lucene search engine
(http://lucene.apache.org/java). 
CLucene aims to be a high-speed alternative to Java Lucene, its API is very
similar to that of the Java version. CLucene has recently been brought up to
date with Lucene 2.3.2. It contains most of the same functionality as the Java version.

This is my first package submission, and as such I need a sponsor for it.
Fedora Account System Username:
Comment 1 greg.hellings 2012-05-27 15:49:11 EDT
The MinGW pages on the wiki ask for a link to my scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4107116

And to add the appropriate email for the group to the CC, which I'm also doing.
Comment 2 Erik van Pienbroek 2012-05-30 02:24:38 EDT
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}
- Please remove the use of the _cross_pkg_name global
- Please add BuildRequires: mingw32-filesystem >= 95 and BuildRequires: mingw64-filesystem >= 95
- Why are the binary packages named mingw32-clucene-core and mingw-64-clucene-core instead of just mingw32-clucene and mingw64-clucene?
- Please replace the %{description} tags with the complete description. Last time I used this tag it didn't produce the expected result
- Why is multithreading disabled?
- Why do you set the CFLAGS _CL_HAVE_WIN32_THREADS=0 ? Is support for win32 threads disabled by this?
- Why is LUCENE_STATIC_CONSTANT_SYNTAX_EXITCODE set to 1 and the other defines to 0?
- The 'rm -rf $RPM_BUILD_ROOT' can be removed from the %install section as it's not needed any more
- The quotes used in %mingw_make call aren't needed any more and can be removed
- The %defattr tags aren't needed any more with modern RPM and can also be removed

I'm not a Fedora sponsor, so I can't formally approve this package. rwmjones: would you be interested in sponsoring Greg?
Comment 3 greg.hellings 2012-05-31 08:57:05 EDT
(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.
Comment 4 Erik van Pienbroek 2012-06-15 11:58:08 EDT
(In reply to comment #3)
> I have uploaded an updated spec and src.rpm file to
> the same URLs as above if anyone else wants to take a look.

Please bump the release and upload a new .src.rpm every time you make a change. This makes it easier to compare changes between versions

Some additional review comments:
- The CMake argument -DDISABLE_MULTITHREADING=ON and -D_CL_HAVE_WIN32_THREADS=0 are used in both the MINGW32_CMAKE_ARGS and the MINGW64_CMAKE_ARGS. To reduce duplication, you might want to move these to the %mingw_cmake call
- The %clean section can be removed completely
- Why are the files %{mingw32_libdir}/CLuceneConfig.cmake and %{mingw64_libdir}/CLuceneConfig.cmake dropped but not the files %{mingw32_libdir}/CLucene/CLuceneConfig.cmake and %{mingw64_libdir}/CLucene/CLuceneConfig.cmake. Shouldn't these two files be dropped as well?
- The CMake arguments -DLUCENE_SYS_INCLUDES:PATH=%{mingw32_libdir} and -DLUCENE_SYS_INCLUDES:PATH=%{mingw64_libdir} shouldn't be needed for MinGW packages. It looks like the native Fedora CLucene package uses it for multilib support, but as Fedora MinGW doesn't use multilib this CMake argument shouldn't be needed for this package

(In reply to comment #3)
> rwmjones said on
> Freenode he is overburdened with sponsorees already. I'm still looking for a
> package sponsor for this package.

One of the things you could do to attract potential sponsors is by helping out with reviewing other packages so you can demonstrate your understanding of the Fedora Packaging Guidelines. Here are some packages which might be interesting to help reviewing:

https://bugzilla.redhat.com/show_bug.cgi?id=832007 - mingw-webkitgtk3
https://bugzilla.redhat.com/show_bug.cgi?id=830388 - mingw-libarchive
https://bugzilla.redhat.com/show_bug.cgi?id=830387 - mingw-xz
Comment 5 greg.hellings 2012-07-06 15:19:37 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > I have uploaded an updated spec and src.rpm file to
> > the same URLs as above if anyone else wants to take a look.
> 
> Please bump the release and upload a new .src.rpm every time you make a
> change. This makes it easier to compare changes between versions

Noted. I figured the bumps should only happen once an actual build was released. My bad.

> 
> Some additional review comments:
> - The CMake argument -DDISABLE_MULTITHREADING=ON and
> -D_CL_HAVE_WIN32_THREADS=0 are used in both the MINGW32_CMAKE_ARGS and the
> MINGW64_CMAKE_ARGS. To reduce duplication, you might want to move these to
> the %mingw_cmake call

I still hope to fix the issue regarding threads on 32-bit architecture, as I know the 32-bit version can be built with threading support. Hence why I want to keep them separated until I can focus on that.

> - The %clean section can be removed completely

Will do.

> - Why are the files %{mingw32_libdir}/CLuceneConfig.cmake and
> %{mingw64_libdir}/CLuceneConfig.cmake dropped but not the files
> %{mingw32_libdir}/CLucene/CLuceneConfig.cmake and
> %{mingw64_libdir}/CLucene/CLuceneConfig.cmake. Shouldn't these two files be
> dropped as well?

The .cmake files are analagous to .pc files. Since they are supplied by the CLucene team they should be maintained, but they shouldn't be placed in the general %{mingw*_libdir}/ root.

> - The CMake arguments -DLUCENE_SYS_INCLUDES:PATH=%{mingw32_libdir} and
> -DLUCENE_SYS_INCLUDES:PATH=%{mingw64_libdir} shouldn't be needed for MinGW
> packages. It looks like the native Fedora CLucene package uses it for
> multilib support, but as Fedora MinGW doesn't use multilib this CMake
> argument shouldn't be needed for this package

I'll give it a go building without it and see how it works out.
Comment 7 greg.hellings 2012-08-13 12:10:35 EDT
Still no review of my latest submission or a sponsor. Bumping for reminders?
Comment 9 Erik van Pienbroek 2012-08-23 15:55:03 EDT
Hi Greg,

I'm not a sponsor so unfortunately I can't help you with that part. Your best chances to attract potential sponsors is by reviewing other packages so you can demonstrate your skills with RPM packaging and the Fedora Packaging Guidelines.

Here are some open review requests for mingw packages which you might find interesting to help reviewing:

mingw-xz: https://bugzilla.redhat.com/show_bug.cgi?id=830387
mingw-libarchive: https://bugzilla.redhat.com/show_bug.cgi?id=830388
mingw-gmp: https://bugzilla.redhat.com/show_bug.cgi?id=833622
mingw-nettle: https://bugzilla.redhat.com/show_bug.cgi?id=833623
mingw-lcms: https://bugzilla.redhat.com/show_bug.cgi?id=851180
mingw-lcms2: https://bugzilla.redhat.com/show_bug.cgi?id=851189
mingw-poppler: https://bugzilla.redhat.com/show_bug.cgi?id=851292

Once you've helped reviewing packages, feel free to mention the ticket(s) here so potential sponsors can follow your effort and decide to sponsor you


From my point of view, your latest .spec file is okay for introduction in Fedora. Just one minor thing that can be improved is that the BuildRequires: gawk isn't necessary as gawk is already part of a default buildroot. But as I'm no sponsor I can't give you a formal approval for this package. For that you really need a sponsor to step up...
Comment 10 Jason Tibbitts 2012-08-23 19:36:28 EDT
Not the best forum for this query, I know, but Eric, why have you not asked to be a sponsor?  You certainly appear to meet the requirements (maintain three packages, been a packager for six months, done five good package reviews).  https://fedorahosted.org/packager-sponsors/
Comment 11 Erik van Pienbroek 2012-08-24 19:44:47 EDT
(In reply to comment #10)
> Not the best forum for this query, I know, but Eric, why have you not asked
> to be a sponsor?  You certainly appear to meet the requirements (maintain
> three packages, been a packager for six months, done five good package
> reviews).  https://fedorahosted.org/packager-sponsors/

I've been considering to become a sponsor for some time already, but as my spare time is too limited due to my fulltime dayjob (which isn't directly Fedora related) I decided not to apply for becoming a sponsor
Comment 12 Kalev Lember 2012-11-10 16:16:20 EST
Hi Greg,

Like we talked on IRC, I'm now a sponsor and going to help you become an official packager.

The spec file here looks very nice, but I have a question about the package naming, which is currently:

upstream tarball: clucene-core
source package: mingw-clucene
binary packages: mingw32-clucene-core / mingw64-clucene-core

Some of the things are called "clucene", and some "clucene-core". Wouldn't it make sense to stick with one name everywhere? I think it's a bit confusing to have different names for the source and binary packages; in the mingw packaging we've so far tried to keep them the same to keep the packaging simple and avoid confusion.
Comment 13 greg.hellings 2012-11-13 09:59:12 EST
The current naming scheme mimics the one by the native package:

upstream tarball: clucene-core
source package: clucene
binary packages: clucene-core / clucene-core-devel / clucene-contribs-lib

The contribs are part of the clucene-core source tarball. I'm not building them, presently, but nothing prevents me from doing so in the future. I can modify the naming scheme if you want, but I had just copied the scheme used by the native packages.
Comment 14 Kalev Lember 2012-11-13 12:58:29 EST
We normally don't do -devel / library package splits in the MinGW packages; everything is shipped in a single package. So instead of one -devel package and two library package like the native build has, the MinGW build would just ship everything in a single package.

The rationale for this is that these packages are only useful for developers and for development purposes, and as such it doesn't make sense to split them up, if everyone is going to install all the subpackages anyway.


> I can modify the naming scheme if you want, but I had just copied the
> scheme used by the native packages.

Oh this should be your choice, you are the Clucene expert. I don't know enough of the project to make the decision. Just pointing out a possible mistake in the packaging that is difficult to correct afterwards (source package renaming needs a re-review).
Comment 15 greg.hellings 2012-11-16 12:18:09 EST
I've renamed the packages and removed the boost and pthreads dependencies. The library now builds a multi-threaded version.

Spec is at the same URL as above.
SRPM is at http://dl.thehellings.com/mingw32/clucene/mingw-clucene-2.3.3.4-3.fc17.src.rpm
Comment 16 Kalev Lember 2012-11-16 13:26:09 EST
Fedora review of mingw-clucene-2.3.3.4-3.fc17.src.rpm 2012-11-16

+ OK
! needs attention

rpmlint output:

$ rpmlint mingw-clucene-2.3.3.4-3.fc18.src.rpm \
          mingw32-clucene \
          mingw64-clucene \
          mingw32-clucene-debuginfo-2.3.3.4-3.fc18.noarch.rpm \
          mingw64-clucene-debuginfo-2.3.3.4-3.fc18.noarch.rpm
mingw32-clucene.noarch: E: description-line-too-long C date with Lucene 2.3.2. It contains most of the same functionality as the Java version.
mingw32-clucene.noarch: E: incorrect-fsf-address /usr/share/doc/mingw32-clucene-2.3.3.4/LGPL.license
mingw64-clucene.noarch: E: description-line-too-long C date with Lucene 2.3.2. It contains most of the same functionality as the Java version.
mingw64-clucene.noarch: E: incorrect-fsf-address /usr/share/doc/mingw64-clucene-2.3.3.4/LGPL.license
mingw-clucene.src: E: description-line-too-long C date with Lucene 2.3.2. It contains most of the same functionality as the Java version.
mingw-clucene.src:70: W: setup-not-quiet
mingw-clucene.src:15: W: mixed-use-of-spaces-and-tabs (spaces: line 15, tab: line 5)
mingw32-clucene-debuginfo.noarch: E: debuginfo-without-sources
mingw64-clucene-debuginfo.noarch: E: debuginfo-without-sources
5 packages and 0 specfiles checked; 7 errors, 2 warnings.

Can you try to address these warnings where it makes sense, to cut down the noise a bit?

One warning that is likely to remain is the debuginfo-without-sources: this is
just a common warning with all mingw debuginfo packages, nothing wrong with
this particular package.

The other one you shouldn't directly fix is the incorrect-fsf-address: we
aren't supposed to patch license files in downstream packages; instead, if you
want to, you can file a ticket with upstream asking if they can update the
LGPL.license file in the next release.

+ 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 (APACHE.license, COPYING, LGPL.license)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  48d647fbd8ef8889e5a7f422c1bfda94  clucene-core-2.3.3.4.tar.gz
  48d647fbd8ef8889e5a7f422c1bfda94  Download/clucene-core-2.3.3.4.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a locale handling
n/a ldconfig in %post and %postun
! Package bundles copies of system libraries

I can see copies of boost and zlib in the tarball, under src/ext/. Can you
remove these in %prep to make sure the package doesn't use the bundled copies?

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
Not applicable to MinGW packages.
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 must not contain libtool .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Directory ownership sane
+ Filenames are valid UTF-8

Some minor nitpicks about the spec file:

> Summary:        A C++ port of Lucene
Most of the mingw packages in Fedora mention mingw in their summary, often
starting it with "Summary: MinGW ...". Might be nice to follow this convention
here as well.

> Group:          Development/System
The package management tools in Fedora don't make any use of the Group tag, so
if you want to, all 3 Group tags here can be removed.

> Patch52: mingw-clucene-core-2.3.3.4-fix-threads.patch
Can you submit this upstream?

> Provides:       mingw32-clucene = %{version}-%{release}
[...]
> Provides:       mingw64-clucene = %{version}-%{release}
Please remove the these provides, they are now the same as the binary package
names.

> %setup -n %{_pkg_name}-core-%{version}
Should be "%setup -qn ...", like rpmlint noted above.
Comment 17 greg.hellings 2012-11-16 14:22:56 EST
I have addressed all of the above, I believe.

The patch did not originate with me, so I don't want to be responsible for submitting it upstream. The rest of the problems which can be addressed have been.

New SRPM: http://dl.thehellings.com/mingw32/clucene/mingw-clucene-2.3.3.4-4.fc17.src.rpm
Spec still at: http://dl.thehellings.com/mingw32/clucene/mingw-clucene.spec
Comment 18 Kalev Lember 2012-11-16 14:43:16 EST
Looks good. I see you've changed the summary for the source package, please edit the summary of the review ticket as well, so that they match (releng git repo creation tools check this).

APPROVED
Comment 19 Kalev Lember 2012-11-16 15:07:16 EST
I've sponsored you as well, please use your new powers wisely!
Comment 20 greg.hellings 2012-11-16 16:21:34 EST
I've put up a new -5 srpm as above along with an updated .spec file that uses %{summary} in the binary packages.
Comment 21 greg.hellings 2012-11-16 17:13:49 EST
New Package SCM Request
=======================
Package Name: mingw-clucene
Short Description: MinGW build of a C++ port of Lucene
Owners: greghellings
Branches: f17 f18
InitialCC:
Comment 22 Jon Ciesla 2012-11-16 17:27:57 EST
Git done (by process-git-requests).
Comment 23 Fedora Update System 2012-11-17 20:50:20 EST
mingw-clucene-2.3.3.4-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mingw-clucene-2.3.3.4-5.fc17
Comment 24 Fedora Update System 2012-11-17 20:50:37 EST
mingw-clucene-2.3.3.4-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mingw-clucene-2.3.3.4-5.fc18
Comment 25 Fedora Update System 2012-11-18 15:12:52 EST
mingw-clucene-2.3.3.4-5.fc18 has been pushed to the Fedora 18 testing repository.
Comment 26 Fedora Update System 2012-11-23 02:46:16 EST
mingw-clucene-2.3.3.4-5.fc18 has been pushed to the Fedora 18 stable repository.
Comment 27 Fedora Update System 2012-11-28 06:53:05 EST
mingw-clucene-2.3.3.4-5.fc17 has been pushed to the Fedora 17 stable repository.

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