Bug 800930 - Review Request: redeclipse - Multiplayer FPS game based on Cube2
Review Request: redeclipse - Multiplayer FPS game based on Cube2
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
Depends On: 739313
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-07 09:14 EST by Martin Erik Werner
Modified: 2012-06-30 18:09 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-30 18:09:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Martin Erik Werner 2012-03-07 09:14:52 EST
Spec URL: http://arand.fedorapeople.org/redeclipse.spec
SRPM URL: http://arand.fedorapeople.org/redeclipse-1.2-1.fc17.src.rpm
(mind, ~0.5G)

Description:
Red Eclipse is an actively developed FPS game based on the cube2 engine (of Sauerbraten fame). One of the main developers of sauerbraten are the co-main-developer of RE.

Red Eclipse is currently packaged for Debian (by me), and I figured it would be nice to package it for Fedora as well.

Some comments and questions:
1. Enet 1.3 is not yet in Fedora, I don't intent to upload RE before it is, however I wanted to have the package reviewed still so that other issues may be fixed in the meantime.

2. I am using my own debian packaging (metatdata) tarballs as Source#s both for the exhaustive copyright info, and for patches. Is this ok? should I be separating those files out as Source#s and Patch#s instead?

3. I am ignoring some rpmlint spelling errors in the Summary and description. How important is spelling vs common usage in this case?

Thanks.
Comment 1 Martin Erik Werner 2012-03-07 12:53:06 EST
The rpmlint spell warnings (see 3. above) are, for reference:
[arand@fed rpmbuild]$ rpmlint redeclipse redeclipse-data redeclipse-debuginfo cube2font 
redeclipse.x86_64: W: spelling-error Summary(en_US) Multiplayer -> Multiplier, Multiplexer
redeclipse.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
redeclipse.x86_64: W: spelling-error %description -l en_US gameplay -> game play, game-play, nameplate
redeclipse.x86_64: W: spelling-error %description -l en_US gamemodes -> game modes, game-modes, gametes
redeclipse.x86_64: W: spelling-error %description -l en_US mutators -> commutators, mutilators, commutator
4 packages and 0 specfiles checked; 0 errors, 5 warnings.

I've also done scratch builds on f17 and f18 with koji, see http://koji.fedoraproject.org/koji/tasks?owner=arand&state=all
(They seem to have gone through without a hitch).
Comment 2 Martin Erik Werner 2012-03-07 13:06:40 EST
Er... running it on the srpms as well:
redeclipse.src: W: strange-permission generate-tarball.sh 0775L
redeclipse.src:13: W: macro-in-comment %{name}
redeclipse.src:13: W: macro-in-comment %{name}
redeclipse.src:13: W: macro-in-comment %{version}
redeclipse.src: W: invalid-url Source0: redeclipse-1.2-fs.tar.xz

I've tried to make the repackaging clear by keeping a Source# string as it would be if not repacking, and the generate script should be executable, no?
Comment 3 Brendan Jones 2012-03-07 13:37:36 EST
Great game this one. 

Use %%{name} in your comments to get rid of those errors.

Remove the executable permissions from your flags. USually you put a comment abouce the script "Sources obtained by "sh redeclipse-generate-tarball.sh <revison>"

You should be using upstream sources and patches, or svn/git snapshot if that is more appropriate see http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning
Comment 4 Martin Erik Werner 2012-03-07 15:04:14 EST
Hello, and thanks for reviewing! :)

I'm in the process of re-uploading the files, but the srpm will obviously take a while...

Changed:
1. Macros in the #Source0 comment to "%%"
2. -x and renamed generate-tarball.sh -> redeclipse-generate-tarball.sh
3. Added redeclipse-server package, in order that the server may be installed without pulling in all of redeclipse-data (which are not required for the server).

> You should be using upstream sources and patches, or svn/git snapshot if that
> is more appropriate see
> http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

I'm not quite sure I follow you here, are you saying some of these patches are inappropriate, if so, which?

Or are you saying the patches should be separated out and not extracted from the debian sources? I figured it was the right thing to do since these are patches that won't get applied upstream (I have asked), and since I wrote the patches for Debian initially, it is the upstream for these patches, no?

The icon-fix patch is an exception, that one is applied upstream, the header should indicate that, should I be including this a a separate Patch# instead? (I was pulling it in since it already exists in the debian sources)

I am not using svn snapshots since the SVN version will become (is?) incompatible with the released version. Maybe there would be reason to do a redeclipse-svn package sometime, but at the moment I do not think there is.

Thanks.
Comment 5 Brendan Jones 2012-03-07 16:59:18 EST
(In reply to comment #4)
> Hello, and thanks for reviewing! :)

Hi, as this is your first package I can't sponsor you but I can at least help you get it into shape before someone does. 

> 
> I'm in the process of re-uploading the files, but the srpm will obviously take
> a while...

Don't upload a new SRPM until you've got your sources correct - its too big. Just list them here for now. 
 
> Or are you saying the patches should be separated out and not extracted from
> the debian sources? I figured it was the right thing to do since these are
> patches that won't get applied upstream (I have asked), and since I wrote the
> patches for Debian initially, it is the upstream for these patches, no?

Correct, just include  the unedited source tarball along with the required patches as diffs (%Patch0, %Patch1 .. etc). Part of the review process is to checksum the upstream tarball against what is in the SRPM. Also, don't include any of the debian license files. 

You also shouldn't be building any bundled libraries (enet for example), you'll have to remove those sources in your %prep section and ad a BuildRequires: enet >= 1.3. I've added a blocker on the bug you've raised for this.

> 
> The icon-fix patch is an exception, that one is applied upstream, the header
> should indicate that, should I be including this a a separate Patch# instead?
> (I was pulling it in since it already exists in the debian sources)

Your patches should be split as you deem appropriate. This is a good candidate for a separate patch. If upstream fix/apply something in a subsequent release it is much easier to remove a single %patch, than it is to selectively edit/recreate a large one.



> I am not using svn snapshots since the SVN version will become (is?)
> incompatible with the released version. Maybe there would be reason to do a
> redeclipse-svn package sometime, but at the moment I do not think there is.

No problem

I'll have a closer look at your spec tomorrow.
Comment 6 Martin Erik Werner 2012-03-07 18:30:35 EST
Spec URL: http://arand.fedorapeople.org/2/redeclipse.spec

(In reply to comment #5)
> (In reply to comment #4)
> (...)
> > Or are you saying the patches should be separated out and not extracted from
> > the debian sources? I figured it was the right thing to do since these are
> > patches that won't get applied upstream (I have asked), and since I wrote the
> > patches for Debian initially, it is the upstream for these patches, no?
> 
> Correct, just include  the unedited source tarball along with the required
> patches as diffs (%Patch0, %Patch1 .. etc). Part of the review process is to
> checksum the upstream tarball against what is in the SRPM. Also, don't include
> any of the debian license files.

Upstream tarball has embedded libs without source code (sdl, freetype, ...), hence I take it repacking is required, And I'm taking the opportunity to remove the associated headers for these libs (no need to document a slew of copyrights), along with the osx/win-specific content.
Should any of this be left alone instead?

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios :
"Since this is a multiple licensing scenario, the package must contain a comment explaining the multiple licensing breakdown. The actual implementation of this is left to the maintainer."

Since the license breakdown is humongous, I consider using the Debian copyright files are my best bet.

> 
> You also shouldn't be building any bundled libraries (enet for example), you'll
> have to remove those sources in your %prep section and ad a BuildRequires: enet
> >= 1.3. I've added a blocker on the bug you've raised for this.

Yeah, I'm keeping it for the time being though in order to test builds.

Hmm, maybe it should be the Enet bug at
https://bugzilla.redhat.com/show_bug.cgi?id=739313
..blocking egoboo and redeclipse individually?

> > 
> > The icon-fix patch is an exception, that one is applied upstream, the header
> > should indicate that, should I be including this a a separate Patch# instead?
> > (I was pulling it in since it already exists in the debian sources)
> 
> Your patches should be split as you deem appropriate. This is a good candidate
> for a separate patch. If upstream fix/apply something in a subsequent release
> it is much easier to remove a single %patch, than it is to selectively
> edit/recreate a large one.

But the only thing that would need removing in my case is the %prep line
patch -p1 < debian/patches/backported-fix-icon-sizes.patch
..so it would be really simple to remove it, no?
Comment 7 Brendan Jones 2012-03-07 23:47:39 EST
(In reply to comment #6)
> 
> Upstream tarball has embedded libs without source code (sdl, freetype, ...),
> hence I take it repacking is required, And I'm taking the opportunity to remove
> the associated headers for these libs (no need to document a slew of
> copyrights), along with the osx/win-specific content.
> Should any of this be left alone instead?

Just remove the offending libraries / directories in the %prep section. No need to remove the osx/win stuff as long as you don't build against it, nor include any of the files in your %file section

> 
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
> :
> "Since this is a multiple licensing scenario, the package must contain a
> comment explaining the multiple licensing breakdown. The actual implementation
> of this is left to the maintainer."
> 
> Since the license breakdown is humongous, I consider using the Debian copyright
> files are my best bet.

You need to include all of the upstream license files with a summary. Because the content licenses are different in the data subpackage you can add a separate License tag for this sub-package. You can't include the Debian file as a license as they are not upstream and have no authority here.
> 
> > 
> > You also shouldn't be building any bundled libraries (enet for example), you'll
> > have to remove those sources in your %prep section and ad a BuildRequires: enet
> > >= 1.3. I've added a blocker on the bug you've raised for this.
> 
> Yeah, I'm keeping it for the time being though in order to test builds.
> 
> Hmm, maybe it should be the Enet bug at
> https://bugzilla.redhat.com/show_bug.cgi?id=739313
> ..blocking egoboo and redeclipse individually?

Sure, change blockers/depends as you see fit.
 
> But the only thing that would need removing in my case is the %prep line
> patch -p1 < debian/patches/backported-fix-icon-sizes.patch
> ..so it would be really simple to remove it, no?

Right, but you would have Patch0: redeclipse-1.2-fix-icon-sizes.patch

and in you %prep section:

%patch0 -p1

for example. Do this for all of your patches.
Comment 8 Brendan Jones 2012-03-08 00:04:25 EST
(In reply to comment #7)
> 
> Right, but you would have Patch0: redeclipse-1.2-fix-icon-sizes.patch
> 

Patch0: %{name}-%{version}-fix-icon-sizes.patch

sorry, is better

> and in you %prep section:
> 
> %patch0 -p1
> 
> for example. Do this for all of your patches.
Comment 9 Martin Erik Werner 2012-03-08 10:02:55 EST
(In reply to comment #7)
> (In reply to comment #6)
> > 
> > Upstream tarball has embedded libs without source code (sdl, freetype, ...),
> > hence I take it repacking is required, And I'm taking the opportunity to remove
> > the associated headers for these libs (no need to document a slew of
> > copyrights), along with the osx/win-specific content.
> > Should any of this be left alone instead?
> 
> Just remove the offending libraries / directories in the %prep section. No need
> to remove the osx/win stuff as long as you don't build against it, nor include
> any of the files in your %file section

Ah, I assumed the srpms was required to be "clean" and "open-source" (in addition to distributable)..
Since not, then I agree, no repack needed.

> > 
> > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
> > :
> > "Since this is a multiple licensing scenario, the package must contain a
> > comment explaining the multiple licensing breakdown. The actual implementation
> > of this is left to the maintainer."
> > 
> > Since the license breakdown is humongous, I consider using the Debian copyright
> > files are my best bet.
> 
> You need to include all of the upstream license files with a summary. Because
> the content licenses are different in the data subpackage you can add a
> separate License tag for this sub-package. You can't include the Debian file as
> a license as they are not upstream and have no authority here.

So in effect, you are asking me to disregard the work already done by me in Debian to create a clear license breakdown, and to rewrite this information in a crappy non-standardised format?

I'll see if I can commit this info upstream and and pull that commit blob in as a patch instead. That would make the information "authoritative", right?


I've switched to using Patch#s and skipped the debug flag patch, instead using CXXFLAGS+=-g in the make invocation.

Latest spec URL: http://arand.fedorapeople.org/3/redeclipse.spec
Comment 10 Brendan Jones 2012-03-08 10:23:28 EST
(In reply to comment #9)
> > 
> > You need to include all of the upstream license files with a summary. Because
> > the content licenses are different in the data subpackage you can add a
> > separate License tag for this sub-package. You can't include the Debian file as
> > a license as they are not upstream and have no authority here.
> 
> So in effect, you are asking me to disregard the work already done by me in
> Debian to create a clear license breakdown, and to rewrite this information in
> a crappy non-standardised format?

Here's the guidelines regarding the situation and your options. http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

Apologies, I stand corrected, you can include a file in the %doc section which outlines the breakdown, but you don't need to pull in a debian tarball. Just copy the breakdown into a new file, removing debian references, and include it as a Source and move it into %doc in our install section.

> 
> I'll see if I can commit this info upstream and and pull that commit blob in as
> a patch instead. That would make the information "authoritative", right?
> 
> 
> I've switched to using Patch#s and skipped the debug flag patch, instead using
> CXXFLAGS+=-g in the make invocation.

You should be using CXXFLAGS=%{optflags} 
http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

IF this still results in the use of non standard compiler flags, you may need to sed/patch the makefile in you %prep section.

> 
> Latest spec URL: http://arand.fedorapeople.org/3/redeclipse.spec
Comment 11 Martin Erik Werner 2012-03-08 14:45:43 EST
(In reply to comment #10)
> (In reply to comment #9)
> (...)
> Apologies, I stand corrected, you can include a file in the %doc section which
> outlines the breakdown, but you don't need to pull in a debian tarball. Just
> copy the breakdown into a new file, removing debian references, and include it
> as a Source and move it into %doc in our install section.

Done

> (..)
> You should be using CXXFLAGS=%{optflags} 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
> 
> IF this still results in the use of non standard compiler flags, you may need
> to sed/patch the makefile in you %prep section.

Done

Updated both:
spec URL: http://arand.fedorapeople.org/4/redeclipse.spec
srpm URL: http://arand.fedorapeople.org/4/redeclipse-1.2-1.fc17.src.rpm
Comment 12 Brendan Jones 2012-03-08 16:25:08 EST
You need a %doc entry in the %files section of the main package. Read this: 
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing

In the license.txt upstream has been very clear about the source and the content, your comments before your all license source is enough as long as you make sure the sub-packages require the main package.

And seriously, lose all of the references to debian for your patches unless you are referring to a specific bug report. The why is much more important than the where. Better yet, refer to upstreams response to said bug report (I understand the debian maintainer is upstream). Whatever is going to help maintain the package longterm
Comment 13 Martin Erik Werner 2012-03-08 19:42:06 EST
(In reply to comment #12)
> You need a %doc entry in the %files section of the main package. Read this: 
> http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
> 

Ah, right, but I will then need to duplicate most of the license info in the -data package since it shouldn't depend on the engine package, right?

> In the license.txt upstream has been very clear about the source and the
> content, your comments before your all license source is enough as long as you
> make sure the sub-packages require the main package.

If this is enough, I'd be happy to remove the all-licenses file, however, I get the impression that (paraphrasing license.txt)
"There are a whole bunch of individual licenses (some custom-written), dig through the subfolders in order to find them"
..is not really good enough, or is it?

Note that there are a few home-made licenses there as well, my comments in the spec file only cover those which have a known shortname in Fedora.

> 
> And seriously, lose all of the references to debian for your patches unless you
> are referring to a specific bug report. The why is much more important than the
> where. Better yet, refer to upstreams response to said bug report (I understand
> the debian maintainer is upstream). Whatever is going to help maintain the
> package longterm

The Debian maintainer is me, who is semi-upstream (doc, etc.) as well.

I discussed those changes only via IRC (with main RE devs), so I'm afraid there's no response-reference for them.

I tried to add some my "why" in the patch comments, hope it is reasonable.

I've also tweaked the patch file headers to be a bit more Fedora-centric

spec URL: http://arand.fedorapeople.org/5/redeclipse.spec
Comment 14 Brendan Jones 2012-03-09 03:52:02 EST
Getting there!

When you make changes to your spec its customary to bump the release number and annotate in the %changlog like so:

* Wed Feb 29 2012 Martin Erik Werner <martinerikwerner@gmail.com> 1.2-3
- blah

* Wed Feb 29 2012 Martin Erik Werner <martinerikwerner@gmail.com> 1.2-2
- blah

Also keep you spec file column with under 72 for readability (except where its not feasible for scripts), I'd also suggest having separate lines for each of your BuildRequires

And Its also customary to order your tags like so:

%package -n blah1

%description -n blah1

%package -n blah2

%description -n blah2

You can also drop rm -rf %{buildroot} from your %install section

Change %{_mandir}/man6/%{name}-server.6.gz to %{_mandir}/man6/%{name}-server.6.* to allow for changes in compression format in subsequent releases

Have a look at http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir. Move the executables to %_bindir if these executables are to be run directly by users and not via the main executable.
Comment 15 Martin Erik Werner 2012-03-09 08:39:37 EST
(In reply to comment #14)
> Getting there!
> 
> When you make changes to your spec its customary to bump the release number and
> annotate in the %changlog like so:
> 
> * Wed Feb 29 2012 Martin Erik Werner <martinerikwerner@gmail.com> 1.2-3
> - blah
> 
> * Wed Feb 29 2012 Martin Erik Werner <martinerikwerner@gmail.com> 1.2-2
> - blah

Even during the review procss?
I though that was only for changes that are made between actual Fedora releases?
Or in the case of a first release, major things which differ compared to upstream (i.e. the patches).

> 
> Also keep you spec file column with under 72 for readability (except where its
> not feasible for scripts), I'd also suggest having separate lines for each of
> your BuildRequires

Ok, fixed, I was doing 80, since that was mentioned for %description

> 
> And Its also customary to order your tags like so:
> 
> %package -n blah1
> 
> %description -n blah1
> 
> %package -n blah2
> 
> %description -n blah2

Ok, done

> 
> You can also drop rm -rf %{buildroot} from your %install section
>
 
Ah, true, targeting EPEL 5 might be a bit excessive, done

> Change %{_mandir}/man6/%{name}-server.6.gz to
> %{_mandir}/man6/%{name}-server.6.* to allow for changes in compression format
> in subsequent releases

Done

> 
> Have a look at http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir.
> Move the executables to %_bindir if these executables are to be run directly by
> users and not via the main executable.

The game needs to be executed via wrapper scripts since it looks in the working dir for the data files.
Comment 16 Martin Erik Werner 2012-03-09 10:17:37 EST
- Start doing minor releases + changelog for review
- Some indentation fixes
- Drop "in the -data package" since licesene.txt is included in -server

spec URL: http://arand.fedorapeople.org/6/redeclipse.spec
Comment 17 Brendan Jones 2012-03-11 11:48:03 EDT
(In reply to comment #13)
> (In reply to comment #12)
> > You need a %doc entry in the %files section of the main package. Read this: 
> > http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
> 
> If this is enough, I'd be happy to remove the all-licenses file, however, I get
> the impression that (paraphrasing license.txt)
> "There are a whole bunch of individual licenses (some custom-written), dig
> through the subfolders in order to find them"
> ..is not really good enough, or is it?
> 
> Note that there are a few home-made licenses there as well, my comments in the
> spec file only cover those which have a known shortname in Fedora.
> 

Its really your call, if some of the maps/content etc are packaged under differing licenses from the main package you should consider sub-packaging those files if feasible and they can be contained. It could also set a precedent for future maps

You should also uncomment the requires and remove the bundled enet. Write your spec as if the enet you require is available (looks like someone will be looking at this soon). if you have already an enet spec patch please attach it to the blocker if you haven't already
Comment 18 Martin Erik Werner 2012-03-11 12:34:35 EDT
(In reply to comment #17)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > You need a %doc entry in the %files section of the main package. Read this: 
> > > http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
> > 
> > If this is enough, I'd be happy to remove the all-licenses file, however, I get
> > the impression that (paraphrasing license.txt)
> > "There are a whole bunch of individual licenses (some custom-written), dig
> > through the subfolders in order to find them"
> > ..is not really good enough, or is it?
> > 
> > Note that there are a few home-made licenses there as well, my comments in the
> > spec file only cover those which have a known shortname in Fedora.
> > 
> 
> Its really your call, if some of the maps/content etc are packaged under
> differing licenses from the main package you should consider sub-packaging
> those files if feasible and they can be contained. It could also set a
> precedent for future maps

I will not do this at this point in time at least, I think it would complicate the packaging greatly, and make updating it a chore (content changing -> new packages).
I mean, in that case I would package two images on one package, a sound file in another, one map in one package and the rest in another package, if going just based on licensing...

> 
> You should also uncomment the requires and remove the bundled enet. Write your
> spec as if the enet you require is available (looks like someone will be
> looking at this soon). if you have already an enet spec patch please attach it
> to the blocker if you haven't already

Ok, done

I do have a spec patch:
-Version:        1.2.1
-Release:        3%{?dist}
+Version:        1.3.3
+Release:        1%{?dist}
I figured it was not worth attaching.

spec URL: http://arand.fedorapeople.org/7/redeclipse.spec
Comment 19 Brendan Jones 2012-03-11 12:40:43 EDT
(In reply to comment #18)
> I will not do this at this point in time at least, I think it would complicate
> the packaging greatly, and make updating it a chore (content changing -> new
> packages).
> I mean, in that case I would package two images on one package, a sound file in
> another, one map in one package and the rest in another package, if going just
> based on licensing...
> 
not feasible then

> > 
> > You should also uncomment the requires and remove the bundled enet. Write your
> > spec as if the enet you require is available (looks like someone will be
> > looking at this soon). if you have already an enet spec patch please attach it
> > to the blocker if you haven't already
> 
> Ok, done
> 
> I do have a spec patch:
> -Version:        1.2.1
> -Release:        3%{?dist}
> +Version:        1.3.3
> +Release:        1%{?dist}
> I figured it was not worth attaching.
> 
> spec URL: http://arand.fedorapeople.org/7/redeclipse.spec

ha ha - I see your point
Comment 20 Brendan Jones 2012-03-12 08:28:50 EDT
Once again Martin it seems I have misled you. It looks like some repacking of the source is required (as you quite rightly suspected at the beginning). My apologies

Have a look at this link, I think it explains it really well. Your debian package is doing the same thing as far as I can tell. http://fedoraproject.org/wiki/Packaging:SourceURL#When_Upstream_uses_Prohibited_Code
Comment 21 Martin Erik Werner 2012-03-12 10:23:29 EDT
Yeah, I saw the discussion on the -devel list, and I was the one who pointed out the fonts in the new egoboo being freeware-ish over at https://bugzilla.redhat.com/show_bug.cgi?id=799778 ;)

But I'm wondering to what extent does this apply?
The wiki article only mentions if the content is legally non-*distributable* as far as I can tell, and I get the impression that *freeware* (or e.g. CC-BY-NC) would not fall under the prohibited code umbrella?

In addition, there is this notice regarding game *data* http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Shareware

In general the documentation is really flaky regarding the separation of the source rpm and the binary rpm, or if there would be any difference there at all?
The license *info* seems to only apply to the binary rpms...

I will be re-repacking the source, anyhow, since it's unclear, for the time being, better safe than sorry.

I'm keeping the Enet lib code (not binary lib) in there though (it is clearly MIT-free), since upstream has warned that Enet shouldn't be treated as a separate lib (and asked for it to be used embedded, which I'm not doing), so I'm guessing that at some point it might be necessary to make use of it, if we're unlucky, better be prepared.
Comment 22 Martin Erik Werner 2012-03-12 11:48:34 EDT
I had to check the koji build above, lucky that was using the initial fully-repacked tarball, so that should be ok :)

With repacking again:
spec URL: http://arand.fedorapeople.org/8/redeclipse.spec

Are there anything else at the moment worth tweaking? Or is it getting to the point of being ready (and waiting for Enet)?
Comment 23 Brendan Jones 2012-03-12 13:54:18 EDT
All looking pretty good. As this is your first package whoever sponsors you will want to have seen at least one informal review done by yourself of another package. 

You can "yum install fedora-review" and use that as your template - or choose one from here: http://fedoraproject.org/wiki/Category:Package_Maintainers/Review_Template. 

Choose any review request you find interesting - just make sure its one that doesn't track the FE-NEEDSPONSER bug. Just list them here so your sponsor can find them easily. You are also free to adhoc comment on any package review you find. 

All in all I think you've done good work here - Fedora and Debian aren't too different when it comes to policy so you shouldn't have any issues grasping the way things are done around Fedora.
Comment 24 Brendan Jones 2012-03-12 14:18:42 EDT
Also have just seen Jon's reply to your question regarding the content license. You will need to remove those as well.

There's also a tool included with rpmdevtools you can run on your source directory. It's not perfect but it may provide some help.

licensecheck <build-dir> -r
Comment 25 Martin Erik Werner 2012-03-12 14:31:26 EDT
There are no NonCommercial content in the RE archive, I was asking that on the account of egoboo.

Yes, I'm well aware of that tool :)
Comment 26 Brendan Jones 2012-03-12 15:42:13 EDT
I've picked up another game for review, also using enet. This could be a good candidate for your informal review. Let me know though as I'm planning on finishing it off in the next day or so (bug 801092).
Comment 27 Martin Erik Werner 2012-03-12 17:05:48 EDT
Yeah, I'm up for having a look at it, will do so now.
I made a warm-up review over at https://bugzilla.redhat.com/show_bug.cgi?id=769794 too
Comment 28 Martin Erik Werner 2012-03-12 19:26:35 EDT
Review done for Sumwars over at bug 801092.
Comment 29 Martin Erik Werner 2012-03-13 02:22:58 EDT
- 1.2-5
- Add Icon Cache scriptlet snippet
- Add BuildArch: noarch for -data subpackage
- Use %%{version} for Source0 and <version> in gen-tarball comment

spec URL: http://arand.fedorapeople.org/9/redeclipse.spec
Comment 30 Martin Erik Werner 2012-03-14 21:48:04 EDT
Brendan, should I at this point be looking for a sponsor? Or is there more to be done before that step?
Comment 31 Tom "spot" Callaway 2012-03-19 16:41:40 EDT
Can you upload the patches for this? The SRPM I downloaded today seems to be missing them.
Comment 32 Martin Erik Werner 2012-03-19 19:21:42 EDT
Ah, indeed, sorry about that, I've skimped a bit too much on the SRPM uploads.

I also spotted some additional things, font miss - facepalm!

- 1.2-6
- Remove fonts in %_prep
- Corrections in all-licenses
  + Adapt comment about font generation (source URL & cube2font)
  + Remove superfluous overview "License:" field (copy of license.txt)


spec URL: http://arand.fedorapeople.org/10/redeclipse.spec
patch0  : http://arand.fedorapeople.org/10/redeclipse-1.2-windowed-by-default.patch
patch1  : http://arand.fedorapeople.org/10/redeclipse-1.2-backported-fix-icon-sizes.patch
patch2  : http://arand.fedorapeople.org/10/redeclipse-1.2-build-with-system-enet.patch
Source1 : http://arand.fedorapeople.org/10/all-licenses
Gen-tar : http://arand.fedorapeople.org/10/redeclipse-generate-tarball.sh

Say if you want the full SRPM.
Comment 33 Martin Erik Werner 2012-03-19 19:22:04 EDT
I've had a look through my gen-tar repacking, and I'm getting unsure again..

None of the following things are used, but how zealous should I be in removing them?
(The CGTextures item is an obvious removal, legally, so repacking is required no matter what.)

There are a bunch of binary blobs which are LGPL, they have no source in this tarball, however their source can be found in other Fedora packages (though likely the wrong version), and online, e.g. the SDL libs.

Yet more blobs are permissively licensed, without source, e.g. freetype.lib.

There's also a bunch of build/install items for win/osx.


So what I'm wondering is, which of these needs to be taken care of in gen-tar, and which in %prep?

And if the case is that I'm already doing repacking, is it wrong to remove more cruft whilst "already at it"?
(Size-wise the gain is ~50MB for the SRPM...)
Comment 34 Tom "spot" Callaway 2012-03-20 09:10:31 EDT
So, the rule is this:

* If something is not used, but it is licensed so that it is redistributable without restrictions, it should be deleted in %prep, but can stay in the tarball. However, you are welcome to remove it from the tarball if you are already generating a modified tarball to make the SRPM smaller.

* Items under licenses which are not redistributable without restrictions must not be included in the Sources.

As to your question about cruft, I think it would be nice to scrape off as much as you can, but I will not block on review for it.
Comment 35 Martin Erik Werner 2012-03-21 05:41:39 EDT
Ah, excellent, as per that I'll keep my current cruft removal (which is currently zealous) pretty much as-is.

I'll also do:
- 1.2-7
- Move removal of Enet & fonts from %%prep to generate-tarball script
- Re-add generate-tarball script as Source1

Should I be versioning my repacking by the way? Like redeclipse-1.2-fs3.tar.xz, in order to distinguish different repackings, or is that unnecessary since it's already contained in a versioned SRPM?


spec URL: http://arand.fedorapeople.org/11/redeclipse.spec
srpm URL: http://arand.fedorapeople.org/11/redeclipse-1.2-7.fc17.src.rpm
Gen-tar : http://arand.fedorapeople.org/11/redeclipse-generate-tarball.sh
Comment 36 Tom "spot" Callaway 2012-03-21 09:26:02 EDT
That's really up to you. I would not recommend you use the same tarball name as the original, just for the purposes of sanity, but there is no real need to sub-version it, unless you anticipate it will be a frequent occurrence (and even then, probably unnecessary because the lookaside cache that Fedora uses to hold the source tarballs uses md5sums to differentiate files, not just file names).
Comment 37 Martin Erik Werner 2012-03-26 07:24:54 EDT
Ah, ok, then I'll stick with what I have (-1.2-fs).

Also noticed the client/server bins was getting stripped in the %install phase:
- 1.2-8
- Set flags for make install to avoid strip and get a useful -debuginfo

spec URL: http://arand.fedorapeople.org/12/redeclipse.spec
Comment 38 Tom "spot" Callaway 2012-03-30 11:58:41 EDT
I haven't forgotten about this, I've just been super super busy this week.
Comment 39 Martin Erik Werner 2012-03-30 12:55:03 EDT
No worries, I kind of guessed that :)
Comment 40 Martin Erik Werner 2012-04-17 07:11:36 EDT
Reverting spurious Bug 553189 - mis-assignment to 0xFFFF
Comment 41 Tom "spot" Callaway 2012-05-02 15:47:26 EDT
=== REVIEW ===
Needs Fixes:

* You should run desktop-file-verify over the .desktop file. See:
https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
and be sure to add BuildRequires: desktop-file-utils

* You have an explicit Requires: redeclipse-data, should that be a versioned requires to ensure clean upgrades? I think Requires: redeclipse-data = %{version}-%{release} even makes sense here.

* rpmlint:

cube2font.x86_64: W: spelling-error %description -l en_US utiliy -> utility, utilize

This one is a legitimate typo. Please fix.

Good:

- rpmlint checks return:
redeclipse.src: W: spelling-error Summary(en_US) Multiplayer -> Multiplier, Multiplexer
redeclipse.src: W: spelling-error %description -l en_US multi -> mulch, mufti
redeclipse.src: W: spelling-error %description -l en_US gameplay -> game play, game-play, nameplate
redeclipse.src: W: spelling-error %description -l en_US gamemodes -> game modes, game-modes, gametes
redeclipse.src: W: spelling-error %description -l en_US mutators -> commutators, mutilators, commutator
redeclipse.src: W: invalid-url Source0: redeclipse-1.2-fs.tar.xz
redeclipse.x86_64: W: spelling-error Summary(en_US) Multiplayer -> Multiplier, Multiplexer
redeclipse.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
redeclipse.x86_64: W: spelling-error %description -l en_US gameplay -> game play, game-play, nameplate
redeclipse.x86_64: W: spelling-error %description -l en_US gamemodes -> game modes, game-modes, gametes
redeclipse.x86_64: W: spelling-error %description -l en_US mutators -> commutators, mutilators, commutator

All of the above are safe to ignore.

- package meets naming guidelines
- package meets packaging guidelines
- license (zlib and CC-BY-SA) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- content is okay
- no need for -docs
- nothing in %doc affects runtime 

Fix those three items and I'll approve this.
Comment 42 Martin Erik Werner 2012-05-02 18:21:41 EDT
Hopefully they are addressed correctly:

* Wed May 02 2012 Martin Erik Werner <martinerikwerner@gmail.com>
- 1.2-9
- Use desktop-file-validate in %%install step
- Typo fix in cube2font %%description: utiliy -> utility
- Added versioned Requires: redeclipse-data = %%{version}-%%{release}

spec URL: http://arand.fedorapeople.org/13/redeclipse.spec
Comment 43 Tom "spot" Callaway 2012-05-03 10:16:16 EDT
Looks great, this package is APPROVED.

Let me know what your Fedora Account username is and I will sponsor you.
Comment 44 Martin Erik Werner 2012-05-03 11:51:23 EDT
It's "arand"
Comment 45 Tom "spot" Callaway 2012-05-03 12:19:25 EDT
You're sponsored (and the review is done, so you don't need to change that flag).
Comment 46 Martin Erik Werner 2012-05-03 13:32:17 EDT
New Package SCM Request
=======================
Package Name: redeclipse
Short Description: Multiplayer FPS game based on Cube2
Owners: arand
Branches: f16 f17
InitialCC:
Comment 47 Gwyn Ciesla 2012-05-03 13:35:18 EDT
Git done (by process-git-requests).
Comment 48 Bruno Wolff III 2012-05-04 13:18:13 EDT
Were you going to add redeclipse to the games group in comps? That's normal for games.
Comment 49 Martin Erik Werner 2012-05-04 14:21:39 EDT
Ah, thanks for the hint, added in rawhide, was told to wait until package was in stable repos to add it to f17 though.
Comment 50 Bruno Wolff III 2012-05-12 12:55:52 EDT
I have another question. I see that there is an f17 build of redeclipse, but I am not finding the package in bohdi. Did you make a request to move the package to f17 updates-testing?
Comment 51 Fedora Update System 2012-05-12 13:25:06 EDT
redeclipse-1.2-9.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/redeclipse-1.2-9.fc17
Comment 52 Martin Erik Werner 2012-05-12 13:28:33 EDT
Ah, I read that but thought updates meant... updates, and wasn't relevant in the case of new packages, submitted now.
Comment 53 Fedora Update System 2012-05-12 15:45:22 EDT
redeclipse-1.2-9.fc17 has been pushed to the Fedora 17 testing repository.
Comment 54 Fedora Update System 2012-06-03 07:24:43 EDT
redeclipse-1.2-10.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/redeclipse-1.2-10.fc17
Comment 55 Martin Erik Werner 2012-06-29 15:23:39 EDT
This now having passed testing-verification, so I figured I'd say a somewhat overdue thank you to everyone who helped me along in packaging RE, especially for reviewing and polishing it through a rather lengthy process :)

Thank you!
Comment 56 Fedora Update System 2012-06-30 18:09:42 EDT
redeclipse-1.2-10.fc17 has been pushed to the Fedora 17 stable repository.

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