Spec URL: http://tadej.fedorapeople.org/billiards.spec SRPM URL: http://tadej.fedorapeople.org/billiards-0.4.1-1.fc17.src.rpm Description: Billiards is a free cue sports simulator. It aims for physical accuracy and simplicity and should hopefully be useful for practicing billiards on your own and against your friends when a real pool table is not available. Currently both a pool table and a billiards table (that is with and without pockets) are implemented allowing you to play eightball, nineball and carom billiards games. Fedora Account System Username: tadej Koji scratch build (for F18): http://koji.fedoraproject.org/koji/taskinfo?taskID=5102355 Note: rpmlint gives 2 warnings about spelling errors (which can be ignored) and an error "billiards.x86_64: E: no-binary". This error is a deficiency of rpmlint since the package contains byte-compiled Lua files (files with extension .lc).
I've seen you're mail to the games-list and I'll review this, expect a full review soon.
Hi, Good: - package meets naming guidelines - package meets packaging guidelines - license (GPLv3+ and GFDL) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - .desktop file properly installed Needs work: - rpmlint checks return: billiards.x86_64: E: no-binary billiards-debuginfo.x86_64: E: empty-debuginfo-package This is caused by billiards being written completely in lua, this is fine, but the package should be noarch then. Adding: "BuildArch: noarch" to the specfile fixes this. -%makeinstall should not be used except for broken Makefiles, Use "make install DESTDIR=%{buildroot}" instead. -The info file being gzipped is done by rpmbuild and this may change to another compression format in the future. You should drop the .gz from the scriptlets (install-info will figure it out itself) and the %files entry should be: %{_infodir}/%{name}.info* So a few small things to fix, but looks good overall. Regards, Hans
Hans, thank you for reviewing the package! (In reply to comment #2) > Needs work: > - rpmlint checks return: > billiards.x86_64: E: no-binary > billiards-debuginfo.x86_64: E: empty-debuginfo-package > This is caused by billiards being written completely in lua, this is fine, > but the package should be noarch then. Adding: "BuildArch: noarch" to the > specfile fixes this. The author of Billiards said to me that there is a problem with packaging Billiards as an arch-independent package since it contains byte-compiled Lua files (the .lc files) which are arch-dependent. I've filled a bug report about rpmlint not recognizing Lua byte-files, which has just recently been fixed upstream: https://sourceforge.net/p/rpmlint/code/ci/be327c1b08402115aa050a6d8160bd2d05d5efd2/ Although this brings a new problem (https://bugzilla.redhat.com/show_bug.cgi?id=919869#c1): "...you cannot ship the *.lc files in /usr/share, they need to be somewhere in /usr/lib(64) instead." I don't know what is the best way to solve this? Also, I've disabled the generation of the empty debuginfo package and added a comment explaining that. > -%makeinstall should not be used except for broken Makefiles, Use "make > install DESTDIR=%{buildroot}" instead. I think you misread the .spec file. I used %make_install, which has been approved by the Packaging guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used (last line of the section). > -The info file being gzipped is done by rpmbuild and this may change to > another compression format in the future. You should drop the .gz from the > scriptlets (install-info will figure it out itself) and the %files entry > should be: > %{_infodir}/%{name}.info* Thanks for pointing this out. Fixed. New spec and SRPM files are here: http://tadej.fedorapeople.org/billiards.spec http://tadej.fedorapeople.org/billiards-0.4.1-2.fc17.src.rpm Regards, Tadej
Created attachment 712211 [details] billiards.spec with modifications for moving the lc files to libdir Hi, (In reply to comment #3) > Hans, thank you for reviewing the package! > > (In reply to comment #2) > > Needs work: > > - rpmlint checks return: > > billiards.x86_64: E: no-binary > > billiards-debuginfo.x86_64: E: empty-debuginfo-package > > This is caused by billiards being written completely in lua, this is fine, > > but the package should be noarch then. Adding: "BuildArch: noarch" to the > > specfile fixes this. > > The author of Billiards said to me that there is a problem with packaging > Billiards as an arch-independent package since it contains byte-compiled Lua > files (the .lc files) which are arch-dependent. Ah, I see. > I've filled a bug report about rpmlint not recognizing Lua byte-files, which > has just recently been fixed upstream: > https://sourceforge.net/p/rpmlint/code/ci/ > be327c1b08402115aa050a6d8160bd2d05d5efd2/ Good! > Although this brings a new problem > (https://bugzilla.redhat.com/show_bug.cgi?id=919869#c1): > "...you cannot ship the *.lc files in /usr/share, they need to be somewhere > in /usr/lib(64) instead." > I don't know what is the best way to solve this? Move the files to %{_libdir} is the way to solve this. I've quickly whipped a version of the package with this done. I'll attach the modified spec file and the (small) patch to billiards.in here. > Also, I've disabled the generation of the empty debuginfo package and added > a comment explaining that. Good. > > -%makeinstall should not be used except for broken Makefiles, Use "make > > install DESTDIR=%{buildroot}" instead. > > I think you misread the .spec file. I used %make_install, which has been > approved by the Packaging guidelines: > https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_. > 25makeinstall_macro_should_not_be_used (last line of the section). Ah, I've never seen anyone do this (use %make_install) I did not know there is both a %makeinstall and a %make_install, very confusing whomever came up with that does not get a beer from me the next time I see him. I would still prefer if you switched to "make install DESTDIR=%{buildroot}" since that is much more readable, what our packaging templates use, and thus what most packages use. But you're free the keep things as is. > > -The info file being gzipped is done by rpmbuild and this may change to > > another compression format in the future. You should drop the .gz from the > > scriptlets (install-info will figure it out itself) and the %files entry > > should be: > > %{_infodir}/%{name}.info* > > Thanks for pointing this out. Fixed. > > New spec and SRPM files are here: > http://tadej.fedorapeople.org/billiards.spec > http://tadej.fedorapeople.org/billiards-0.4.1-2.fc17.src.rpm Looks good, but as discussed the lc files should be moved to %{_libdir}. Regards, Hans
Created attachment 712212 [details] billiards.in patch, to make billiards work with an other prefix then techne has I just noticed that techne itself contains lc files under /usr/share too, if those are moved to %{_libdir} too, then I guess that %{_libdir}/techne will be part of the default search path and this patch won't be needed.
(In reply to comment #5) > Created attachment 712212 [details] > billiards.in patch, to make billiards work with an other prefix then techne > has > > I just noticed that techne itself contains lc files under /usr/share too, if > those are moved to %{_libdir} too, then I guess that %{_libdir}/techne will > be part of the default search path and this patch won't be needed. To clarify, I'm not saying that you should modify techne as such before moving forward with this review, using the patch instead is fine with me too. For techne itself an other possible approach is to not byte-compile the kolmogorov file as that seems to be the only lc file in there.
Hi! (In reply to comment #4) > > Move the files to %{_libdir} is the way to solve this. I've quickly whipped > a version > of the package with this done. I'll attach the modified spec file and the > (small) patch to billiards.in here. Thanks! I've used your modified .spec file and the patch to billiards.in. > Ah, I've never seen anyone do this (use %make_install) I did not know there > is both a %makeinstall and a %make_install, very confusing whomever came > up with that does not get a beer from me the next time I see him. > > I would still prefer if you switched to "make install DESTDIR=%{buildroot}" > since that is much more readable, what our packaging templates use, and > thus what most packages use. But you're free the keep things as is. Ok, I understand your arguments and I've switched to "make install DESTDIR=%{buildroot}". When I saw the this change in Packaging Guidelines I thought it was the new and preferred way to go. (In reply to comment #5) > I just noticed that techne itself contains lc files under /usr/share too, if > those are moved to %{_libdir} too, then I guess that %{_libdir}/techne will > be part of the default search path and this patch won't be needed. Ooh, thanks for catching this. Yes, this way, just Techne would need fixing. (In reply to comment #6) > For techne itself an other possible approach is to not byte-compile the > kolmogorov file as that seems to be the only lc file in there. Yes, this is another possible approach. I'll contact the upstream author and suggest this to him. New spec and SRPM files are here: http://tadej.fedorapeople.org/billiards.spec http://tadej.fedorapeople.org/billiards-0.4.1-3.fc17.src.rpm Thanks and best regards, Tadej
Hi, (In reply to comment #7) > New spec and SRPM files are here: > http://tadej.fedorapeople.org/billiards.spec > http://tadej.fedorapeople.org/billiards-0.4.1-3.fc17.src.rpm Looks good -> Approved! Regards, Hans
New Package SCM Request ======================= Package Name: billiards Short Description: A free cue sports simulator Owners: tadej Branches: f17 f18 f19 InitialCC:
Git done (by process-git-requests).
Hans, thank you for reviewing this and helping out!