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=680205 Hans de Goede <hdegoede@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@xxxxxxxxxxxxxxxxx |hdegoede@xxxxxxxxxx Flag| |fedora-review? --- Comment #6 from Hans de Goede <hdegoede@xxxxxxxxxx> 2011-03-08 04:21:53 EST --- Hi Brandon, Thanks for the second revision. It still needs some work, but overall I'm quite pleased with how it looks. So I've done a full review, here are the results: Good: ===== - package meets naming guidelines - package meets packaging guidelines (more or less, see needs work) - license (zlib) OK, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86) - no locales - not relocatable - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file - devel package ok - no .la files - post/postun ldconfig ok - devel requires base package n-v-r Needs work: =========== - rpmlint checks return: allegro5.src:196: E: files-attr-not-set <repeated a gazillion times> This is caused by your %files section for subpackages missing a %defattr line, you need to repeat the %defattr line in all "%files foo" section, as the first line after the "%files foo" header allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_main. allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_font.so allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro.so allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_primitive.so allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_memfile.so allegro5.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_color.so allegro5-addon-acodec.x86_64: W: devel-file-in-non-devel-package /usr/lib64/liballegro_acodec.so <etc> You need to move all the liballegro*.so files (but not the liballegro*.so.5* ones) to their resp. -devel subpackages, the .so symlinks are not needed runtime (they are only for linking (ie building) binaries). <some other warnings which can be ignored> - license text not in %doc !! You do not seem to include any of the docs at all, at a minimum the license text should be added to the %files list for the main package, to do this add a line like this: %doc LICENSE.txt To the main %files section typically the %doc line is the first line of a %files section after the %defattr line. Preferably all non install instruction files should be included, ie: %doc CHANGES-5.0.txt CONTRIBUTORS.txt LICENSE.txt README.txt - Fedora uses a standardized URL for sf.net downloads, please change Source0 to http://downloads.sourceforge.net/alleg/allegro-5.0.0.tar.gz - You are missing BuildRequires for: libXext-devel libXxf86vm-devel libXrandr-devel libXinerama-devel libXpm-devel openal-soft-devel - Unnecessary BuildRequires for (please remove): make gcc These are always installed in the buildroot Also you BuildRequire libcurl-devel, but I don't see cmake checking for that are you sure it is needed? - Unowned directory: /usr/include/allegro5 You should add a %dir %{_includedir}/allegro5 to the "%files devel" section, so that it owns this directory (and it gets removed on uninstall). Note that if you do as I suggest under file list simplification below, this is not needed! The various other -devel packages should also have a Requires: %{name}-devel = %{version}-%{release} To ensure the directory has an owner when they are installed, and their headers are likely using headers from the main -devel, so we need this anyways. - Please drop the empty %doc statement from the "%files devel" section - File list simplification You can use wildcards in %files, so I would use %{_mandir}/man3/al_*.3* To replace all the manpage lines, note the * at the end this is there in case the manpage compression (which is done by rpmbuild) ever changes to for example bz2 You can also include an entire dir, instead of using a separate %dir to own it and the listing all the files separately. For example for the include files I would put the following single line in "%files devel": %{_includedir}/allegro5 This will make it own the dir (so need for the %dir line I gave you above) and all files in it. The all files in it is a problem as you want to have some files only in separate addon_foo-devel packages, you can achieve this by using: %{_includedir}/allegro5 %exclude %{_includedir}/allegro5/allegro_acodec.h %exclude %{_includedir}/allegro5/allegro_audio.h %exclude %{_includedir}/allegro5/allegro_native_dialog.h %exclude %{_includedir}/allegro5/allegro_image.h %exclude %{_includedir}/allegro5/allegro_physfs.h %exclude %{_includedir}/allegro5/allegro_ttf.h - Do something with allegro5.cfg, either include it as %doc (so as example), or install it in /etc (assuming the provided cfg file matches the buildin defaults, and that allegro5 will look for it in /etc) -- 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