Bug 225630 - Merge Review: buildsys-macros
Merge Review: buildsys-macros
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dennis Gregorovic
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:48 EST by Nobody's working on this, feel free to take it
Modified: 2009-09-21 16:17 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-03 14:54:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
roozbeh: fedora‑review+


Attachments (Terms of Use)

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

http://cvs.fedora.redhat.com/viewcvs/devel/buildsys-macros/
Initial Owner: dgregor@redhat.com
Comment 1 Roozbeh Pournader 2007-02-05 10:17:34 EST
rpmlint output:
W: buildsys-macros non-standard-group Development/Buildsystem
W: buildsys-macros incoherent-version-in-changelog 7-1.fc6 7-1.fc7
W: buildsys-macros no-url-tag
W: buildsys-macros no-documentation
W: buildsys-macros non-conffile-in-etc /etc/rpm/macros.disttag

The first two should at least be corrected.

Other random comments:
* Change license to public domain. A 21-byte file is not really worth licensing
as GPL: http://www.gnu.org/licenses/gpl-faq.html#WhatIfWorkIsShort
* Comment line before "Version:" talks about Fedora Core. Should be changed to
Fedora.
* Description field is the same as Summary field. Not good.
* Use %{_sysconfdir} instead of /etc everywhere.
* Consider using echo instead of printf. The printf lines are tricky and not
very legible.
* In the first line that writes to the macros.disttag line, use ">" instead of ">>".
* The package puts files in /etc/rpm without owning that directory or requiring
any package that does so (blocker).
* Change %defattr(-,root,root) to %defattr(-,root,root,-)
Comment 2 Dennis Gregorovic 2007-02-05 17:43:31 EST
Thank you for the review.  I've checked in fixes for everything except
"no-documentation" as none is needed.  
Comment 3 Roozbeh Pournader 2007-02-06 08:33:04 EST
Minor remaining issues:
MUST: US English
* I am not a native speaker, but I think you need a "the" before 'dist' in the
package description: "define the product version and *the* 'dist' tag".

MUST: rpmlint output
W: buildsys-macros conffile-without-noreplace-flag /etc/rpm/macros.disttag
  This is bad, I think. Using %config(noreplace) is recommended, although I
  don't see any real difference, as I don't think anybody may install this
  package on his normal box where it may be updated. It's not in the normal
  repos IIRC.
W: buildsys-macros no-documentation
  It's fine.

The bureaucracy:
MUST: named fine
MUST: spec file named fine
MUST: packaging guidelines met (except noreplace, mentioned above)
MUST: license fine
MUST: no license file needed as it's public domain
MUST: spec file was made legible
MUST: no source
MUST: builds into noarch on FC6/i386
MUST: no excludearch
MUST: no special build deps
MUST: no locale
MUST: not a lib
MUST: not relocatable
MUST: requires rpm that owns /etc/rpm
MUST: permissions fine
MUST: no dup files
MUST: file permissions fine
MUST: %clean section exists
MUST: macro use fine
MUST: package has code
MUST: no large docs
MUST: no %doc
MUST: no header or static lib
MUST: no *.pc
MUST: no *.so.*
MUST: no -devel
MUST: no *.la
MUST: not GUI
MUST: doesn't own others' files

Package is approved.
Comment 4 Dennis Gregorovic 2007-02-06 10:21:38 EST
Thanks again for the review.  I agree that %config(noreplace) makes sense even
though this package isn't expected to be installed by end users.  Fixes checked in.

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