Bug 825557
Summary: | Review Request: mingw-clucene - CLucene 2.3.3.4 built for MinGW | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | greg.hellings |
Component: | Package Review | Assignee: | Kalev Lember <kalevlember> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | erik-fedora, fedora-mingw, kalevlember, notting, package-review |
Target Milestone: | --- | Flags: | kalevlember:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-11-23 07:46:13 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
greg.hellings
2012-05-27 18:55:56 UTC
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. 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? (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. (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 (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. http://dl.thehellings.com/mingw32/clucene/mingw-clucene-2.3.3.4-1.fc17.src.rpm http://dl.thehellings.com/mingw32/clucene/mingw-clucene.spec Uploaded new files as per the previous two posts. Still no review of my latest submission or a sponsor. Bumping for reminders? Updated uploads: http://dl.thehellings.com/mingw32/clucene/mingw-clucene-2.3.3.4-2.fc17.src.rpm http://dl.thehellings.com/mingw32/clucene/mingw-clucene.spec Koji results for build http://koji.fedoraproject.org/koji/taskinfo?taskID=4415767 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... 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/ (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 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. 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. 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).
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 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. 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 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 I've sponsored you as well, please use your new powers wisely! I've put up a new -5 srpm as above along with an updated .spec file that uses %{summary} in the binary packages. New Package SCM Request ======================= Package Name: mingw-clucene Short Description: MinGW build of a C++ port of Lucene Owners: greghellings Branches: f17 f18 InitialCC: Git done (by process-git-requests). 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 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 mingw-clucene-2.3.3.4-5.fc18 has been pushed to the Fedora 18 testing repository. mingw-clucene-2.3.3.4-5.fc18 has been pushed to the Fedora 18 stable repository. mingw-clucene-2.3.3.4-5.fc17 has been pushed to the Fedora 17 stable repository. |