Bug 225630

Summary: Merge Review: buildsys-macros
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Dennis Gregorovic <dgregor>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dgregor
Target Milestone: ---Flags: roozbeh: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-02-03 19:54:32 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Nobody's working on this, feel free to take it 2007-01-31 17:48:13 UTC
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 15:17:34 UTC
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 22:43:31 UTC
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 13:33:04 UTC
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 15:21:38 UTC
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.