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)
Summary: | Review Request: thewidgetfactory - A tool for previewing widgets | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Luya Tshimbalanga <luya_tfz> |
Component: | Package Review | Assignee: | Peter Gordon <peter> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cheese, luya |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-10-23 09:05:05 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Luya Tshimbalanga
2006-10-21 03:45:21 UTC
(In reply to comment #0) > SRPM URL: http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.x86_64.rpm SRPM? Whoops. Here is the SRPM http://www.thefinalzone.com/RPMS/thewidgetfactory-0.2.1-1.src.rpm 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.) 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. Er..didn't mean to close the bug.... 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. 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. (Er. How the heck did that change from Package Review to 915resolution? Sorry about that one...) It looks like a problem related to bugzilla. *** Bug 249595 has been marked as a duplicate of this bug. *** unsetting cvs flag. please reset with a cvs request if you have one. |