Spec URL: http://castopulence.org/rpm/allegro5/0.0.1/allegro5.spec SRPM URL: http://castopulence.org/rpm/allegro5/0.0.1/allegro5-5.0.0-0.fc13.src.rpm Description: Hi! I'm new to packaging RPMs, but here it goes. I've been working for the past couple of weeks to get the latest stable release of the Allegro library packaged up. I've been tracking the project on GitHub: http://github.com/bamccaig/allegro5-rpm The Allegro library is a game programming library (though many in the community use it for all sorts of multimedia applications as well). Version 5 of the library is not backwards compatible with previous versions of the library.
(Forgot to fill in the summary... :( Sorry about that. Fixed)
(In reply to comment #1) > (Forgot to fill in the summary... :( Sorry about that. Fixed) No problem, I'll review this and assuming all goes well eventually sponsor you as discussed by email. Note I'm going on a business trip (leaving tomorrow, back in a week), and I likely won't get around to any in depth review before then. Some initial comment based on a quick look at the spec file. First of all, this looks pretty good for a first package! Other remarks: -Your spec file contents is ordered like this: -main package "tags" (key value pairs) -main package description -prep -build -install -clean -main package files -changelog -sub-package1 tags -sub-package1 desc -sub-package1 files -sub-package2 tags -sub-package2 desc -sub-package2 files This is rather unusual, the usual way to do this is: -main package "tags" (key value pairs) -main package description -sub-package1 tags -sub-package1 desc -sub-package2 tags -sub-package2 desc -prep -build -install -clean -main package files -sub-package1 files -sub-package2 files -changelog Also see for example: /etc/rpmdevtools/spectemplate-lib.spec (yum install rpmdevtools) -Please split the long BuildRequires line to fit into 80 char wide terminals, note you can do this like this: BuildRequires: foo bar BuildRequires: more and stuff -You've created rather a lot of subpackages, this feels very Debian-esque For example I personally would: -put the man pages in the main -devel package -put most of the addons in the main package, some rules of thumb: -does it drag in extra dependencies, or just make the package slightly larger, if no extra deps put it in the main package (I think that this will apply to: addon-color, addon-dialog, addon-main, addon-memfile and addon-primitives) -will it likely be needed in most usage scenarios, if yes put it in the main package. I think this applies to addon-audio, addon-image addon-physfs otoh indeed belongs in a subpackage, not sure about addon-fonts and addon-ttf -All (sub)packages which require other (sub)packages from the same package must do so by the full N-V-R (name version release), ie: Requires: %{name} = %{version}-%{release} -Drop all the Requires on libs, rpm will autogenerate these as soname dependencies. -Some of the -devel subpackage Requires on other -devel subpackages can like be dropped too, as rpm generates automatic cross devel dependencies based on pkgconfig files, see the actually generated dependencies in the build rpm ("rpm -qp --requires foo.rpm") -try running rpmlint on the build rpms, and see if it finds anything useful: rpmlint *.src.rpm *.x86_64.rpm -try running rpmlint on the installed package (does some other checks): rpmlint <installed-package-name> Regards, Hans
OK, thanks, I'll work on those things. :)
Spec URL: http://castopulence.org/rpm/allegro5/5.0.0-2/allegro5.spec SRPM URL: http://castopulence.org/rpm/allegro5/5.0.0-2/allegro5-5.0.0-2.fc13.src.rpm I've tried to address your initial remarks. I guess it's ready for round two. Not necessarily there yet, but we'll see. I reorganized the spec (I agree that it makes a lot more sense this way), split the BuildRequires (it was only so long because I didn't now how to split it xD), added explicit %{version}-%{release} dependencies on subpackages, got rid of all library dependencies (not sure I did this correctly), and fixed up some simple issues that rpmlint caught. To determine which subpackages I would merge into the core package(s) I did the following with the built RPMs: for f in $HOME/rpmbuild/RPMS/i386/*; do rpm -pq --requires $f > `basename $f`.requires; done for f in allegro5-addon-*.requires; do diff -u allegro5-5.0.0-1.fc13.i386.rpm.requires $f | less; done I then examined the differences and only merged the packages that showed external differences. The only exception to that rule was leaving addon-physfs*, even though I didn't notice any differences, per your advice. I suspect that maybe I'm missing some explicit dependencies there (unless the main package somehow needs it too). We'll see... ^^
Errr, merged the packages that *didn't* show external dependency differences. xD
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)
Note: I started a scratch build to check we now have all BuildRequires, see: http://koji.fedoraproject.org/koji/taskinfo?taskID=2893477 Looking at the build log it seems that the dump support was not getting build: http://koji.fedoraproject.org/koji/getfile?taskID=2893478&name=build.log Looking closer a local build had the same issue, and this actually is an issue with the dumb package, so I've fixed the build package and started builds for f14 + f15 + devel, you can find the f14 build (which should work fine on f13) here: http://koji.fedoraproject.org/koji/taskinfo?taskID=2893762
Thanks. :) I think I have those addressed now. I had also noticed the DUMB errors, but since I had installed all dumb packages I wasn't sure what to do. :-X Glad you knew. :) - Copied %defattr line beneath every %files line. - Moved /.so$/ symlinks to devel packages. - Moved %doc to top of main %files and added suggested files. - Replaced Source0 URI with corrected URI. - Added/removed suggested BuildRequires. I didn't have libXpm-devel installed until my RPM failed to build with the new BuildRequires, though I was still able to build before. Was it just excluding features automatically or something? - Added %{_includedir}/allegro5 and %exclude lines instead of %dir. - Added n-v-r devel dependency to subpackage devels. - Empty %doc removed. - Used globs to simplify %files section. In addition to al_*.3* manpages, there are also ALLEGRO_*.3* manpages. Can a glob like that be used for those too or is it necessary to name them each explicitly? An example of such a file is "%{_mandir}/man3/ALLEGRO_AUDIO_DEPTH.3.gz". At the very least I have substituted /.gz$/ with *. - The top of allegro5.cfg says that in UNIX it can be located at /etc/allegro5rc so I've added %install commands to move it there with some guidance from #fedora-devel. We'll see if I did it properly with the next review. :-X As for the libcurl-devel dependency, I borrowed that from one of the upstream developers' Debian-based build tutorial: http://wiki.allegro.cc/index.php?title=Install_Allegro5_From_SVN/Linux/Debian I asked #allegro on freenode today and one of the developers told me that one of the example programs can be made to use libcurl. So it's probably not necessary right now. That made me think that maybe I should create a subpackage for the example programs, but without the source code to examine their only real use is testing that Allegro is installed and working. I guess I could always create the subpackage for the binaries and users interested in the source of the examples could fetch it separately... What do you think? Some of my previous changes were made at home and apparently I forgot to push them to my GitHub repository so I'll wait until I merge those before publishing a new revision. :)
(In reply to comment #8) <snip> > - Added/removed suggested BuildRequires. I didn't have libXpm-devel installed > until my RPM failed to build with the new BuildRequires, though I was still > able to build before. Was it just excluding features automatically or > something? Yes, just like it was automatically excluding dumb support. > - Used globs to simplify %files section. In addition to al_*.3* manpages, there > are also ALLEGRO_*.3* manpages. Can a glob like that be used for those too Yes! > - The top of allegro5.cfg says that in UNIX it can be located at > /etc/allegro5rc so I've added %install commands to move it there with some > guidance from #fedora-devel. We'll see if I did it properly with the next > review. :-X > Sounds good. > As for the libcurl-devel dependency, I borrowed that from one of the upstream > developers' Debian-based build tutorial: > > http://wiki.allegro.cc/index.php?title=Install_Allegro5_From_SVN/Linux/Debian > > I asked #allegro on freenode today and one of the developers told me that one > of the example programs can be made to use libcurl. So it's probably not > necessary right now. Agreed lets drop it. > That made me think that maybe I should create a subpackage > for the example programs, but without the source code to examine their only > real use is testing that Allegro is installed and working. I guess I could > always create the subpackage for the binaries and users interested in the > source of the examples could fetch it separately... What do you think? You could take a shot at it, but creating sub packages for example code often is a pain (because example programs tend to not be made to get installed into an FHS location, so they end up not being able to find their data files, etc.). Not worth the effort IMHO. Talking about "example" programs, are there any tools with allegro5 which we maybe should package (Like the allegro4 grabber program) ? > > Some of my previous changes were made at home and apparently I forgot to push > them to my GitHub repository so I'll wait until I merge those before publishing > a new revision. :) Ok, I'll do another review pass once you've posted your new version.
Spec URL: http://castopulence.org/rpm/allegro5/5.0.0-3/allegro5.spec SRPM URL: http://castopulence.org/rpm/allegro5/5.0.0-3/allegro5-5.0.0-3.fc13.src.rpm As long as I didn't screw up merging I think I should have all existing concerns addressed. The rpmdev-diff looks good to me. I'm rather satisfied with the rpmlint output now too. :) It would be nice if you could do something like %doc(null) or something to suppress warnings in subpackages. The only thing that bothers me now is the "W: unused-direct-shlib-dependency" warnings when tested against the installed packages.
Hi, (In reply to comment #10) > The only thing > that bothers me now is the "W: unused-direct-shlib-dependency" warnings when > tested against the installed packages. Oh, good one I did not do that (run rpmlint on the installed package), I just did this and I saw these too. Looking closer at them they are harmless. I've taken another look at your package and it looks fine now! One remaining issue is that rpmlint now says: allegro5.src:294: W: macro-in-%changelog %doc allegro5.src:299: W: macro-in-%changelog %exclude You can fix this by using %%doc / %%exclude in the changelog in the spec file, which will then expand to %doc / %exclude in the changelog in the actual build rpms. I don't see this as a blocker / a reason to do another release. Please do fix this before importing the package into fedora pkg git (step 2.1.17 of: http://fedoraproject.org/wiki/PackageMaintainers/Join). This package is APPROVED! Please go create an account in the Fedora Account System if you've not done so already, and let me now your fas login and then I'll sponsor you. Regards, Hans p.s. Can you please provide karma to the following updates (after logging in first, create a Fedora Account System account for this if you haven't already): https://admin.fedoraproject.org/updates/dumb-0.9.3-11.fc14 https://admin.fedoraproject.org/updates/dumb-0.9.3-11.fc15 If you provide karma they will go to updates stable, so that the correct version of dump will be in place when you build allegro5 for F-15 / F-14. Do you plan on also building it for F-13 ? (then I need to do a dumb update for that too).
New Package SCM Request ======================= Package Name: allegro5 Short Description: Allegro 5 is a game programming library. Owners: bamccaig Branches: f13 f14 f15 InitialCC: jwrdegoede
Git done (by process-git-requests).
allegro5-5.0.0-3.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/allegro5-5.0.0-3.fc13
allegro5-5.0.0-3.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/allegro5-5.0.0-3.fc14
allegro5-5.0.0-3.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/allegro5-5.0.0-3.fc15
allegro5-5.0.0-3.fc13 has been pushed to the Fedora 13 testing repository.
allegro5-5.0.0-3.fc13 has been pushed to the Fedora 13 stable repository.
allegro5-5.0.0-3.fc14 has been pushed to the Fedora 14 stable repository.
allegro5-5.0.0-3.fc15 has been pushed to the Fedora 15 stable repository.