I'll be happy to review.

Hmmm, I swear I actually submitted it, but here it is again.

No builddep on desktop-file-install breaks mock builds it seems.

Any chance you could you fix that yourself and then continue with the review? I
think more things will come up and I don't want to have a zillion iterations.

I'm thinking that it might not be such a good idea to ship an old beta instead
of a CVS pull.  In fact I'm not sure beta3 is even winnable (since you can't
talk to Hawkwind.)

(In reply to comment #4)
> I'm thinking that it might not be such a good idea to ship an old beta instead
> of a CVS pull.  In fact I'm not sure beta3 is even winnable (since you can't
> talk to Hawkwind.)

I didn't know that. Strange they never did another beta then. I assume the game
is fully playable with the CVS version? (or atleast it is as far as you know it).

I think the main developer got married and ended up without much time.  However,
there is still occasional development and there is talk of another beta.

I used to occasionally follow development of xu4, but I hadn't paid much
attention in some time.  After seeing this ticket I happened to check their
forum and the first thing there is a discussion about the Hawkwind problem.  It
seems it's commonly reported, and the solution is always to get the last daily
snapshot.  Unfortunately those are only for Windows and OSX, but they're just
pulls from CVS.

My understanding is that you can finish the game with the snapshots, but I
haven't tried it.  Perhaps I will, since it's been so long since I last played U4.

Right, I know this may be the first reject in a while so I'm gonna split this
into two parts, jargon and some rationale, 

BUT... although Autodownloader in principal is still okay, on extra
consideration of the PackagingGuidelines and quick discussions with a couple of
other contributors (including other Games SIG people) I don't feel comfortable
setting the flag to +, in actual fact, per PackageReviewProcess
(http://fedoraproject.org/wiki/PackageReviewProcess) I'm closing the bug as
WONTFIX and setting fedora-review as -

Just some quick rationale:
* Although the data files can be freely downloaded, they cannot be freely
distributed per the licensees email
* The agreement to allow distribution via his site only was given in 1997, I
believe that Copyright law gives content 50 years before released out of
copyright (and even then there are ways of extending the claim)
* While per the autodownloader review I'm happy with files been placed in the
users directory but it's the fact that the game won't start without the user
agreeing is in my opinion bad

The next stage in my opinion is to take this up with fedora-devel/maintainers or
even fedora-legal, maybe even FESCo in the end.  I'd be happy to review once
again if/when the bug gets reopened.

If it's any consolation, I have fully checked the package and would have
normally been happy to approve, but legal gets in the way.

Oh, an addition to the rationale:

The data would typically be okay under the binary firmware provision in the
ReviewGuidelines, but the non-redistribute part of the author/distributors
agreement is the gotcha.

(In reply to comment #7)
> Right, I know this may be the first reject in a while so I'm gonna split this
> into two parts, jargon and some rationale, 
> 
> BUT... although Autodownloader in principal is still okay, on extra
> consideration of the PackagingGuidelines and quick discussions with a couple of
> other contributors (including other Games SIG people) I don't feel comfortable
> setting the flag to +

It would have been nice if I was involved in these discussions. Are there any
logs of them, any other reasons mentioned in the discussions not mentioned in
comment #7 ?

If I understand correctly this boils down too:
> * While per the autodownloader review I'm happy with files been placed in the
> users directory but it's the fact that the game won't start without the user
> agreeing is in my opinion bad
> 

The rest I agree with and is why autodownloader was written to begin with. So if
I understand correctly the only real issue is, that the xu4 engine is not usable
without any data files. Don't we ship players / libs for a gazillion other file
formats too, I'm sure most of them are not usefull without actual data files,
and that we do not ship data files in all supported formats. I fail to see the
difference.

For example we also ship libsidplay and the xmms-sid plugin, where as I'm sure
we do not ship any .sid files. Both are useless without any .sid files still we
ship them.

Anyways I'll contact Spot about this, and then escalate as needed.

p.s.

Also I have announced the idea of autodownloader a long time ago, and there
wasn't any real objection (nor any real support), see this thread:
https://www.redhat.com/archives/fedora-extras-list/2006-August/msg00558.html

(In reply to comment #9)
> (In reply to comment #7)
> > Right, I know this may be the first reject in a while so I'm gonna split 
this
> > into two parts, jargon and some rationale, 
> > 
> > BUT... although Autodownloader in principal is still okay, on extra
> > consideration of the PackagingGuidelines and quick discussions with a 
couple of
> > other contributors (including other Games SIG people) I don't feel 
comfortable
> > setting the flag to +
> It would have been nice if I was involved in these discussions. Are there any
> logs of them, any other reasons mentioned in the discussions not mentioned in
> comment #7 ?
The discussions were quick "oh, by the way, whats your opinion on this compared 
with that" (this been the review, that been the guidelines) otherwise.
> If I understand correctly this boils down too:
> > * While per the autodownloader review I'm happy with files been placed in 
the
> > users directory but it's the fact that the game won't start without the user
> > agreeing is in my opinion bad
> > 
> The rest I agree with and is why autodownloader was written to begin with. So 
if
> I understand correctly the only real issue is, that the xu4 engine is not 
usable
> without any data files. Don't we ship players / libs for a gazillion other 
file
> formats too, I'm sure most of them are not usefull without actual data files,
> and that we do not ship data files in all supported formats. I fail to see the
> difference.
> For example we also ship libsidplay and the xmms-sid plugin, where as I'm sure
> we do not ship any .sid files. Both are useless without any .sid files still 
we
> ship them.
I think you'd find the difference is that xu4 cannot START without the files, 
by all means you can start xmms with the -sid plugin, and you can sure build 
programs with libsidplay.
> Anyways I'll contact Spot about this, and then escalate as needed.

As I've stated autodownloader is fine, it can even be used by independant 
packages, but we are talking about xu4 here, and the data files, do not meet 
the packaging guidelines, the game will not start/work at all without them.

Until the guidelines are changed to suit this, or there is a consensus, OR 
someone from fedora-legal states that it's okay, I'm not changing position on 
this one.

(In reply to comment #11)
> I think you'd find the difference is that xu4 cannot START without the files, 
> by all means you can start xmms with the -sid plugin, and you can sure build 
> programs with libsidplay.

well, you cannot start a cmdline sid player either without any sid files, still
I'm sure that that would be fine, but this is pointeless, as said I've asked
Spot who usually takes the decisions about thie kinda licensing related stuff.
(Although there is no licensing problem here, as we are not distributing
anything other then GPL code).

Hmm, I've been thinking and reading about this some more, and can someone please
tell me where in the guidelines it says that an oss program that requires some
content to be usefull, may only be included when that content can be included too?

The only thing close to this I can find is the shareware section of the guidelines:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-9188d8409f09df3a87140acc70b20b5b2d6890b0

Notice that that says:
"In this case, the gamedata files can be packaged and included in Fedora, as
long as the files meet the requirements for binary firmware."

Notice the "can be packaged", nowhere does it say that it must be packaged, or
that opensource engines like doom, without content, are not permissable. If you
think it says so, please quote the part where it does.

The only other sections that comes close is emulators, and that doesn't apply
here, as this is clearly as much an emulator as the doom engine is an emulator.

17:17:33 @     XulChris | so what is the case with u4? it has a binary blob that
is freely available but non-distributable?
17:17:39              G |  However, it is worth noting that some non-executable
content exists that is required to make Open Source applications functional. An
                           example of this would be open sourced game engines,
such as Doom, Heretic, and Descent. These game engines come with freely
                           distributable shareware gamedata files.
17:17:45              G |  In this case, the gamedata files can be packaged and
included in Fedora, as long as the files meet the requirements for binary firmware.
17:18:01 @     XulChris | right but the u4 data doesnt fall into that category AFAIK
17:18:21              G |  problem here, it's not "freely distributable" it's
"freely avaliable if you get it from upstream"
17:18:47 @     XulChris | ok yea
17:19:09              G |  upstream for datafiles is also 10 years old
17:19:17 @     XulChris | so you cannot include the binary blob as a package,
but i see no problem including a downloader to download it from upstream and
                          installing it
17:19:17              G |  so the chances of contacting them are slim i think
17:21:14              G |  right, but god knows what section says that a package
must be usable as is (i think)
17:25:04 @     XulChris | well, there is nothing illegal about distributing GPL
software even if its unusable :)
17:25:42 @     XulChris | if origin systems wants to slap the dmca on redhat im
sure they will just remove the package
17:26:12      fedorared |  can you distribute a link to something you can't get
anywhere else? I think so, but of course IANAL.
17:26:46              G |  I'm gonna post to fedora-legal later tonight I think
17:26:58              G |  is there a tracker bug for pack reviews forwarded to
-legal?
17:27:30 @     XulChris | yes FE-LEGAL i believe, have to check

Here is the log from the Games channel, notice that nobody actually said this
shouldn't be packaged and I actually say "there is nothing illegal about it".


In my opinion, this package is perfectly fine.

oops, I actually didn't mean to reset the flags.  My initial reaction was to
just reset the flags and do the review myself, I forgot to change the flags back
though.  Since they have been changed, I guess I'll go ahead and re-assign it to
me and do the review.

==== MUST FIX ====
- Add Requires: desktop-file-utils

Everything else looks okay.

er that should be BuildRequires, but you know that ;-)

==== SHOULD FIX ====
please also add a comment in spec why you use %{makeinstall}

You know, some time ago I would have objected to this package for precisely the
reason that Nigel did, but then the decision was made to include Codec Buddy,
http://fedoraproject.org/wiki/Releases/FeatureCodecBuddy, which exists only to
download non-free (but zero cost) content in precisely the same manner that
autodownloader+xu4 does.  So now I really don't know where the line should be
drawn.  I suppose if you actually had to go out and buy something in order to
make xu4 useful then that would be unacceptable.

(In reply to comment #19)
> You know, some time ago I would have objected to this package for precisely the
> reason that Nigel did, but then the decision was made to include Codec Buddy,
> http://fedoraproject.org/wiki/Releases/FeatureCodecBuddy, which exists only to
> download non-free (but zero cost) content in precisely the same manner that
> autodownloader+xu4 does.  So now I really don't know where the line should be
> drawn.  I suppose if you actually had to go out and buy something in order to
> make xu4 useful then that would be unacceptable.

Funny that you mention that, actually CodecBuddy is worse, and crosses a line
which I would never cross with any use of autodownloader:
1) It downloads non-free code!, not content but code!
2) It only does that for mp3, for for example divx, it takes you to a place
   where you can buy non free codecs for this from fluendo. So to quote you:
   'I suppose if you actually had to go out and buy something in order to
   make xu4 useful then that would be unacceptable' That actually happens with 
   CodecBuddy! (AFAIK).

Sorry to interrupt. I have not read the previous discussion
in detail, however my recognition is that the problem with
this package is:

* the package xu4 _itself_ has no license problem
* to use xu4 actually, xu4 needs some data and one (who
  uses xu4) has to download the data from some URL
* And the license of the data is in question

Okay?
If so, this is exactly the case of "xtide" package which
I maintain and was reviewed by Patrice Dumas.
If my recognition is correct, please check the comment by Patrice
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211626#c32
and what we actually did in 
/usr/share/doc/xtide-common-2.9.3/README.fedora (in xtide-common
rpm).

(In reply to comment #21)
> Sorry to interrupt. I have not read the previous discussion
> in detail, however my recognition is that the problem with
> this package is:
> 
> * the package xu4 _itself_ has no license problem
> * to use xu4 actually, xu4 needs some data and one (who
>   uses xu4) has to download the data from some URL
> * And the license of the data is in question
> 

Correct, except that the license is not in question, it can be freely and
legally downloaded, but may not be (re)distributed.

> Okay?
> If so, this is exactly the case of "xtide" package which
> I maintain and was reviewed by Patrice Dumas.
> If my recognition is correct, please check the comment by Patrice
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211626#c32
> and what we actually did in 
> /usr/share/doc/xtide-common-2.9.3/README.fedora (in xtide-common
> rpm).

Correct, this is the same. Except I created a python gtk gui app todo the
downloading, which will explain to the user that a download is necessary, why
and asks his permission todo the actual download. You may want to check this
package out. The autodownloader app used is configurable through a config file,
so it may be of use (userfriendlier, works interactively even when launched from
the menu) to the xtide case too.

(In reply to comment #22)
> (In reply to comment #21)
> > Sorry to interrupt. I have not read the previous discussion
> > in detail, however my recognition is that the problem with
> > this package is:
> > 
> > * the package xu4 _itself_ has no license problem
> > * to use xu4 actually, xu4 needs some data and one (who
> >   uses xu4) has to download the data from some URL
> > * And the license of the data is in question
> > 
> 
> Correct, except that the license is not in question, it can be freely and
> legally downloaded, but may not be (re)distributed.

For xtide, the data can be downloaded _freely_, however
(as Patrice and me wrote in README.fedora) 
"their license restricts commercial redistribution and selling"
so we judged that the data part cannot be included into Fedora.

So from your comment this package is exactly the case of
xtide.

> 
> > Okay?
> > If so, this is exactly the case of "xtide" package which
> > I maintain and was reviewed by Patrice Dumas.
> > If my recognition is correct, please check the comment by Patrice
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211626#c32
> > and what we actually did in 
> > /usr/share/doc/xtide-common-2.9.3/README.fedora (in xtide-common
> > rpm).
> 
> Correct, this is the same. Except I created a python gtk gui app todo the
> downloading, 

We simply added a bash shell script to help to download the
data from the URL (/usr/sbin/xtide-get_harmonics-data.sh in
xtide-common) .
IMO this is also the same with xtide.

IN SHORT I see no problem with this package (xu4)
judged from what I read in previous comments.

Hey, folks, I just ran this by FESCo and the general agreement is that
everything's good here.  There might have been issues if Hans hadn't done such a
complete job with autodownloader; it's key for some folks that users are
informed as to what they're downloading and given the option not to do anything.

(In reply to comment #25)
> Hey, folks, I just ran this by FESCo and the general agreement is that
> everything's good here.  There might have been issues if Hans hadn't done such a
> complete job with autodownloader; it's key for some folks that users are
> informed as to what they're downloading and given the option not to do anything.

Thank you FESco! I'm also very happy with the positive words about
autodownloader as that involved quite some work.

(In reply to comment #4)
> I'm thinking that it might not be such a good idea to ship an old beta instead
> of a CVS pull.  In fact I'm not sure beta3 is even winnable (since you can't
> talk to Hawkwind.)

Okay, this new version is based on CVS now

(In reply to comment #16 and #18)
> ==== MUST FIX ====
> - Add Requires: desktop-file-utils
Fixed

> ==== SHOULD FIX ====
> please also add a comment in spec why you use %{makeinstall}
Fixed


New version here:
Spec URL: http://people.atrpms.net/~hdegoede/xu4.spec
SRPM URL: http://people.atrpms.net/~hdegoede/xu4-1.1-0.1.20070510.fc7.src.rpm

Notice that this now has version 1.1, as that is how the CVS tree identifies
itself in the games about box.

==== REVIEW GUIDELINES ====
- rpmlint output clean
- package named according to package naming guidelines
- package meets packaging guidelines
- package licensed with open source compatible license
- license matches actual license
- license file included in %doc
- spec written in American english
- spec file legible
- sources match upstream
# diff -ubBr /home/fedora/rpmbuild/SPECS/u4/ /home/chris/cvs/u4/
- package successfully compiles on x86_64 FC6
- all build dependencies in BR
- no locales
- no shared libraries
- package is not relocatable
- package owns all directories it creates
- directories it does not create are brought in through Requires
- no duplicates in %files
- file permissions set properly
- contains proper %clean
- macro usage consistent
- contains code
- no large documentation
- files in %doc do not affect runtime
- no header files
- no static libraries
- no pkgconfig files
- no library with suffix
- no need for devel subpackage
- no libtool archives
- contains proper .desktop file
- does not own files or directories owned by other packages
- contains proper install
- all filenames utf-8

==== SHOULD FIX ====
- spec comments should show how to check out code from the cvs on or before the
date of cvsdate.  I think cvs ... -D %{cvsdate} might work.

**** APPROVED ****

New Package CVS Request
=======================
Package Name:      xu4
Short Description: Ultima IV recreated
Owners:            j.w.r.degoede
Branches:          FC-6 devel
InitialCC:         <empty>

Thanks for the review!

Imported and build, closing.