[Bug 225698] Merge Review: dmidecode

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225698

--- Comment #13 from Prarit Bhargava <prarit at redhat.com> 2010-11-08 14:29:36 EST ---
(In reply to comment #9)
> Thanks for the update Prarit. I know you did it with good intentions but please
> don't close a Merge Review until the reviewer sets the "+" flag.
> 
> I see that some issues are resolved but some are not. And since the package had
> updates since I made the initial review, there are additional issues
> introduced, thus I'm reopening the bug (sorry!)
> 
> (For each issue, I am adding the reference to the respective guideline which
> you can find at the bottom.)
> 
> First the old issues and questions: (from comment #5)
> > 
> > * The release tag is a mess. Can we continue with the usual convention?:
> > 2%{?dist} [1]

This appears to be the case in the latest spec:

Version:        2.10
Release:        2%{?dist}
Epoch:          1

(or maybe I'm misunderstanding you ...)

> > 
> > * Source0 must be full URL (with %{name} and %{version} macros) [2]

Fixed.

> > 
> > - Buildroot is improper but it will be obsoleted soon so it's not a problem.
> > 
> > * We prefer %defattr(-,root,root,-) [3]

In latest spec:

%files
%defattr(-,root,root)
> > 
> > * Parallel make must be supported whenever possible. If it is not supported,
> > this should be noted in the SPEC file as a comment. [4]
> > 

Done -- added '-j' to make

> 
> Also additional issues:
> 
> * Usage of $RPM_BUILD_ROOT and %{buildroot} in the spec file is against the
> macro consistency guideline. Please only use one or the other. [5]

Done.  Using %{buildroot} only.

> 
> ! The patches should be explained and links from upstream bugtracker should be
> given as comments, if possible. [6]

Okay -- I'm planning on updating dmidecode for Fedora shortly so these patches
won't be necessary.  I will make note of this practice for the future ...

> 
> ! BuildRequires:  /usr/bin/aclocal /usr/bin/automake /usr/bin/autoconf
> can be replaced by simply
>   BuildRequires:  automake autoconf
> since file dependencies should be avoided as much as possible. [7]

Ah, cool :)  Done.

> 
> [1] http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Release
> [2] http://fedoraproject.org/wiki/Packaging:SourceURL
> [3] https://bugzilla.redhat.com/show_bug.cgi?id=481363
> [4] http://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
> [5] http://fedoraproject.org/wiki/Packaging/Guidelines#macros
> [6]
> http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
> [7] http://fedoraproject.org/wiki/Packaging/Guidelines#File_Dependencies

[Stuck trying to get these changes committed to git ... will ping when it's
done]

P.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.