Bug 198944 - Review Request: compiz
Review Request: compiz
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Cantrell
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FC-ACCEPT
  Show dependency treegraph
 
Reported: 2006-07-14 16:40 EDT by Kristian Høgsberg
Modified: 2013-01-09 20:27 EST (History)
1 user (show)

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


Attachments (Terms of Use)

  None (edit)
Description Kristian Høgsberg 2006-07-14 16:40:09 EDT
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-14 22:25:01 EDT
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 17:31:42 EDT
I'd propose to close it as obsolete once compiz shows up in core
Comment 3 Jesse Keating 2006-07-17 14:26:15 EDT
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 16:09:54 EDT
- 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-17 21:59:01 EDT
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 11:31:20 EDT
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 12:27:40 EDT
package owner will be krh, I'll handle the initial import and comps addition
Comment 9 Jesse Keating 2006-07-18 12:51:01 EDT
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.