Bug 447639
Summary: | Review Request: typespeed - Test your typing speed and get your fingers' CPS | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michel Lind <michel> |
Component: | Package Review | Assignee: | Christoph Wickert <christoph.wickert> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, hdegoede, notting |
Target Milestone: | --- | Flags: | christoph.wickert:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 0.6.4-2.fc9 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-08-01 01:49:06 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: |
Description
Michel Lind
2008-05-20 22:03:16 UTC
Wanna swap reviews? : * elice - Elice is a PureBasic to c++ translator / compiler https://bugzilla.redhat.com/show_bug.cgi?id=448310 * lostlabyrinth - Lost Labyrinth is a coffeebreak dungeon crawling game https://bugzilla.redhat.com/show_bug.cgi?id=448311 * lostlabyrinth-sounds - Lost Labyrinth sounds https://bugzilla.redhat.com/show_bug.cgi?id=448312 * lostlabyrinth-graphics - Lost Labyrinth graphics https://bugzilla.redhat.com/show_bug.cgi?id=448313 * trophy - Car racing game with special features https://bugzilla.redhat.com/show_bug.cgi?id=448422 Since Hans' Reviews are all done I'm going to review this one. Stay tuned. Fix - MUST: rpmlint: rpmlint /var/lib/mock/fedora-rawhide-i386/result/typespeed-* typespeed.i386: E: zero-length /var/games/typespeed.score typespeed.i386: W: file-not-utf8 /usr/share/doc/typespeed-0.6.4/ChangeLog typespeed.i386: E: setgid-binary /usr/bin/typespeed games 02755 typespeed.i386: E: non-standard-executable-perm /usr/bin/typespeed 02755 The errors are due to the games group ownership and are therefor save to ignore but please convert the ChangeLog to UTF-8 with iconv. This should be done together with the sed command in %prep and not during %install OK - MUST: The package is named according to the Package Naming Guidelines OK - MUST: The spec file name matches the base package %{name} OK - MUST: The package meets the Packaging Guidelines OK - MUST: The package is licensed with a Fedora approved license (GPLv2+) OK - MUST: The License field in the package spec file matches the actual license. OK - MUST: The source package includes the text of the license and it is correctly included in %doc OK - MUST: The spec file is written in American English OK - MUST: The spec file is legible OK - MUST: The sources used to build the package match the upstream source by md5 fb55b92ad7e29a1a6a7a3e1ca383d5e2 OK - MUST: The package successfully compiles and builds into binary rpms on i386 OK - MUST: No known ExcludeArch OK - MUST: All build dependencies are listed in BuildRequires OK - MUST: The spec file handles locales properly with the %find_lang macro OK - MUST: The package is not designed to be relocatable OK - MUST: The package owns all directories that it creates OK - MUST: The package does not contain any duplicate files in the %files listing FIX - MUST: Although permissions on files are set properly, the %defattr is wrong. Should be "%defattr(-,root,root,-)" for _all_ files, different permissions should be set with %attr, for example %attr(2755,root,games) %{_bindir}/%{name} %attr(664,root,games) %{_localstatedir}/games/%{name}.score OK - MUST: The package has a %clean section, which contains rm -rf %{buildroot} OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines OK - MUST: The package contains code, no content OK - MUST: No large documentation files for a separate %{name}-doc package OK - MUST: The files included as %doc do not affect the runtime of the application Fix - MUST: The package includes a %{name}.desktop file (although it it no GUI application) which is correctly installed with desktop-file-install. But the Category "Application" is obsolete and should be removed. I also suggest to include typespeed.desktop as a separate file and not to create it in the spec. OK - MUST: The package does not own files or directories already owned by other packages OK - MUST: At the beginning of %install the package runs 'rm -rf %{buildroot}' OK - MUST: All filenames in the package are valid UTF-8 OK - SHOULD: The package builds in mock OK - SHOULD: The package functions as described, but I noticed a small bug: The German words file contains some English words, for example "Equipment". FIX - Minor: %description should be reworked a little: DOS is an abbreviation and should therefor be written in capital letters. The third sentence needs an article at the beginning, "The Idea" instead of "Idea". I suggest you simply use the description from the 'About' paragraph of the homepage: "Typespeed's idea is ripped from ztspeed (a DOS game made by Zorlim). The Idea behind the game is rather easy: type words that are flying by from left to right as fast as you can. If you miss 10 or more words, game is over. You can play typespeed for your own or with a friend using TCP/IPv4." Ping?! ChangeLog and description are no blockers, but the %files section and the desktop file really should be fixed. Updated spec & SRPM: Spec URL: http://salimma.fedorapeople.org/for_review/games/typespeed.spec SRPM URL: http://salimma.fedorapeople.org/for_review/games/typespeed-0.6.4-2.fc9.src.rpm Thanks for the review -- sorry for the delay. The package 9ad47f3cc9af37116e7c4c47e54e55e1 typespeed-0.6.4-2.fc9.src.rpm fixes all blockers: OK - MUST: %files section fixed OK - MUST: desktop file is valid OK - SHOULD: ChangeLog is UTF-8 OK - SHOULD: Description was reworked Two more suggestions: - Please remove INSTALL from %doc. It's generic and not needed when installing from rpm - Consider adding an icon to the desktop file. I suggest ether "keyboard" or simply "terminal" since this is a console application. Anyway: None of these is a blocker, so this package is APPROVED. Keyboard icon sounds perfect -- thanks for the suggestion! New Package CVS Request ======================= Package Name: typespeed Short Description: Test your typing speed and get your fingers' CPS Owners: salimma Branches: F-8 F-9 EL-5 InitialCC: Cvsextras Commits: yes cvs done. typespeed-0.6.4-2.fc9 has been submitted as an update for Fedora 9 typespeed-0.6.4-2.fc8 has been submitted as an update for Fedora 8 typespeed-0.6.4-2.fc9 has been pushed to the Fedora 9 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update typespeed'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-6538 typespeed-0.6.4-2.fc9 has been pushed to the Fedora 9 stable repository. If problems still persist, please make note of it in this bug report. typespeed-0.6.4-2.fc8 has been pushed to the Fedora 8 stable repository. If problems still persist, please make note of it in this bug report. |