Bug 233783 - Review Request: vegastrike-data - Data files for Vega Strike
Review Request: vegastrike-data - Data files for Vega Strike
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Gordon
Fedora Package Reviews List
:
Depends On: 233782
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-24 19:50 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-21 18:22:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
peter: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-03-24 19:50:42 EDT
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
Comment 1 Peter Gordon 2007-04-22 21:36:24 EDT
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.) 
Comment 2 Hans de Goede 2007-04-23 16:41:13 EDT
(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.
Comment 3 Peter Gordon 2007-04-23 21:12:27 EDT
(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. 

Comment 4 Hans de Goede 2007-04-24 02:42:50 EDT
New Package CVS Request
=======================
Package Name:      vegastrike-data
Short Description: Data files for Vega Strike
Owners:            j.w.r.degoede@hhs.nl
Branches:          devel
InitialCC:         <empty>
Comment 5 Hans de Goede 2007-04-27 03:44:54 EDT
Imported, but not build yet, since thas has a runtime Requires on vegastrike
itself which isstill awaiting review.
Comment 6 Hans de Goede 2007-05-21 18:22:46 EDT
Build now and copied to FC-6, closing.

Note You need to log in before you can comment on or make changes to this bug.