Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 211718 (thewidgetfactory) - Review Request: thewidgetfactory - A tool for previewing widgets
Summary: Review Request: thewidgetfactory - A tool for previewing widgets
Keywords:
Status: CLOSED NEXTRELEASE
Alias: thewidgetfactory
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Gordon
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
: 249595 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-10-21 03:45 UTC by Luya Tshimbalanga
Modified: 2009-04-01 16:29 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-23 09:05:05 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Luya Tshimbalanga 2006-10-21 03:45:21 UTC
Spec URL: http://www.thefinalzone.com/RPMS/thewidgetfactory.spec
SRPM URL: http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.x86_64.rpm
Description: The Widget Factory is a program designed to show a wide range of widgets
on a single window.

Comment 1 Ralf Corsepius 2006-10-21 04:13:27 UTC
(In reply to comment #0)
> SRPM URL: http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.x86_64.rpm

SRPM?

Comment 2 Luya Tshimbalanga 2006-10-21 04:50:34 UTC
Whoops. Here is the SRPM
http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.src.rpm

Comment 3 Peter Gordon 2006-10-21 07:14:15 UTC
A brief comment before I start a formal review of this: Your %files section
contains %{_bindir}, which means that the package will own /usr/bin (or whatever
directory it expands to for the user's RPM macros). It should only own the
specific binaries it provides. %{_bindir} and other system directories are owned
by the fileystem package, and this is a Very Bad Thing(TM). (See
http://fedoraproject.org/wiki/Packaging/Guidelines#head-a5931a7372c4a00065713430984fa5875513e6d4
for more information.)

Comment 4 Peter Gordon 2006-10-21 08:21:26 UTC
Hi, Luya. Ok, here we go. :)

** MUST items **

GOOD: rpmlint on the source RPM is silent
The binary RPM gives one error:
  E: thewidgetfactory standard-dir-owned-by-package /usr/bin
See the Blockers section below for more information.

GOOD: Timestamps in the source appear to be preserved.

GOOD: Package complies with the NamingGuidelines

GOOD: The spec file is named appropriately, in the form "%{name}.spec" 

GOOD: License is open-source compatible (GPL), and the License field in the spec
file correctly notes this.

GOOD: A copy of the license is included in the package (COPYING, in %doc)

GOOD: The spec is written in American English, and is clear and legible.

GOOD: The source tarball included in the SRPM matches that of upstream.
  $ md5sum thewidgetfactory-*.tar.gz
  60175721233c6f265326fcdc0334c269  thewidgetfactory-0.2.1-srpm.tar.gz
  60175721233c6f265326fcdc0334c269  thewidgetfactory-0.2.1-upstream.tar.gz

GOOD: The package successfully builds in mock (FC6/x86)

GOOD: All necessary BuildRequires listed. (Probably a bit simpler than many
other packages, since there is only one. ^_^) 

GOOD: No duplicate files listed in %files.

GOOD: The spec contains a %clean section, which invokes a single "rm -rf
$RPM_BUILD_ROOT" command.

GOOD: Usage of macros in the spec is consistent.

GOOD: The package contains code, and no prohibited content.

GOOD: Files marked as %doc do not affect the program at runtime if not present.

GOOD: Package contains no .la libtool archives.


** SHOULD items **

GOOD: A copy of the license (GPL) is included in the tarball from upstream
("COPYING").

GOOD: The package appears to build properly on all supported architectures that
I was able to test (built in an FC6/x86 Mock chroot, and is currently chugging
through an FC5/x86 Mock build, which I expect to succeed).

GOOD: The software contained in the binary package runs as described, with no
noticable errors (FC6/x86).


** Not Applicable **
N/A: The package does not require ExcludeArch semantics.

N/A: The package does not require %find_lang semantics, since it installs no
locales.

N/A: The package does not require %post/%postun calls to /sbin/ldconfig, since
it installs no shared libraries. 

N/A: Package is not relocatable.

N/A: There is no large documentation, so a -doc subpackage is not needed.

N/A: No header files, shared or static library files, so no -devel subpackage is
needed.

N/A: The package contains no pkgconfig (.pc) files.

N/A: The package does not use translations, so no translated %description or
Summary tag is available.

N/A: No scriplets are used.

N/A: No subpackages exist, so worries about fully-versioned Requires for those
are not present.


** Blockers **

BAD: The application includes a GUI interface, but no .desktop file for that
application. (See http://fedoraproject.org/wiki/Packaging/Guidelines#desktop on
the wiki for more information.) If the source from upstream does not have one,
as it seems for this package, please create one yourself and include it as a
separate Source in the RPM. (Remember, then, to add the desktop-file-install
scriptlet to the %install section of the spec file, and add desktop-file-utils
as a BuildRequires for this script call. Also, you'll need to add the generated
.desktop file to your %files listing.)

BAD: The package should not own %{_bindir}, which is owned by the filesystem
package. Unless there is good reason for such ownership to be shared, this
should be changed in the %files to only list the specific binary within that
directory (such as "%{_bindir}/twf").

--

Those are the only two blockers I can see in this. Fix those, and I'll approve
the package for importing into CVS.

Comment 5 Peter Gordon 2006-10-21 08:24:28 UTC
Er..didn't mean to close the bug....

Comment 6 Luya Tshimbalanga 2006-10-21 23:40:31 UTC
Thank you for this excellent reviewing. I have updated these following files:

Spec URL: http://www.thefinalzone.com/RPMS/thewidgetfactory.spec
SRPM URL: http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-2.x86_64.rpm

Upstream does not have a icon for the desktop file, I have to comment out the
Icon  part.

Comment 7 Peter Gordon 2006-10-22 01:18:59 UTC
The  %{_bindir} ownership has been fixed; and the .desktop stuff looks good.
Nice work.

The only other issue I can see is that you might want to use the %{name} macro
as part of the Source1 tag instead of hardcoding it, but that's entirely
personal preference as I understand it, and certainly not a blocker.

This package is therefore APPROVED. Go ahead and import it into CVS and request
branches as needed, etc.

Don't forget to close this bug as NEXTRELEASE after you import and build it.

Comment 8 Peter Gordon 2006-10-22 01:22:24 UTC
(Er. How the heck did that change from Package Review to 915resolution? Sorry
about that one...)

Comment 9 Luya Tshimbalanga 2006-10-22 01:34:43 UTC
It looks like a problem related to bugzilla.

Comment 10 Mamoru TASAKA 2007-07-27 17:57:28 UTC
*** Bug 249595 has been marked as a duplicate of this bug. ***

Comment 11 Dennis Gilmore 2009-04-01 16:29:24 UTC
unsetting cvs flag.  please reset with a cvs request if you have one.


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