Bug 225630 - Merge Review: buildsys-macros
Summary: Merge Review: buildsys-macros
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dennis Gregorovic
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 17:48 UTC by Nobody's working on this, feel free to take it
Modified: 2009-09-21 20:17 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-02-03 19:54:32 UTC
Type: ---
Embargoed:
roozbeh: fedora-review+


Attachments (Terms of Use)

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

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.


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