Bug 225717 - Merge Review: ed
Merge Review: ed
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:31 EST by Nobody's working on this, feel free to take it
Modified: 2008-03-24 20:46 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-24 20:46:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:31:36 EST
Fedora Merge Review: ed

http://cvs.fedora.redhat.com/viewcvs/devel/ed/
Initial Owner: karsten@redhat.com
Comment 1 Pace Willisson 2007-02-03 16:15:06 EST
Change "Prereq /sbin/install-info" to "BuildRequires info"

In %changelog, change "May 23 1999" comment from "%post" to "%%post"

Remove dot from end of Summary:
Comment 2 Michael Schwendt 2007-02-04 07:42:51 EST
Missing "rm -rf $RPM_BUILD_ROOT" at beginning of %install section.

> install doc/ed.1 $RPM_BUILD_ROOT%{_mandir}/man1

Here, you could use "install -p -m0644" to preserve time-stamps and
set the file access permissions, and then spare yourself the separate 
%attr(0644,root,root) in the %files section.
Comment 3 Karsten Hopp 2007-02-05 11:26:12 EST
re comment #1: 'BuildRequires info' is wrong, please don't suggest this in other
reviews.
This needs to be:
Requires(post): /sbin/install-info
Requires(preun): /sbin/install-info
Comment 4 Karsten Hopp 2007-02-05 11:39:03 EST
This review is quite incomplete:
- Buildroot should be versioned according to the packaging guidelines.
- rpmlint complains about mixed-use-of-spaces-and-tabs
- %makeinstall shouldn't be used, 'make install DESTDIR=$RPM_BUILD_ROOT' is better

Fixed in ed-0.4-3
Comment 5 Karsten Hopp 2007-03-13 11:36:04 EDT
What's the status of this review ? Is it approved ?
Comment 6 Tom "spot" Callaway 2007-03-13 11:50:48 EDT
I'll do the review this afternoon for this one.
Comment 7 Tom "spot" Callaway 2007-03-13 15:36:53 EDT
Why does ed BuildRequires: autoconf? I never see it invoked, and it doesn't need
it to build.

REVIEW
=========

Blocker:
BR: autoconf is not necessary

Good:

- rpmlint checks return nothing
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Fix the one blocker and I'll approve this package.
Comment 8 Karsten Hopp 2007-03-14 07:25:16 EDT
I've removed BR autoconf and updated to ed-0.5 which fixes another issue with
'%' as a range. New package is ed-0.5-1
Comment 9 Tom "spot" Callaway 2008-03-24 20:46:57 EDT
Whoa, this one got really forgotten in the shuffle. Everything looks good here,
except for the license tag, which was just a minor error that I've corrected.

Approved.

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