Bug 198944 - Review Request: compiz
Summary: Review Request: compiz
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: David Cantrell
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: FC-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-07-14 20:40 UTC by Kristian Høgsberg
Modified: 2013-01-10 01:27 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-18 18:36:26 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

Description Kristian Høgsberg 2006-07-14 20:40:09 UTC
Spec URL: http://people.redhat.com/krh/compiz/compiz.spec
SRPM URL: http://people.redhat.com/krh/compiz/compiz-0.0.13-5.1.fc6.src.rpm
Description: compiz - open gl windows and compositing manager with patches to run on aiglx in fedora core.

Comment 1 Jason Tibbitts 2006-07-15 02:25:01 UTC
Note that there's an existing compiz review ticket pending for Extras.  What
should happen to it?

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=192432

Comment 2 Matthias Clasen 2006-07-15 21:31:42 UTC
I'd propose to close it as obsolete once compiz shows up in core

Comment 3 Jesse Keating 2006-07-17 18:26:15 UTC
How does this spec compare to the one in the Extras review?  Does this one have
the fixes and updates generated from that review?

NEEDSWORK:
- release string should be whole number unless it is a pre-release (snapshot). 
If it is a snapshot, as it looks like, you should use the prelease naming
scheme:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a
- dist tag is %{?dist} not %{dist}
- gconf stuff should follow the scriptletsnippits or have good reason not to:
http://fedoraproject.org/wiki/ScriptletSnippets#head-ff64cd482595764f672082d5a3b83e1fc22962e8
- changelog shouldn't use dist tag as part of the version-release.
- missing br gettext

rpmlint output (once added gettext)
W: compiz-debuginfo invalid-license X11/MIT/GPL
W: compiz invalid-license X11/MIT/GPL
W: compiz invalid-license X11/MIT/GPL

Ignorable I suppose, but it would help if upstream included a LICENSE file.

W: compiz macro-in-%changelog dist

Leave out the macro.

W: compiz mixed-use-of-spaces-and-tabs

Ignorable

W: compiz non-conffile-in-etc /etc/gconf/schemas/compiz.schemas

Should this be marked a conf file?



Comment 4 Matthias Clasen 2006-07-17 20:09:54 UTC
- Can you please explain what version number you would like to see here ?
  compiz-0.0.13-5.1.fedora6
  sounds like a perfectly fine n-v-r to me

- The GConf invokations in %post an %preun look perfectly fine to me. 
  This insistance on slavishly copying some snipplets from a wiki page
  is really the ugliest part of package review...
  The one thing that is wrong with the gconf invokations is the --shutdown
  I think that has no place there.

- We never mark gconf schema files as conf files, why do we have to go thorough
  this again for every package review ?

I'll fix the other issues 


Comment 6 Kristian Høgsberg 2006-07-18 01:59:01 UTC
Thanks for the review, Jesse, comments below.

(In reply to comment #3)
> How does this spec compare to the one in the Extras review?  Does this one have
> the fixes and updates generated from that review?

To the extent that it makes sense, yes.

> NEEDSWORK:
> - release string should be whole number unless it is a pre-release (snapshot). 
> If it is a snapshot, as it looks like, you should use the prelease naming
> scheme:
>
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a
> - dist tag is %{?dist} not %{dist}
> - gconf stuff should follow the scriptletsnippits or have good reason not to:
>
http://fedoraproject.org/wiki/ScriptletSnippets#head-ff64cd482595764f672082d5a3b83e1fc22962e8
> - changelog shouldn't use dist tag as part of the version-release.
> - missing br gettext

Fixed (thanks, Matthias).

> rpmlint output (once added gettext)
> W: compiz-debuginfo invalid-license X11/MIT/GPL
> W: compiz invalid-license X11/MIT/GPL
> W: compiz invalid-license X11/MIT/GPL
> 
> Ignorable I suppose, but it would help if upstream included a LICENSE file.

Upstream doesn't include a license because different parts of the project are
under different licenses.  You need to check individual files for their license.

> W: compiz macro-in-%changelog dist
> 
> Leave out the macro.
> 
> W: compiz mixed-use-of-spaces-and-tabs
> 
> Ignorable
> 
> W: compiz non-conffile-in-etc /etc/gconf/schemas/compiz.schemas
> 
> Should this be marked a conf file?


Comment 7 Jesse Keating 2006-07-18 15:31:20 UTC
Changes look good, rpmlint is fine (with ignores in place)

Approving.  Who wants to own this package in dist-fc6, and how will we put it in
Comps?

Comment 8 Matthias Clasen 2006-07-18 16:27:40 UTC
package owner will be krh, I'll handle the initial import and comps addition

Comment 9 Jesse Keating 2006-07-18 16:51:01 UTC
package added to dist.

Please close when built into rawhide.


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