Bug 650643

Hello I'm not a sponsor, but I have a few suggestions reviewing you package.

rpmlint SPECS/cegui06.spec SRPMS/cegui06-0.6.2-1.fc14.src.rpm
SPECS/cegui06.spec:9: W: macro-in-comment %{version}
SPECS/cegui06.spec: W: invalid-url Source0: CEGUI-0.6.2.tar.gz
cegui06.src: W: spelling-error %description -l en_US codecs -> codes, coders, code's
cegui06.src: W: spelling-error %description -l en_US xml -> XML, cml, ml
cegui06.src:9: W: macro-in-comment %{version}
cegui06.src: W: invalid-url Source0: CEGUI-0.6.2.tar.gz
1 packages and 1 specfiles checked; 0 errors, 6 warnings.

Change the comments above the Source0 field on how to generate the tarball used in the srpm
Use a link such as:
 http://downloads.sourceforge.net/crayzedsgui/CEGUI-0.6.2b.tar.gz
I would remove the macro %{version}, from your comment
Change the spelling of xml to XML in your %description

(In reply to comment #1)
> Hello I'm not a sponsor, but I have a few suggestions reviewing you package.
> 
> Change the comments above the Source0 field on how to generate the tarball used
> in the srpm

That means I would need to update the comment with each new upstream release, thanks but no thanks.

> Use a link such as:
>  http://downloads.sourceforge.net/crayzedsgui/CEGUI-0.6.2b.tar.gz

No, a link cannot be used as we are not using the tarbals as provided by upstream as those contain a few non free sources which we cannot distribute in Fedora.

> Change the spelling of xml to XML in your %description

I'll fix that in a next revision.

(In reply to comment #2)
> (In reply to comment #1)
> > Hello I'm not a sponsor, but I have a few suggestions reviewing you package.
> > 
> > Change the comments above the Source0 field on how to generate the tarball used
> > in the srpm
> 
> That means I would need to update the comment with each new upstream release,
> thanks but no thanks.
> 

This is a MUST
https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

i.e. If you modify the upstream tarball, you must provide the
     way in detail how you generated the new tarball.

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Hello I'm not a sponsor, but I have a few suggestions reviewing you package.
> > > 
> > > Change the comments above the Source0 field on how to generate the tarball used
> > > in the srpm
> > 
> > That means I would need to update the comment with each new upstream release,
> > thanks but no thanks.
> > 
> 
> This is a MUST
> https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code
> 
> i.e. If you modify the upstream tarball, you must provide the
>      way in detail how you generated the new tarball.

I'm doing that! I was referring to how replacing %{version} in the generate instructions with the actual version would require me to update the generate
instructions each release. If the generation procedure changes the instructions
need to be updated of course.

(In reply to comment #4)
> (In reply to comment #3)
> > i.e. If you modify the upstream tarball, you must provide the
> >      way in detail how you generated the new tarball.
> 
> I'm doing that! I was referring to how replacing %{version} in the generate
> instructions with the actual version would require me to update the generate
> instructions each release. If the generation procedure changes the instructions
> need to be updated of course.

Just using %%version in comment or $VERSION in generation script
should be enough (Of course if generation procedure changes the comment
or the script should be rewritten anyway)

Has someone committed to officially doing this review?

(In reply to comment #6)
> Has someone committed to officially doing this review?

No. You can see this by the "assigned to" bugzilla field, the rule is that if someone wants to claim a package for review he/she should assign the review bug to him/herself. Since this is currently assigned to: "Nobody's working on this, feel free to take it", you are free to take it :)

Greetings, here are more details of certain suggestions I made previously.

The following is an excerpt taken from
http://people.fedoraproject.org/~jwrdegoede/cegui06.spec

# This is
# http://downloads.sourceforge.net/crayzedsgui/CEGUI-%{version}.tar.gz
# with the bundled GLEW: RendererModules/OpenGLGUIRenderer/GLEW
# removed as its an older GLEW version which contains
# parts under then non Free SGI OpenGL and GLX licenses
Source0:        CEGUI-%{version}.tar.gz

These are inaccurate comments on what the source is or contains, not how it is
generated. The current macro %{version} would expand to 0.6.2. The current URL
in the comments is inaccurate which would expand to
http://downloads.sourceforge.net/crayzedsgui/CEGUI-0.6.2.tar.gz. This points to
a non-existent resource which redirects you to
http://sourceforge.net/projects/crayzedsgui/files/ that lists all files of the
project. One can only guess that what the comments are referring to is
http://downloads.sourceforge.net/crayzedsgui/CEGUI-0.6.2b.tar.gz. This is a
guess. One cannot be sure how the source tar is generated or from what
resource.

The following is a general example as it is still unclear of what resource is
used or what is excluded. Here is a starting-point suggestion for the source
tar generation comments or script:
# CEGUI contains some code that we cannot ship
# Do the following to generate the tarball:
# Download http://downloads.sourceforge.net/crayzedsgui/CEGUI-%%versionb.tar.gz
# tar xzf CEGUI-%%versionb.tar.gz
--exclude=CEGUI-%%version/RendererModules/OpenGLGUIRenderer/GLEW
# tar czf CEGUI-%%version.tar.gz CEGUI-%%version

(In reply to comment #8)
> The current macro %{version} would expand to 0.6.2. The current URL
> in the comments is inaccurate which would expand to
> http://downloads.sourceforge.net/crayzedsgui/CEGUI-0.6.2.tar.gz. This points to
> a non-existent resource

Ah I see, yes that is a problem. The cegui06 package is based on the F-14 cegui package and I took the tarbal and Source0 comment from there. It seems that since that rpm was made, upstream did a small bugfix release named 0.6.2b,
and *removed* the old tarbal, bad upstream!

Anyways I've redone the package using the new 0.6.2b tarbal (which only contains fixes to the lua plugin which does not get build) as a base and completely spelling out the removal of the GLEW directory (really how hard is it to read?).

Sorry if I sound a bit grumpy, I admit the url to the original tarbal has gone 404, but really how hard is it to understand:
"with the bundled GLEW: RendererModules/OpenGLGUIRenderer/GLEW removed" ?

Anyways here is a new version based on the new 0.6.2b tarbal:
Spec URL: http://people.fedoraproject.org/~jwrdegoede/cegui06.spec
SRPM URL:
http://people.fedoraproject.org/~jwrdegoede/cegui06-0.6.2-2.fc14.src.rpm

Hans, the source rpm seems broken - it reports size of 20911303 B, but I can download only 7708672 B and get cpio error when unpacking it.

Hi,

(In reply to comment #10)
> Hans, the source rpm seems broken - it reports size of 20911303 B, but I can
> download only 7708672 B and get cpio error when unpacking it.

How right you are, looks like the upload got truncated somehow, I've just re-uploaded it.

Thanks,

Hans

formal review is here, see the notes explaining OK* and BAD statuses below:

OK	source files match the one got by cleaning upstream one
OK*	package meets naming and versioning guidelines.
OK	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
BAD	license field matches the actual license.
OK	license is open source-compatible. License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
OK	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	correct scriptlets present.
OK	code, not content.
OK	documentation is in -docs subpackage
OK	%docs are not necessary for the proper functioning of the package.
OK	headers in -devel
OK	pkgconfig files in -devel.
OK	no libtool .la droppings.
OK	not a GUI app.

- no diff between the included source archive and the one reproduced with the steps written in spec file
- I think the version should be 0.6.2b instead of 0.6.2
- there are 3 files in the src/elements directory that are LGPLv2+ licensed, looks like as an omission, the copyright holder is the same as in the MIT licensed files, also it's fixed in 0.7.5, I am not sure what will be the best solution here ...

(In reply to comment #12)
> - no diff between the included source archive and the one reproduced with the
> steps written in spec file

? Not sure what you mean here.

> - I think the version should be 0.6.2b instead of 0.6.2

Alphanumeric chars in the version are frowned upon / disallowed by the guidelines I could add the b to the release field.

> - there are 3 files in the src/elements directory that are LGPLv2+ licensed,
> looks like as an omission, the copyright holder is the same as in the MIT
> licensed files, also it's fixed in 0.7.5, I am not sure what will be the best
> solution here ...

I think that given that this is a library, and we're talking LGPL not GPL, that it probably is best / easiest to just put "MIT and LGPLv2+" in the license tag.
Do you agree?

(In reply to comment #13)
> (In reply to comment #12)
> > - no diff between the included source archive and the one reproduced with the
> > steps written in spec file
> 
> ? Not sure what you mean here.

just a note how I've compared the sources

> > - I think the version should be 0.6.2b instead of 0.6.2
> 
> Alphanumeric chars in the version are frowned upon / disallowed by the
> guidelines I could add the b to the release field.

OK, let it be as it's now (0.6.2)
 
> > - there are 3 files in the src/elements directory that are LGPLv2+ licensed,
> > looks like as an omission, the copyright holder is the same as in the MIT
> > licensed files, also it's fixed in 0.7.5, I am not sure what will be the best
> > solution here ...
> 
> I think that given that this is a library, and we're talking LGPL not GPL, that
> it probably is best / easiest to just put "MIT and LGPLv2+" in the license tag.
> Do you agree?

yes

This package is APPROVED.

Thanks for the review!

New Package SCM Request
=======================
Package Name: cegui06
Short Description: CEGUI library 0.6 for apps which need this specific version
Owners: jwrdegoede
Branches:
InitialCC:

Git done (by process-git-requests).

Imported and build (this had to wait for the main cegui package to move to 0.7.x), closing.