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 177104
Summary: | Review Request: abook - Text-based addressbook program for mutt | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Dominik 'Rathann' Mierzejewski <dominik> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | tcallawa |
Target Milestone: | --- | Flags: | gwync:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-08-16 18:21:07 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
Dominik 'Rathann' Mierzejewski
2006-01-06 12:32:15 UTC
This isn't a formal review because I can't sponsor you, but here are a couple of comments on the spec file: * Consider if you want to use %{?dist} in the release line. It's optional, but many packagers find it useful. This can also be added after importing into CVS. * Please use the recommended value for BuildRoot per the packaging guidelines: http://fedoraproject.org/wiki/Packaging/Guidelines * Don't add the check for "/" in %clean. Just remove $RPM_BUILD_ROOT with no checks. The BuildRoot: setting ensures that it won't be "/". * rpmlint warnings: W: abook non-standard-group Networking/Mail Applications/Internet or Applications/Productivity might be more appropriate. Additionally: * missing rm -rf ${RPM_BUILD_ROOT} as first line of %install * missing BuildRequires: gettext * it defaults to running "lynx" as a textmode web browser, when not installed, the error message is not obvious, since it only flashes on the screen very briefly -- alternatively: Requires: lynx * don't package in %doc the INSTALL file, since it is irrelevant and confusing to users of your package * prefer make DESTDIR=${RPM_BUILD_ROOT} over %makeinstall macro unless the latter breaks > * prefer make DESTDIR=${RPM_BUILD_ROOT} over %makeinstall macro
> unless the latter breaks
should read:
* prefer make DESTDIR=${RPM_BUILD_ROOT} over %makeinstall macro
unless the former breaks and the latter works instead
It's still missing to empty the buildroot at beginning of %install. Fix that after CVS import. APPROVED Dropping FE-NEEDSPONSOR. Tom Callaway offered sponsorship in bug 177235 (and bugzilla change-several-bugs-at-once feature requires me to add/edit something). uhm, bugzilla is broken :( Withdrawing my approval from 2006-04-04. Back to FE-NEW. See bug 177105 comment 14 for background. Updated to 0.5.6. http://rpm.greysector.net/extras/abook.spec http://rpm.greysector.net/extras/abook-0.5.6-1.src.rpm I'll go ahead and take a look at this. It builds fine in mock; rpmlint on the SRPM has this to say: W: abook macro-in-%changelog rlz1 W: abook macro-in-%changelog rlz1 W: abook macro-in-%changelog rlz1 W: abook macro-in-%changelog rlz1 W: abook macro-in-%changelog rlz1 You just need to double some percent signs. E: abook no-cleaning-of-buildroot You should clean out $RPM_BUILD_ROOT at the beginning of %install. rpmlint on the built RPM is quiet. * source files match upstream: 87d25df96864a7c507a4965e6d1da49d abook-0.5.6.tar.gz * package meets naming and packaging guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * dist tag is present. * build root is correct. * license field matches the actual license. * license is open source-compatible. License text included in package. * latest version is being packaged. * BuildRequires are proper. * compiler flags are appropriate. * %clean is present. * package builds in mock (development, x86_64). * debuginfo package looks complete. X rpmlint has valid complaints * final provides and requires are sane: abook = 0.5.6-1.fc6 = libncursesw.so.5()(64bit) libreadline.so.5()(64bit) lynx * %check is not present; no test suite upstream * no shared libraries are present. * package is not relocatable. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. * no libtool .la droppings. * not a GUI app. * locale files present; fing_lang used appropriately. http://rpm.greysector.net/extras/abook.spec http://rpm.greysector.net/extras/abook-0.5.6-2.src.rpm Fixed. rpmlint is now quiet; everything looks good to me. APPROVED Imported and built for devel, FC5 branch requested. Package Change Request ====================== Package Name: abook New Branches: el6 Owners: rathann I'd like to maintain the EPEL-6 branch of this package as well. Git done (by process-git-requests). |