Bug 650643 - Review Request: cegui06 - CEGUI library 0.6 for apps which need this specific version
Review Request: cegui06 - CEGUI library 0.6 for apps which need this specific...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-07 10:18 EST by Hans de Goede
Modified: 2011-01-03 04:33 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-01-03 04:33:56 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2010-11-07 10:18:25 EST
Spec URL: http://people.fedoraproject.org/~jwrdegoede/cegui06.spec
SRPM URL: http://people.fedoraproject.org/~jwrdegoede/cegui06-0.6.2-1.fc14.src.rpm
Description:
Crazy Eddie's GUI System is a free library providing windowing and widgets for
graphics APIs / engines. This package contains the older version 0.6 for
apps which cannot be easily ported to 0.7. As such this version has been build
without additional image codecs or xml parsers.
Comment 1 Steven Garcia 2010-11-07 18:07:43 EST
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
Comment 2 Hans de Goede 2010-11-08 03:48:04 EST
(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.
Comment 3 Mamoru TASAKA 2010-11-08 04:00:23 EST
(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.
Comment 4 Hans de Goede 2010-11-08 05:09:15 EST
(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.
Comment 5 Mamoru TASAKA 2010-11-08 08:24:17 EST
(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)
Comment 6 Bruno Wolff III 2010-11-08 16:14:51 EST
Has someone committed to officially doing this review?
Comment 7 Hans de Goede 2010-11-08 16:25:01 EST
(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 :)
Comment 8 Steven Garcia 2010-11-08 16:48:10 EST
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
Comment 9 Hans de Goede 2010-11-09 03:31:01 EST
(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
Comment 10 Dan Horák 2010-11-22 08:11:44 EST
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.
Comment 11 Hans de Goede 2010-11-22 08:39:04 EST
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
Comment 12 Dan Horák 2010-11-22 10:34:43 EST
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 ...
Comment 13 Hans de Goede 2010-11-22 10:49:48 EST
(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?
Comment 14 Dan Horák 2010-11-22 11:03:57 EST
(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.
Comment 15 Hans de Goede 2010-11-22 14:28:49 EST
Thanks for the review!
Comment 16 Hans de Goede 2010-11-22 14:30:03 EST
New Package SCM Request
=======================
Package Name: cegui06
Short Description: CEGUI library 0.6 for apps which need this specific version
Owners: jwrdegoede
Branches:
InitialCC:
Comment 17 Jason Tibbitts 2010-11-23 11:01:52 EST
Git done (by process-git-requests).
Comment 18 Hans de Goede 2011-01-03 04:33:56 EST
Imported and build (this had to wait for the main cegui package to move to 0.7.x), closing.

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