Bug 605373
Summary: | Review Request: qgis - A user friendly Open Source Geographic Information System | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Volker Fröhlich <volker27> | ||||
Component: | Package Review | Assignee: | Kevin Kofler <kevin> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | bruno, davejohansen, fedora-package-review, kevin, notting, pahan | ||||
Target Milestone: | --- | Flags: | kevin:
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: | 2010-07-14 15:49:39 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: | |||||||
Attachments: |
|
Description
Volker Fröhlich
2010-06-17 18:57:46 UTC
I'm taking this and I will be sponsoring Volker. So, before I start reviewing the package against the packaging criteria, I'm comparing it with the current (orphaned) qgis.spec, mainly to check for upgrade path issues (and found one of those among other things, see point 3 below). 1. I see you're putting the %description entries all after the %package entries. It's more common to list both %package and %description for each subpackage together like the old specfile did it. (But the way you're putting them is not incorrect, just unusual.) 2. The Summary entries in %package python and %package grass are swapped. Please fix them. 3. You dropped the theme subpackages. If you drop a subpackage from an existing Fedora package, you have to Obsolete it for upgrade paths. Please add: Obsoletes: qgis-theme-classic < 1.1 Obsoletes: qgis-theme-gis < 1.1 Obsoletes: qgis-theme-nkids < 1.1 to the main package. 4. One bizarre thing which was already in the old specfile is that BuildRequires are listed per subpackage. This doesn't make much sense, BuildRequires are per SRPM, not per subpackage. The grass BRs are even listed twice (once in the main package and once with the subpackage). While this is not strictly required, I'd suggest listing all the BuildRequires together. 5. You're setting GRASS_PREFIX twice, once with the snippet copied from the original specfile and once with your GRASS_PREFIX=/usr/lib64/. The last setting prevails, so please remove the unused snippet. And you must fix the hardcoded /usr/lib64/ to %{_libdir} or it won't build on 32-bit. 6. VERBOSE=1 in the make command line (which you added) is not needed because the %cmake macro already sets -DCMAKE_VERBOSE_MAKEFILE=ON, i.e. VERBOSE=1 is the default anyway. 7. You changed the chrpath call to (i) zap the rpath from the libraries and (ii) not zap any rpath from the executables, are you sure this is right? If in doubt, please use chrpath on everything (both the libraries and the executables). But then I'd like to know where the rpath is coming from in the first place. (Have you actually checked whether there's an rpath?) Also try just using %{_cmake_skip_rpath} in the %cmake call and see if that makes the rpaths go away. (It's better to not generate rpaths in the first place than to remove them.) 8. You added an additional chmod +x on libraries, are you sure this (and the one which was already there) is needed? The current CMake should be installing shared libraries with +x permissions already! 9. The devel subpackage doesn't need any ldconfig calls (ldconfig calls are only needed for shared libraries, not devel symlinks to them), please remove the 2 scriptlets you added. 10. Just a hint: instead of: %dir %{_libdir}/%{name} %{_libdir}/%{name}/* you can just list: %{_libdir}/%{name}/ in the file list, it will have the same effect (and yes, %exclude also works on that). Updated upon comments: Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-2.fc12.src.rpm The old spec file is located here: Spec URL: http://geofrogger.net/qgis_review/qgis-1.spec Koji scratch build for F12 successful: http://koji.fedoraproject.org/koji/taskinfo?taskID=2284720 rpmlint output: qgis.i686: W: spelling-error %description -l en_US geotiff -> geotaxis, geotropism, geothermic qgis.i686: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging qgis.i686: W: obsolete-not-provided qgis-theme-classic qgis.i686: W: obsolete-not-provided qgis-theme-gis qgis.i686: W: obsolete-not-provided qgis-theme-nkids qgis.i686: W: shared-lib-calls-exit /usr/lib/libqgis_core.so.1.4.0 exit qgis.src: W: spelling-error %description -l en_US geotiff -> geotaxis, geotropism, geothermic qgis.src: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging qgis.src:233: W: macro-in-%changelog %description qgis.src:233: W: macro-in-%changelog %package qgis.src: W: no-buildroot-tag qgis.x86_64: W: spelling-error %description -l en_US geotiff -> geotaxis, geotropism, geothermic qgis.x86_64: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging qgis.x86_64: W: obsolete-not-provided qgis-theme-classic qgis.x86_64: W: obsolete-not-provided qgis-theme-gis qgis.x86_64: W: obsolete-not-provided qgis-theme-nkids qgis.x86_64: W: shared-lib-calls-exit /usr/lib64/libqgis_core.so.1.4.0 exit.5 * The "spelling-error" warnings are obviously bogus. * shared-lib-calls-exit is an upstream issue, definitely not relevant for a rereview, but it should be brought to upstream's attention (it's a bad idea to call exit in a library). * The obsolete-not-provided warnings are harmless. You could in principle add Provides for the themes which are there now (qgis-theme-classic, qgis-theme-gis and qgis-theme-default), but IMHO that's not needed, because nobody is going to try to yum install a qgis theme if qgis already ships it and no other package is ever going to Require a qgis theme. * no-buildroot-tag is not an issue for a Fedora (as opposed to EPEL) package. So the rpmlint test passes, I'm going through the remainder of the review checklist next. Scratch build for F13 also successful: http://koji.fedoraproject.org/koji/taskinfo?taskID=2284753 A scratch build for Rawhide FAILS due to a Qt-4.7-related issue: http://koji.fedoraproject.org/koji/getfile?taskID=2284767&name=build.log cd /builddir/build/BUILD/qgis-1.4.0/src/core && /usr/bin/c++ -Dqgis_core_EXPORTS -DQT_SVG_LIB -DQT_GUI_LIB -DQT_XML_LIB -DQT_SQL_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_NO_CAST_TO_ASCII -D_LARGE_FILE=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE=1 -DSQLITE_ENABLE_RTREE=1 -D_HAVE_PTHREAD_ -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -fPIC -I/builddir/build/BUILD/qgis-1.4.0/src/core/spatialite/headers/spatialite -I/usr/include/QtSvg -I/usr/include/QtGui -I/usr/include/QtXml -I/usr/include/QtSql -I/usr/include/QtNetwork -I/usr/include/QtCore -I/builddir/build/BUILD/qgis-1.4.0 -I/builddir/build/BUILD/qgis-1.4.0/src/core -I/builddir/build/BUILD/qgis-1.4.0/src/core/composer -I/builddir/build/BUILD/qgis-1.4.0/src/core/pal -I/builddir/build/BUILD/qgis-1.4.0/src/core/raster -I/builddir/build/BUILD/qgis-1.4.0/src/core/renderer -I/builddir/build/BUILD/qgis-1.4.0/src/core/symbology -I/builddir/build/BUILD/qgis-1.4.0/src/core/spatialindex/include -I/builddir/build/BUILD/qgis-1.4.0/src/core/symbology-ng -I/usr/include/gdal -DCORE_EXPORT= -DGUI_EXPORT= -DPYTHON_EXPORT= -DANALYSIS_EXPORT= -o CMakeFiles/qgis_core.dir/qgslabelattributes.cpp.o -c /builddir/build/BUILD/qgis-1.4.0/src/core/qgslabelattributes.cpp /builddir/build/BUILD/qgis-1.4.0/src/core/qgshttptransaction.cpp: In constructor 'QgsHttpTransaction::QgsHttpTransaction(QString, QString, int, QString, QString, QNetworkProxy::ProxyType, QString, QString)': /builddir/build/BUILD/qgis-1.4.0/src/core/qgshttptransaction.cpp:51: error: call of overloaded 'QString(int)' is ambiguous /usr/include/QtCore/qstring.h:427: note: candidates are: QString::QString(const QByteArray&) /usr/include/QtCore/qstring.h:425: note: QString::QString(const char*) /usr/include/QtCore/qstring.h:726: note: QString::QString(const QString&) /usr/include/QtCore/qstring.h:106: note: QString::QString(QChar) /usr/include/QtCore/qstring.h:105: note: QString::QString(const QChar*) /builddir/build/BUILD/qgis-1.4.0/src/core/qgshttptransaction.cpp:51: error: call of overloaded 'QString(int)' is ambiguous /usr/include/QtCore/qstring.h:427: note: candidates are: QString::QString(const QByteArray&) /usr/include/QtCore/qstring.h:425: note: QString::QString(const char*) /usr/include/QtCore/qstring.h:726: note: QString::QString(const QString&) /usr/include/QtCore/qstring.h:106: note: QString::QString(QChar) /usr/include/QtCore/qstring.h:105: note: QString::QString(const QChar*) make[2]: *** [src/core/CMakeFiles/qgis_core.dir/qgshttptransaction.cpp.o] Error 1 At line 51 of qgis-1.4.0/src/core/qgshttptransaction.cpp, you need to (in a patch) change the somestring(0) initializations to somestring(), i.e. drop the 0 in the (0). The reason is that QString((const char *) 0) == QString() == QString::null, but QString(0) is ambiguous in Qt 4.7 because there's now also a QString::QString(const QChar *) constructor (whereas it was being interpreted as QString((const char *) 0) previously). So the redundant 0 should be omitted and the default constructor used instead. (Note that other files might be affected by the same issue, the build stops at the first error.) MUST Items: + rpmlint output OK (see above) + named and versioned according to the Package Naming Guidelines + spec file name matches base package name + complies with all the legal guidelines: ! License: GPLv2+ valid, but the actual license is GPLv2+ with exceptions + No known patent problems + No emulator, no firmware, no binary-only or prebuilt components + COPYING packaged as %doc ! Exception_to_GPL_for_Qt.txt must also be packaged as %doc + source matches upstream: MD5: 47710e7aa14c2a672c7f28457b0c956f SHA1: 105f353f36e9625d1eb15f1a315882f214dbe026 SHA256: bbe07eedec4bda95cca994002941b91fd0e50d5cdee2bf79be0c4f7b930cc7da + builds on at least one arch (F12 Koji scratch build) + no known non-working arches, so no ExcludeArch needed + no missing BuildRequires (builds in mock) ! translations are not properly tagged per language %{_datadir}/%{name}/i18n is not an acceptable way to package translations Please use %dir %{_datadir}/%{name}/i18n/ to own the directory and call %find_lang --with-qt to get the files lang-tagged properly. + ldconfig correctly called in %post and %postun + no duplicated system libraries + package not relocatable ! directory ownership not correct: + doesn't own directories owned by another package ! doesn't own all package-specific directories: %{_datadir}/%{name}/ not owned, please add %dir %{_datadir}/%{name}/ ! doc dir listed twice: %docdir %{_datadir}/%{name}/doc %{_datadir}/%{name}/doc Since those files are needed at runtime, please use just %{_datadir}/%{name}/doc instead. + no other duplicate files in %files + permissions correct, defattr used correctly + macros used where possible + no non-code content + no large documentation files, so no -doc package needed ! %doc files required at runtime: The %docdir marking of %{_datadir}/%{name}/doc is incorrect (see above). + all header files in -devel + no static libraries, so no -static package needed + /usr/lib(64)/*.so symlinks are correctly in -devel + /usr/lib(64)/qgis/*.so plugins (NOT symlinks) are correctly NOT in -devel + -devel requires %{name} = %{version}-%{release} + no .la files + .desktop file for the GUI app qgis present + desktop-file-install is used in %install and the .desktop file passes validation + all filenames are valid UTF-8 + other packaging guidelines: + complies with the FHS + proper changelog, tags, BuildRoot, BuildRequires, Summary ! Description errors: * In %description grass, "with GRASS system." should be "with the GRASS system." * In %description python, "Python integrations" should be "Python integration", and the dot at the end is missing. + no macros in Summary and Description + no non-UTF-8 characters + all relevant documentation included as %doc + RPM_OPT_FLAGS are used (%cmake macro) + debuginfo package is valid + no rpaths + no configuration files, so %config guideline doesn't apply + no init scripts, so init script guideline doesn't apply + no timestamp-clobbering file commands + _smp_mflags used + not a web application, so web application guideline doesn't apply + no conflicts No version-specific MUST items for F13 and F14. Version-specific MUST items for F12: + %clean section present and correct + buildroot is deleted at the beginning of %install SHOULD Items: + license already included upstream + no translations for description and summary provided by upstream + package builds in mock (F12 Koji scratch build) ! package doesn't build in Rawhide (Qt 4.7) * will do functionality test later + scriptlets are sane + subpackages other than -devel also require %{name} = %{version}-%{release} + no .pc files, so "placement of .pc files" is irrelevant + no file dependencies Please fix the following items: MUST Items: * change License to GPLv2+ with exceptions and package Exception_to_GPL_for_Qt.txt as %doc * use %dir %{_datadir}/%{name}/i18n/ and %find_lang --with-qt instead of %{_datadir}/%{name}/i18n/ * add %dir %{_datadir}/%{name}/ * remove %docdir %{_datadir}/%{name}/doc * fix "with GRASS system" to "with the GRASS system" in %description grass * fix "Python integrations" to "Python integration" in %description python * add a dot at the end of %description python SHOULD Items: * get the package to build in Rawhide (I can help if needed, at least by firing up scratch builds, and if we really can't get that to work right now and everything else works fine, we can proceed with the sponsorship process so you can at least fire up scratch builds) Obviously, the Rawhide issue should get fixed before issuing builds for releases, in order to preserve upgrade paths, and if not sorted out in a reasonable time, an FTBFS bug will get filed and you'll be asked to act on it. Updated upon comments, except the Rawhide issue: Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-3.fc12.src.rpm The old spec file is located here: Spec URL: http://geofrogger.net/qgis_review/qgis-2.spec So it looks like all the issues I found are indeed addressed, except the build failure on Rawhide. But is the BuildRequires: gettext you added really needed? Qt doesn't use gettext for translations. Kevin thanks for pointing this out to me. I didn't notice it had a new review request. I'm happy to give ownership to Volker and stay on as a co-maintainer if it passes review. I don't think that gettext applies in this case as the docs for that seemed to refer to a different setup for handling translations. I don't know enough to be sure. I have already noticed a few things. rpath needs to be stripped from qgis_help as well as qgis. lupdate-qt4 needs a target. I used "." and that seemed to act reasonably for the build. /usr/bin/lupdate-qt4 -no-obsolete . -ts i18n/*.ts When I checked the bogus executable flag list, I found that ./src/core/composer/qgscomposershape.h and ./src/core/composer/qgscomposershape.cpp no longer were set as executable. You might want to verify this and remove those two from the chmod list. The user guide no longer is in the source tar file. Instead an English and German version are distributed separately under the GDFL. It would be nice to have those in a -doc subpackage. The original package put doc files in two different directories and that looked wrong to me. I hadn't gotten far enough along to check that. My local test builds have been on F13, but I can do mock f14 builds relatively easily as I have a local rawhide repo. I'll take a look at doing builds with the new stuff in rawhide. The user guide locations are: Source2: http://download.osgeo.org/qgis/doc/manual/qgis-1.4.0_user_guide_en.pdf Source3: http://download.osgeo.org/qgis/doc/manual/qgis-1.4.0_user_guide_de.pdf It looks like in F14 a buildrequires for qt-webkit-devel will be needed. Even with the above buildrequires qgshelpviewer.cpp doesn't build. Though it did help with some things. Created attachment 429326 [details]
Patch to fix most (maybe all) ambiguous references to qstring
I still haven't got a build to finish for F14, but this seems to have taken care of the problem for most if not all cases of ambiguous references to qstring.
It's better to use QString() for the default arguments rather than (const char *) 0 (the default constructor is faster and does the same thing; if you write (const char *) 0 as a default argument, that implicitly calls the QString(const char *) constructor). Volker has been working on a newer specfile, we've been talking to each other on IRC, but I'm waiting for him to upload it. We had the qt4-webkit-devel BR already figured out (it should use the qt4-webkit-devel virtual Provides, which is also provided by qt-devel on Fedora < 14) and he was struggling with the QString(0) issues, so maybe your patch will help. Removed all QString problems for now, but it won't build in Rawhide because of https://bugzilla.redhat.com/show_bug.cgi?id=587707 See also https://bugzilla.redhat.com/show_bug.cgi?id=611487 Killefiz and me will propably take a look into this tonight. Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-4.fc12.src.rpm The old spec file is located here: Spec URL: http://geofrogger.net/qgis_review/qgis-3.spec The specfile looks good, all the issues reported above have been addressed. Now we have a new build failure on Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=2307263 make[2]: *** No rule to make target `python/plugins/fTools/ui_frmAbout.py', needed by `python/plugins/fTools/tools/CMakeFiles/ftools_tools'. Stop. Looks like a parallel make race condition: http://koji.fedoraproject.org/koji/getfile?taskID=2307265&name=build.log Notice how the missing file is generated by: cd /builddir/build/BUILD/qgis-1.4.0/python/plugins/fTools && /usr/bin/pyuic4 /builddir/build/BUILD/qgis-1.4.0/python/plugins/fTools/frmAbout.ui -o /builddir/build/BUILD/qgis-1.4.0/python/plugins/fTools/ui_frmAbout.py after the error message. Removed smp_mflags. Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-5.fc12.src.rpm The old spec file is located here: Spec URL: http://geofrogger.net/qgis_review/qgis-4.spec New Rawhide build failure: [ 45%] Building CXX object src/app/CMakeFiles/qgis.dir/composer/qgscomposer.cpp.o cd /builddir/build/BUILD/qgis-1.4.0/src/app && /usr/bin/c++ -DQT_SVG_LIB -DQT_WEBKIT_LIB -DQT_GUI_LIB -DQT_XML_LIB -DQT_SQL_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_NO_CAST_TO_ASCII -DHAVE_PGCONFIG=1 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/QtSvg -I/usr/include/QtWebKit -I/usr/include/QtGui -I/usr/include/QtXml -I/usr/include/QtSql -I/usr/include/QtNetwork -I/usr/include/QtCore -I/builddir/build/BUILD/qgis-1.4.0 -I/builddir/build/BUILD/qgis-1.4.0/src/app -I/builddir/build/BUILD/qgis-1.4.0/src/app/composer -I/builddir/build/BUILD/qgis-1.4.0/src/app/legend -I/builddir/build/BUILD/qgis-1.4.0/src/app/attributetable -I/builddir/build/BUILD/qgis-1.4.0/src/app/../ui -I/usr/include/QtUiTools -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/composer -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/raster -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/renderer -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/symbology -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/symbology-ng -I/builddir/build/BUILD/qgis-1.4.0/src/app/../gui -I/builddir/build/BUILD/qgis-1.4.0/src/app/../gui/symbology-ng -I/builddir/build/BUILD/qgis-1.4.0/src/app/../plugins -I/builddir/build/BUILD/qgis-1.4.0/src/app/../python -I/usr/include/gdal -I/builddir/build/BUILD/qgis-1.4.0/src/app/../core/spatialite/headers/spatialite -I/builddir/build/BUILD/qgis-1.4.0/src/core/spatialite/headers -DCORE_EXPORT= -DGUI_EXPORT= -DPYTHON_EXPORT= -DANALYSIS_EXPORT= -o CMakeFiles/qgis.dir/composer/qgscomposer.cpp.o -c /builddir/build/BUILD/qgis-1.4.0/src/app/composer/qgscomposer.cpp /builddir/build/BUILD/qgis-1.4.0/src/app/composer/qgscomposer.cpp: In member function 'void QgsComposer::restoreWindowState()': /builddir/build/BUILD/qgis-1.4.0/src/app/composer/qgscomposer.cpp:1033:44: error: cannot call constructor 'QVariant::QVariant' directly /builddir/build/BUILD/qgis-1.4.0/src/app/composer/qgscomposer.cpp:1033:44: error: for a function-style cast, remove the redundant '::QVariant' This one looks like a new error from GCC 4.5, which entered Rawhide very recently. The code should just use QVariant(something), not QVariant::QVariant(something). Patched that. Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-6.fc12.src.rpm The old spec file is located here: Spec URL: http://geofrogger.net/qgis_review/qgis-5.spec Now we get up to 91%, but there's some more QString(0) (Qt 4.7) fun in qgsgrassmapcalc.cpp: http://koji.fedoraproject.org/koji/getfile?taskID=2308289&name=build.log Patched that. Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-7.fc12.src.rpm Successfully builds in Rawhide now. Spec URL: http://geofrogger.net/qgis_review/qgis.spec SRPM URL: http://geofrogger.net/qgis_review/qgis-1.4.0-8.fc12.src.rpm Right: http://koji.fedoraproject.org/koji/taskinfo?taskID=2309653 All the issues found in the initial review (comment #8) have been addressed, no new issues have been introduced. Thus the package is APPROVED. I am now sponsoring Volker Fröhlich (FAS account: volter) into the packager group. Welcome! This was imported and built already. (It's a rereview, so the CVS module already existed.) Package Change Request ====================== Package Name: qgis New Branches: el6 Owners: volter Git done (by process-git-requests). Package Change Request ====================== Package Name: qgis New Branches: epel7 Owners: daveisfera InitialCC: volter Git done (by process-git-requests). |