Bug 225717 - Merge Review: ed
Summary: Merge Review: ed
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
Target Milestone: ---
Assignee: Tom "spot" Callaway
QA Contact: Fedora Package Reviews List
Depends On:
TreeView+ depends on / blocked
Reported: 2007-01-31 18:31 UTC by Nobody's working on this, feel free to take it
Modified: 2008-03-25 00:46 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-03-25 00:46:57 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora-review+

Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 18:31:36 UTC
Fedora Merge Review: ed

Initial Owner: karsten@redhat.com

Comment 1 Pace Willisson 2007-02-03 21:15:06 UTC
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 12:42:51 UTC
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 16:26:12 UTC
re comment #1: 'BuildRequires info' is wrong, please don't suggest this in other
This needs to be:
Requires(post): /sbin/install-info
Requires(preun): /sbin/install-info

Comment 4 Karsten Hopp 2007-02-05 16:39:03 UTC
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 15:36:04 UTC
What's the status of this review ? Is it approved ?

Comment 6 Tom "spot" Callaway 2007-03-13 15:50:48 UTC
I'll do the review this afternoon for this one.

Comment 7 Tom "spot" Callaway 2007-03-13 19:36:53 UTC
Why does ed BuildRequires: autoconf? I never see it invoked, and it doesn't need
it to build.


BR: autoconf is not necessary


- 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 11:25:16 UTC
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-25 00:46:57 UTC
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.


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