Spec URL: http://people.atrpms.net/~hdegoede/vegastrike-data.spec SRPM URL: http://people.atrpms.net/~hdegoede/vegastrike-data-0.4.3-1.src.rpm Description: Data files for Vega Strike, a GPL 3D OpenGL Action RPG space sim that allows a player to trade and bounty hunt. --- The vegastrike engine review is bug 233782
Here we go; sorry for the lateness of this review. == vegastrike-data 0.4.3-1 == ++ GOOD: * Naming and version/release are good - especially since it's a large data package that can be easily copied in the repository instead of rebuilt per distro. Spec file name is "%{name}.spec" as required. * Game engine/runtime and data packages are separate, with this data-only package being noarch. * License (GPL) is acceptable; and a copy of it is included in the installed documentation (%doc vega-license.txt). * Ownership and permissions of files/directories are sane with no duplicates in the %files listing; and the %defattr line is good. * %changelog section is good. * BuildRoot is properly defined, and cleaned both as the first step in %install and as the only step in %clean. * Dependency list (only the base vegastrike package) is OK. * Successfully builds into binary (noarch) RPMs on both Development and FC-6 (x86_64). * Non-ASCII files are encoded in UTF-8, as required. * Included documentation is good. * Macro and $RPM_* variable usage is consistent. * Package is not relocatable and contains no translations (so %find_lang stuff is not necessary). * Package source matches that of upstream. (The md5sum doesn't match; but a recursive diff between the source checkout as noted in the spec and the unpacked tarball included in your source RPM returns nothing different between the two.) * Files marked as %doc are not required at runtime. ++ BAD: (1) rpmlint complains about several empty files in the source and binary RPMs: I: vegastrike-data checking E: vegastrike-data zero-length /usr/share/vegastrike/units/weapons/weapons.blank E: vegastrike-data zero-length /usr/share/vegastrike/units/factions/factions.template E: vegastrike-data zero-length /usr/share/vegastrike/units/weapons/weapons.template E: vegastrike-data zero-length /usr/share/vegastrike/units/subunits/subunits.blank E: vegastrike-data zero-length /usr/share/vegastrike/units/subunits/subunits.template E: vegastrike-data zero-length /usr/share/vegastrike/units/factions/factions.blank These seem ignorable at first glance though - could you verify this please? (2) As-is, it seems to include its own locally-modified copy of various Python modules (modules/builtin/). A brief perusal of the diff between the included python modules and the system copies of them shows mostly variable renaming and similar generally-insignificant changes. (3) This contains a lot of ISO-8859 text files, as follows. These should be encoded in UTF-8. ./textures/sol2/sources.txt: ISO-8859 text ./accounts/test2.save: ISO-8859 text, with very long lines ./accounts/test1.save: ISO-8859 text, with very long lines ./accounts/default.save: ISO-8859 text, with very long lines ./universe/fgnames/purist.txt: ISO-8859 text ./universe/fgnames/forsaken.txt: ISO-8859 text ./universe/fgnames/LIHW.txt: ISO-8859 text ./universe/fgnames/confed.txt: ISO-8859 text ./universe/fgnames/highborn.txt: ISO-8859 text ./universe/fgnames/shaper.txt: ISO-8859 text ./universe/fgnames/cities.txt: ISO-8859 English text ./universe/fgnames/unadorned.txt: ISO-8859 text ./universe/fgnames/homeland-security.txt: ISO-8859 text ./universe/fgnames/ISO.txt: ISO-8859 text ./universe/fgnames/merchant.txt: ISO-8859 text ./universe/fgnames/andolian.txt: ISO-8859 text (4) The splash_holovid and splash_pacifier animations contain objectionable images (scantily-clad women in rather lude poses). These should probably be removed or replaced with more appropriate content. (5) You make executable every Python file in this which has a shebang. Is this really needed or can the shebang lines be removed instead? (The rest of the scriplets are otherwise sane.)
(In reply to comment #1) > Here we go; sorry for the lateness of this review. > > ++ BAD: > (1) rpmlint complains about several empty files in the source and binary RPMs: > I: vegastrike-data checking > E: vegastrike-data zero-length /usr/share/vegastrike/units/weapons/weapons.blank > E: vegastrike-data zero-length > /usr/share/vegastrike/units/factions/factions.template > E: vegastrike-data zero-length /usr/share/vegastrike/units/weapons/weapons.template > E: vegastrike-data zero-length /usr/share/vegastrike/units/subunits/subunits.blank > E: vegastrike-data zero-length > /usr/share/vegastrike/units/subunits/subunits.template > E: vegastrike-data zero-length /usr/share/vegastrike/units/factions/factions.blank > > These seem ignorable at first glance though - could you verify this please? > Yes, I saw those warnings before submission myself too, but I've deliberately ignored them, as I think these empty files might still be needed / usefull. > (2) As-is, it seems to include its own locally-modified copy of various Python > modules (modules/builtin/). A brief perusal of the diff between the included > python modules and the system copies of them shows mostly variable renaming and > similar generally-insignificant changes. > Good catch, removed. > (3) This contains a lot of ISO-8859 text files, as follows. These should be > encoded in UTF-8. > ./textures/sol2/sources.txt: ISO-8859 text > ./accounts/test2.save: ISO-8859 text, with very long lines > ./accounts/test1.save: ISO-8859 text, with very long lines > ./accounts/default.save: ISO-8859 text, with very long lines > ./universe/fgnames/purist.txt: ISO-8859 text > ./universe/fgnames/forsaken.txt: ISO-8859 text > ./universe/fgnames/LIHW.txt: ISO-8859 text > ./universe/fgnames/confed.txt: ISO-8859 text > ./universe/fgnames/highborn.txt: ISO-8859 text > ./universe/fgnames/shaper.txt: ISO-8859 text > ./universe/fgnames/cities.txt: ISO-8859 English text > ./universe/fgnames/unadorned.txt: ISO-8859 text > ./universe/fgnames/homeland-security.txt: ISO-8859 text > ./universe/fgnames/ISO.txt: ISO-8859 text > ./universe/fgnames/merchant.txt: ISO-8859 text > ./universe/fgnames/andolian.txt: ISO-8859 text > Notice these are data files, not user docs, and I think the game might actually expect / depend on them being ISO-8859, so I've kept these as is. > (4) The splash_holovid and splash_pacifier animations contain objectionable > images (scantily-clad women in rather lude poses). These should probably be > removed or replaced with more appropriate content. > These are just 2 of a long list of in game fake advertisements, which are there to create a certain atmosphere. I personally find the ones about guns and joining the army / the ones promoting militarism much more offensive then the 2 you name. IOW this is pretty subjective. Removing any of them feels like applying censorship to me, and lets please not go there unless things are clearly illegal or really bad taste / inappropriate > (5) You make executable every Python file in this which has a shebang. Is this > really needed or can the shebang lines be removed instead? (The rest of the > scriplets are otherwise sane.) Most of these were in the builtin dir, the remaining 2 are really scripts meant to be executed stand alone, and thus should be executable. New version here: Spec URL: http://people.atrpms.net/~hdegoede/vegastrike-data.spec I only updated the specfile as the sources didn't change and it takes eons to upload it with my link.
(In reply to comment #2) > Yes, I saw those warnings before submission myself too, but I've deliberately > ignored them, as I think these empty files might still be needed / usefull. That's OK, then. > > (2) As-is, it seems to include its own locally-modified copy of various Python > > modules (modules/builtin/). A brief perusal of the diff between the included > > python modules and the system copies of them shows mostly variable renaming and > > similar generally-insignificant changes. > > > > Good catch, removed. Thanks! :] > Notice these are data files, not user docs, and I think the game might actually > expect / depend on them being ISO-8859, so I've kept these as is. That's acceptable, then; though it should be brought to the attention of the upstream devs. UTF-8 is so much nicer than having to worry about codepages and stuff. ;) > These are just 2 of a long list of in game fake advertisements, which are there > to create a certain atmosphere. I personally find the ones about guns and > joining the army / the ones promoting militarism much more offensive then the 2 > you name. IOW this is pretty subjective. Removing any of them feels like > applying censorship to me, and lets please not go there unless things are > clearly illegal or really bad taste / inappropriate I agree. Someone from FESCo would need to make the final yea or nay, though if it came down to it. OK as-is. > Most of these were in the builtin dir, the remaining 2 are really scripts meant > to be executed stand alone, and thus should be executable. > Thanks for the clarification, then. With the fixes applied as you've mentioned, vegastrike-data 0.4.3-2 is APPROVED. > I only updated the specfile as the sources didn't change and it takes eons to > upload it with my link. Highly understandable - it takes eons to download it, even. ^_^ Though that's a problem with most large game-data packages.
New Package CVS Request ======================= Package Name: vegastrike-data Short Description: Data files for Vega Strike Owners: j.w.r.degoede Branches: devel InitialCC: <empty>
Imported, but not build yet, since thas has a runtime Requires on vegastrike itself which isstill awaiting review.
Build now and copied to FC-6, closing.