Bug 290041 - Review Request: generic-logos - Icons and pictures
Review Request: generic-logos - Icons and pictures
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-13 17:07 EDT by Bill Nottingham
Modified: 2014-03-16 23:08 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-25 11:50:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Bill Nottingham 2007-09-13 17:07:35 EDT
Spec URL: http://people.redhat.com/notting/review/generic-logos.spec
SRPM URL: http://people.redhat.com/notting/review/generic-logos-7.92.1-1.fc8.src.rpm
Description:
The generic-logos package contains various image files which can be
used by the bootloader, anaconda, and other related tools. It can
be used as a replacement for the fedora-logos package, if you are
unable for any reason to abide by the trademark restrictions on the
fedora-logos package.

Needed to implement http://fedoraproject.org/wiki/Releases/FeatureGenericLogos
Comment 1 Kevin Fenzi 2007-09-20 22:48:22 EDT
I'd be happy to review this package. Look for a full review in a bit. 
Comment 2 Kevin Fenzi 2007-09-20 22:57:00 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPLv2)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
See below - Package owns all the directories it creates.  
See below - No rpmlint output.
See below - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag

Issues:

1. Should the
# should be ifarch i386
bits really be %ifarch? why aren't they now...

2. I assume cvs is the Source for this package?
Perhaps it would be worth making a hosted project in case people want to
work on/contribute to this package?

3. rpmlint says:

generic-logos.noarch: W: no-url-tag
generic-logos.src: W: no-url-tag

A hosted (or other upstream) would fix this.

generic-logos.noarch: E: only-non-binary-in-usr-lib

Ignore since it has to put anaconda-runtime has to be there.

generic-logos.src:10: W: unversioned-explicit-obsoletes redhat-logos

Can you specifiy a version on that Obsoletes?

generic-logos.src:70: E: hardcoded-library-path in /usr/lib/anaconda-runtime/*.jpg

No way to avoid that I see off hand. Do you?

generic-logos.src: W: strange-permission generic-logos.spec 0600

Spec should be 644.

4. Should this package Require the anaconda, grub, and other packages
where it places images in dirs they own? Or should this be an exception
to that rule?
Comment 3 Bill Nottingham 2007-09-21 12:36:57 EDT
(In reply to comment #2)
> Issues:
> 
> 1. Should the
> # should be ifarch i386
> bits really be %ifarch? why aren't they now...

It's a noarch package, and making it per-arch probably isn't worth it.

> 2. I assume cvs is the Source for this package?
> Perhaps it would be worth making a hosted project in case people want to
> work on/contribute to this package?

At the moment, it's CVS on my laptop. Will move to somewhere @ fp.o once it's
approved.

> 3. rpmlint says:
> 
> generic-logos.noarch: W: no-url-tag
> generic-logos.src: W: no-url-tag
> 
> A hosted (or other upstream) would fix this.
> 
> generic-logos.noarch: E: only-non-binary-in-usr-lib
> 
> Ignore since it has to put anaconda-runtime has to be there.
> 
> generic-logos.src:10: W: unversioned-explicit-obsoletes redhat-logos
> 
> Can you specifiy a version on that Obsoletes?

Not sure what the last version is. The idea is to obsolete any redhat-logos from
earlier RHL or RHEL releases.

> generic-logos.src:70: E: hardcoded-library-path in /usr/lib/anaconda-runtime/*.jpg
> 
> No way to avoid that I see off hand. Do you?

That's where anaconda is looking for it.

> generic-logos.src: W: strange-permission generic-logos.spec 0600
> 
> Spec should be 644.

AFAICT, this is a RPM bug. It's 0644 in the tarball, and the source RPM is built
with -ts.

> 4. Should this package Require the anaconda, grub, and other packages
> where it places images in dirs they own? Or should this be an exception
> to that rule?

Hm. fedora-logos very specifically doesn't do this, because you don't want the
grub splash pulling in anaconda, for example. This would fall in the same category.
Comment 4 Kevin Fenzi 2007-09-24 13:22:44 EDT
ok. That all makes sense to me... I see no further blockers here, 
so this package is APPROVED. 

Don't forget to close this once it's been imported and built. 

Also, a nice hosted space or something would be good if you can manage it. 

Comment 5 Bill Nottingham 2007-09-24 13:39:11 EDT
New Package CVS Request
=======================
Package Name: generic-logos
Short Description: icons and pictures
Owners: notting@redhat.com
Branches: devel
InitialCC: 
Cvsextras Commits: no
Comment 6 Jason Tibbitts 2007-09-24 21:36:28 EDT
CVS done.

Note: Owners: should be an FAS ID, not an email address.  Fortunately yours is easy.
Comment 7 Bill Nottingham 2007-09-25 11:50:51 EDT
Built.

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