Red Hat Bugzilla – Bug 225634
Merge Review: cadaver
Last modified: 2012-08-20 08:12:16 EDT
Fedora Merge Review: cadaver
Initial Owner: firstname.lastname@example.org
The current package looks already quite good. A few minor possible spec file
- Remove the zero epoch from the neon-devel BR
- Include a quick comment as to why pie is needed
- Change %defattr(-,root,root) to the more usual %defattr(-,root,root,-)
(pretty useless, though, since the package doesn't include directories)
- Move the %doc line up so that it isn't in between two normal lines
I'll test a build and post the results.
Test build went fine, resulting binary is fine.
You could remove the obsolete "cadaver-0.22.5.tar.gz.asc" from CVS, though.
Thanks for the review!
> - Remove the zero epoch from the neon-devel BR
> - Include a quick comment as to why pie is needed
It's not "needed", it's a feature, I don't think that explaining what -pie is in
every spec file which uses it is really a good idea.
> - Change %defattr(-,root,root) to the more usual %defattr(-,root,root,-)
> (pretty useless, though, since the package doesn't include directories)
> - Move the %doc line up so that it isn't in between two normal lines
And fixed. In -2. Also fixed the use of %makeinstall, which is now unnecessary
Good catch, the %makinstall, I've often missed it in my own spec files :-/
Everything looks good, except maybe the NLS : Passing --enable-nls doesn't
change anything, and configure always reports "Internationalization: Not
built". I'm not sure why since a quick look seems to indicate that the messages
about having NLS enabled should lead to "Internationalization" being enabled.
Anyway, this isn't vital, and not review specific, so feel free to have a look
and close the bug, but I'll fedora-review + anyway.
Ah, the "Not built" thing is a cosmetic configure script bug, I've fixed it
upstream. --enable-nls is in fact the default but being explicit here is good
since the upstream default has changed over time here.
Thanks again for the review.
Package Change Request
Package Name: cadaver
New Branches: el6
See #739496 which was the request for me to take EPEL ownership.
Git done (by process-git-requests).