Bug 637923 - Review Request: firmware-extract - A firmware-tools plugin to add firmware extraction from vendor binaries
Summary: Review Request: firmware-extract - A firmware-tools plugin to add firmware e...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Praveen K Paladugu
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-09-27 18:43 UTC by Matt Domsch
Modified: 2011-03-24 14:11 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-03-24 14:11:46 UTC
Type: ---
Embargoed:
praveen_paladugu: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Matt Domsch 2010-09-27 18:43:05 UTC
Spec URL: http://domsch.com/linux/fedora/firmware-extract.spec
SRPM URL: http://linux.dell.com/files/libsmbios/download/firmware-extract/firmware-extract-2.0.13/firmware-extract-2.0.13-1.src.rpm
Description: 
A firmware-tools plugin which adds the --extract mode to firmwaretool.

Comment 1 Matt Domsch 2010-10-06 02:46:11 UTC
Charles/Sri - maybe something your team can review?  (Fedora new package review process - Michael Brown is the upstream owner.)

Comment 2 Matt Domsch 2010-10-06 02:46:50 UTC
Ah, I see Praveen grabbed it.  Thanks!

Comment 3 Praveen K Paladugu 2010-10-06 13:27:02 UTC

1) Please remove the suse related macros from the spec file.
2) Please update the pythong_sitelib definition with conditionals as shown at: http://fedoraproject.org/wiki/Packaging:Python#Macros 
3) I don't see a need for the following lines in the spec file: 
touch configure
find . -type f -newer configure -print0 | xargs -r0 touch

4) Please update the spec file to use either $RPM_BUILD_ROOT/%{buildroot} instead of both. 

5) Please add a changelog comment to be in consistent with the current versioning.


Nothing stands out in the rpmlint errors :).

Praveen

Comment 4 Michael E Brown 2010-10-06 16:43:53 UTC
1) there are two compat statements for suse compatibility. (6 total LOC, plus associated comments) This is not excessive and doesnt violate packaging standards, as far as I know.

2) The %{!? construct will only define the macro if it is not already defined. Thus, adding an %if only adds extra lines of code without technically changing the behaviour at all. I've changed the comment.

3) I've seen problems on when the system I create the tarball on has a time that is faster than the buildsystem time. These two lines fix the build for this corner case. I've removed them for now, as the build system which I used that had this problem seems to be fixed now.

4) Fixed.

5) fixed

I've pushed the spec file updates to master git, but I have not created a new release since this was only spec updates without code changes.

Comment 5 Matt Domsch 2010-10-06 17:12:10 UTC
http://domsch.com/linux/fedora/firmware-extract/
now has the updated spec and SRPM.

Comment 6 Praveen K Paladugu 2010-10-06 18:55:24 UTC
1))) It certainly doesn't violate any rules. Just wanted to clean up the spec file of dead code/commands. Let it be for now. 

2))) Got it. Thanks



4))) I still see both $RPM_BUILD_ROOT and %{buildroot} being used in the updated spec file.


Praveen

Comment 7 Michael E Brown 2010-10-06 19:18:38 UTC
4) indeed. oops. Fixed, good spot! Fixes pushed to git master.

Comment 8 Praveen K Paladugu 2010-10-06 19:37:30 UTC
The latest update in git looks good.

Please go ahead and request Fedora GIT access.


Praveen

Comment 9 Praveen K Paladugu 2010-10-06 19:57:52 UTC
Forgot to mention, please upload the final version to a publicly accessible location for future reference.


Praveen

Comment 10 Matt Domsch 2010-10-07 03:28:01 UTC
New Package SCM Request
=======================
Package Name: firmware-extract
Short Description: A firmware-tools plugin to add firmware extraction from vendor binaries
Owners: mdomsch mebrown praveenp
Branches: f12 f13 f14 el4 el5 el6
InitialCC:

Comment 11 Matt Domsch 2010-10-07 03:29:09 UTC
http://domsch.com/linux/fedora/firmware-extract/
has the final SRPM and spec.

Comment 12 Kevin Fenzi 2010-10-08 20:32:55 UTC
Git done (by process-git-requests).


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