Bug 211718 - (thewidgetfactory) Review Request: thewidgetfactory - A tool for previewing widgets
Review Request: thewidgetfactory - A tool for previewing widgets
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Gordon
Fedora Package Reviews List
:
: 249595 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-20 23:45 EDT by Luya Tshimbalanga
Modified: 2009-04-01 12:29 EDT (History)
2 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description Luya Tshimbalanga 2006-10-20 23:45:21 EDT
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 00:13:27 EDT
(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 00:50:34 EDT
Whoops. Here is the SRPM
http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.src.rpm
Comment 3 Peter Gordon 2006-10-21 03:14:15 EDT
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 04:21:26 EDT
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 04:24:28 EDT
Er..didn't mean to close the bug....
Comment 6 Luya Tshimbalanga 2006-10-21 19:40:31 EDT
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-21 21:18:59 EDT
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-21 21:22:24 EDT
(Er. How the heck did that change from Package Review to 915resolution? Sorry
about that one...)
Comment 9 Luya Tshimbalanga 2006-10-21 21:34:43 EDT
It looks like a problem related to bugzilla.
Comment 10 Mamoru TASAKA 2007-07-27 13:57:28 EDT
*** Bug 249595 has been marked as a duplicate of this bug. ***
Comment 11 Dennis Gilmore 2009-04-01 12:29:24 EDT
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.