Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=605373 --- Comment #2 from Kevin Kofler <kevin@xxxxxxxxxxxxxxxx> 2010-06-22 19:48:20 EDT --- 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). -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review