Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 178568
Summary: | Review Request: lacewing Asteroid like game with may different ships | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Hans de Goede <hdegoede> | ||||||
Component: | Package Review | Assignee: | Wart <wart> | ||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | chabotc, fedora-extras-list, imlinux | ||||||
Target Milestone: | --- | ||||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
URL: | http://users.olis.net.au/zel/ | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2006-01-31 08:19:37 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: | |||||||||
Bug Depends On: | |||||||||
Bug Blocks: | 163779 | ||||||||
Attachments: |
|
Description
Hans de Goede
2006-01-21 19:31:40 UTC
Couple of things -paths used where macro's should be %{_usr} and %{_bindir} -go ahead and move lacew.cfg to /etc -I would create a different rpm for the lwdata.zip -What license is lacewing.png released under? Where did it come from? -Would it make more sense just use the quote from the webpage as the entire description? -add || : to the end of your post and postun commands. Mike wrote: -paths used where macro's should be %{_usr} and %{_bindir} I could replace /usr with %{prefix} in the make commands, yes, but where do you want to use %{_bindir) ? -go ahead and move lacew.cfg to /etc I don't want to polute /etc with this its not a garbage-bin, I'm working on several packages like this one, which all have a similar scheme. -I would create a different rpm for the lwdata.zip Why, this is the only game which uses it and it won't run without it. -What license is lacewing.png released under? Where did it come from? I created it with gthumb from a bmp in lwdata.zip, license is thus GPL -Would it make more sense just use the quote from the webpage as the entire description? Not in my opinion -add || : to the end of your post and postun commands. Will do. Under post and postun you have : /usr/bin/gtk-update-icon-cache I would change this to %{_bindir}/gtk-update-icon-cache rpmlint output: E: lacewing file-in-usr-marked-as-conffile /usr/share/lacewing/lacew.cfg W: lacewing wrong-file-end-of-line-encoding /usr/share/doc/lacewing-1.10/readme.txt W: lacewing wrong-file-end-of-line-encoding /usr/share/doc/lacewing-1.10/licence.txt You should run %{__sed} -i 's/\r//' on these two files to fix the line endings. (In reply to comment #4) > rpmlint output: > E: lacewing file-in-usr-marked-as-conffile /usr/share/lacewing/lacew.cfg Given that this file basically just provides a set of default options, I would either: (a) not mark it as a config file, or (b) move it to /etc the idea being that /usr should be able to be mounted read-only. So if it's a file that's expected to be edited at some time, it should go in /etc, and if not it can stay in /usr but not be marked %config. (In reply to comment #3) > Under post and postun you have : > > /usr/bin/gtk-update-icon-cache > > I would change this to > > %{_bindir}/gtk-update-icon-cache I would advise against this. I used to do that sort of thing myself but was convinced not to do so. The reason is that by keeping %{_bindir} etc. for use only as targets for installation of files, someone can rebuild the package from the SRPM and specify different destination directories, e.g. $ rpmbuild --rebuild --define '_bindir /myapps' package.spec and get the binaries installed where they want them. If you make changes like the one suggested in comment #3, this will fail unless the package builder has also installed gtk-update-icon-cache in the same place that they want to install this package to. So I'd leave it as /usr/bin/gtk-update-icon-cache I've created a new version with all above comments taken into account: http://home.zonnet.nl/jwrdegoede/lacewing.spec http://home.zonnet.nl/jwrdegoede/lacewing-1.10-2.src.rpm See the changelog in the specfile for all the changes as thunderbird in rawhide currently has broken cut and paste support. Regarding %{_bindir} and comment 5 , I agree with comment 5, the wiki scriplets page however suggests using %{_bindir} so I have done that. One last note if you try to build this on Rawhide, allegro-devel is currently broken on rawhide. It is fixed in CVS but can't be build because of buildsys trouble. So todo a testbuild of this package on rawhide, first checkout allegro from CVS, build that locally and install it. Also note that rawhide mockbuilds will also fail because of this and because rawhide has broken deps internally. (In reply to comment #6) > Regarding %{_bindir} and comment 5 , I agree with comment 5, the wiki scriplets > page however suggests using %{_bindir} so I have done that. Normally I would just fix the wiki scriptlets page but it appears that Ville Skyttä was the person that changed /usr/bin/gtk-update-icon-cache to %{_bindir}/gtk-update-icon-cache there (revision 17), and I respect Ville's opinions so it'd be interesting if he could expand on why he made that change. MUST items: * rpmlint output is clean * package and spec name matches upstream * GPL license valid, matches upstream, license file included * Meets packaging guidelines * Spec file is legible and in Am. English. * Source matches upstream (md5sum ok) * Builds cleanly on FC5 i386 * Valid BR; none are redundant * No lang files; no shlibs. * Package not relocatable * 0wns all directories that it creates * File permissions ok * %clean looks good * code, not content * minimal doc files, do not affect runtime * no -devel package necessary * desktop file installed correctly SHOULD items: * package includes license fie - mock build not tested, but did build fine on FC5. * Package runs and causes loss of productivity. :) MUSTFIX: * typo in the Summary: 'may' -> 'many' * Leave out the phrase "Quoting from the webpage" from the description. It seems excessive. SHOULDFIX (won't block approval): * consider splitting the data files and the program into separate packages. This will allow you to make smaller updates to fix problems with the code without requiring users to download the unchanged data files again (~135k vs. 635k download) Since there have been no addtional comments about the use of %{_bindir} in the pre/post scripts, and since they match the guidelines, I'm willing to leave them as-is. Thanks for reviewing. MUSTFIX: fixed SHOULDFIX: I see your rationale, but make install depends on the data being there, so the only way todo this is make this a subpackage and if I then rebuild it to fix a bug in the engine the sub-package will get a version bump also. Or I should /could change the Makefile. Concidering the amount of work to create a seperate date src.rpm, the little gain (only 0.5 Mb on many Mb's of updates / day) and taking into account that I don't plan todo updates frequently I'll just keep it as one big package for now. I've attached a the new spec and a diff to the previous version, I can't upload because I'm not behind my home PC and my ISP only allows ftp access to my homepage from my home PC (GRRR). <shameless plug to lure you in doing another review) If you like lacewing you might also like the "sequel": https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178625 Created attachment 123858 [details]
New specfile
Created attachment 123859 [details]
diff between last and new specfile
I'll accept your reasoning for not splitting the data and source packages. Changes look good, except for the extra " at the end of %description. Fix that before you check it in. APPROVED Imported & Build. |