Bug 456903

* Wrong URL
* Missing License file

Peter, thanks for pointing out the URL mistake, but what additional license file
are you looking for?

[petro@host-12-116 SPECS]$ LANG=C rpmbuild -ba cxgb3-firmware.spec 
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.55034
+ umask 022
+ cd /usr/src/redhat/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.55034
+ umask 022
+ cd /usr/src/redhat/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.55034
+ umask 022
+ cd /usr/src/redhat/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ /bin/rm -rf /var/tmp/cxgb3-firmware-6.0.0-root
+ /bin/mkdir -p /var/tmp/cxgb3-firmware-6.0.0-root/lib/firmware
+ cd /var/tmp/cxgb3-firmware-6.0.0-root/lib/firmware
+ for foo in t3fw-6.0.0.bin.gz t3b_protocol_sram-1.1.0.bin.gz
t3c_protocol_sram-1.1.0.bin.gz t3b_tp_eeprom-1.1.0.bin.gz t3c_tp_eeprom-1.1.0.bin.gz
+ gunzip -dc /usr/src/redhat/SOURCES/t3fw-6.0.0.bin.gz
++ basename t3fw-6.0.0.bin.gz .gz
++ basename t3fw-6.0.0.bin.gz .gz
+ chmod 0644 t3fw-6.0.0.bin
+ for foo in t3fw-6.0.0.bin.gz t3b_protocol_sram-1.1.0.bin.gz
t3c_protocol_sram-1.1.0.bin.gz t3b_tp_eeprom-1.1.0.bin.gz t3c_tp_eeprom-1.1.0.bin.gz
+ gunzip -dc /usr/src/redhat/SOURCES/t3b_protocol_sram-1.1.0.bin.gz
++ basename t3b_protocol_sram-1.1.0.bin.gz .gz
++ basename t3b_protocol_sram-1.1.0.bin.gz .gz
+ chmod 0644 t3b_protocol_sram-1.1.0.bin
+ for foo in t3fw-6.0.0.bin.gz t3b_protocol_sram-1.1.0.bin.gz
t3c_protocol_sram-1.1.0.bin.gz t3b_tp_eeprom-1.1.0.bin.gz t3c_tp_eeprom-1.1.0.bin.gz
+ gunzip -dc /usr/src/redhat/SOURCES/t3c_protocol_sram-1.1.0.bin.gz
++ basename t3c_protocol_sram-1.1.0.bin.gz .gz
++ basename t3c_protocol_sram-1.1.0.bin.gz .gz
+ chmod 0644 t3c_protocol_sram-1.1.0.bin
+ for foo in t3fw-6.0.0.bin.gz t3b_protocol_sram-1.1.0.bin.gz
t3c_protocol_sram-1.1.0.bin.gz t3b_tp_eeprom-1.1.0.bin.gz t3c_tp_eeprom-1.1.0.bin.gz
+ gunzip -dc /usr/src/redhat/SOURCES/t3b_tp_eeprom-1.1.0.bin.gz
++ basename t3b_tp_eeprom-1.1.0.bin.gz .gz
++ basename t3b_tp_eeprom-1.1.0.bin.gz .gz
+ chmod 0644 t3b_tp_eeprom-1.1.0.bin
+ for foo in t3fw-6.0.0.bin.gz t3b_protocol_sram-1.1.0.bin.gz
t3c_protocol_sram-1.1.0.bin.gz t3b_tp_eeprom-1.1.0.bin.gz t3c_tp_eeprom-1.1.0.bin.gz
+ gunzip -dc /usr/src/redhat/SOURCES/t3c_tp_eeprom-1.1.0.bin.gz
++ basename t3c_tp_eeprom-1.1.0.bin.gz .gz
++ basename t3c_tp_eeprom-1.1.0.bin.gz .gz
+ chmod 0644 t3c_tp_eeprom-1.1.0.bin
+ /usr/bin/install -m 0644 /usr/src/redhat/SOURCES/LICENSE-t3fw
/var/tmp/cxgb3-firmware-6.0.0-root/lib/firmware/LICENSE-t3fw
/usr/bin/install: cannot stat `/usr/src/redhat/SOURCES/LICENSE-t3fw': No such
file or directory
error: Bad exit status from /var/tmp/rpm-tmp.55034 (%install)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.55034 (%install)
[petro@host-12-116 SPECS]$

Whoops!

Updated SPEC file:
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware.spec

Update SRPM:
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-2.src.rpm

Thanks, Peter.

IANAL, but seems that this Review Request would block Fedora Legal. 

/me summoning dark wizards of law!

Peter,  I've taken care of legal concerns between Red Hat/Fedora and Chelsio
already -- I did that before creating the BZ.

Sooooo, any technical thoughts on my latest rpms from comment #4?

Anyone else who's listening want to review these?  Bill?

Just to clarify why I'm not reviewing this:

* I have no rights to view BZ# 188271 (so I can't see the whole picture of current situation with this Review Request).
* FE-Legal still blocks by this package (If everything OK, then you should unblock it with some comments explaining why).
* This package is for RHEL only, but I personally think that new package should go to Rawhide first, and only after that it should be pushed down (F9, F8, EL5, EL4).

(In reply to comment #9)
> Just to clarify why I'm not reviewing this:
> 
> * I have no rights to view BZ# 188271 (so I can't see the whole picture of
> current situation with this Review Request).

It's a RHEL tracking bug.  Knowing it's full contents does not block you from reviewing this package and indicating whether or not the spec-file looks fine and whether or not the files are placed in an acceptable location on installation.

> * FE-Legal still blocks by this package (If everything OK, then you should
> unblock it with some comments explaining why).

I've already sent the needed into to Spot and he agrees that we have everything in order.  I will leave it up to him to drop the tracker just to 'honor the process.'

> * This package is for RHEL only, but I personally think that new package should
> go to Rawhide first, and only after that it should be pushed down (F9, F8, EL5,
> EL4).

I agree and I plan to push it there as well.

Well, the license tag is wrong... it should be:

  License: Redistributable, no modification permitted

Aside from that, its permissable for Fedora (it is firmware, see: http://fedoraproject.org/wiki/Licensing#Binary_Firmware )

Lifting FE-Legal.

Cannot set up fedora-review flags. I guess that probably only redhat employeers could review RHEL-related requests, but I'm not sure.

Should we need a RHEL review guidelines?

Thanks, Tom.  I uploaded new files with updated language:

http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware.spec
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-3.src.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-3.noarch.rpm

Notes: 

* Pretty nice example of "Catch 22"! License clearly says - "READ THIS BEFORE INSTALLING, LOADING OR OTHERWISE USING THIS SOFTWARE." It's hard to know that you must read this license before installing this package until you're actually installed it. :) 

Probably this is a blocker since every attempt to install this proprietary blob renders you to violation this license.

* Macro %sourcefiles used only once, thus seems quite useless for me.

* Ungzipping of blobs should be placed in %prep, but it's just a cosmetic.

* No need to install license - adding it to %doc would be enough. Right now it listed twice (in %docs and among files in /lib/firmware), and that's a blocker.

(In reply to comment #14)
> Notes: 
> 
> * Pretty nice example of "Catch 22"! License clearly says - "READ THIS BEFORE
> INSTALLING, LOADING OR OTHERWISE USING THIS SOFTWARE." It's hard to know that
> you must read this license before installing this package until you're actually
> installed it. :) 

Aha, there is something missing here. I went back and looked closely at this, and we actually got an alternate license grant from Chelsio. Andy, you should pull out that license text and replace it with the terms from the CEO's letter.

Man, this is starting to look like a comedy of errors.  I thought I'd done that originally, but upon further inspection I did not.

So what's the status of this Package Review?
RHEL 5.3 Beta Freeze is Sep 19 when all errata must be filed and given to QE.

I'll wait until Andy provides us srpm with corect license. I hope it will be soon.

(In reply to comment #14)
> Notes: 
> 
> * Pretty nice example of "Catch 22"! License clearly says - "READ THIS BEFORE
> INSTALLING, LOADING OR OTHERWISE USING THIS SOFTWARE." It's hard to know that
> you must read this license before installing this package until you're actually
> installed it. :) 
> 
> Probably this is a blocker since every attempt to install this proprietary blob
> renders you to violation this license.

Changed working in LICENSE.cxgb3-firmware file to what we wanted to use originally.

> * Macro %sourcefiles used only once, thus seems quite useless for me.

It isn't incredibly useful, but when I need to update to new firmware or other files it will easy for me to change it there.

> * Ungzipping of blobs should be placed in %prep, but it's just a cosmetic.

I didn't feel like unzipping them to one spot and then copying them somewhere else in install.  It seemed to make more sense to unzip them to the installed location, so I left it as it was before.

> * No need to install license - adding it to %doc would be enough. Right now it
> listed twice (in %docs and among files in /lib/firmware), and that's a blocker.

Fixed.

Thanks for reviewing this, Peter!

Updates are available here:

http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-4.noarch.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-4.src.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware.spec

(In reply to comment #19)

> Changed working in LICENSE.cxgb3-firmware file to what we wanted to use
> originally.

OK.

> > * Macro %sourcefiles used only once, thus seems quite useless for me.
> 
> It isn't incredibly useful, but when I need to update to new firmware or other
> files it will easy for me to change it there.

In this case you should manipulate with %{SOURCEX} directly. See below.

> > * Ungzipping of blobs should be placed in %prep, but it's just a cosmetic.
> 
> I didn't feel like unzipping them to one spot and then copying them somewhere
> else in install.  It seemed to make more sense to unzip them to the installed
> location, so I left it as it was before.

From my POV (if I understood these things correctly) the building of package consists from three main phases - preparation of all sources in one place (%prep), building sources (%build, which left blank in this case) and installation (and only installation) of what we currenly have. According to this phylosophy, I'd better unpack all stuff in %prep, separately from %install phase, rather than combining them.

I propose changing spec in the following way:

===============================================

%prep
%setup -c %{name}-%{version} -T
cp %{SOURCE0} %{SOURCE1} %{SOURCE2} %{SOURCE3} %{SOURCE4} %{SOURCE5} .
gzip -d *.gz

%build

%install
%{__rm} -rf %{buildroot}
# Install all firmware files
mkdir -p %{buildroot}/lib/firmware
install -p -m 0644 t3[fbc]* %{buildroot}/lib/firmware

%clean
%{__rm} -rf %{buildroot}

%files
%defattr(-, root, root, 0755)
%doc LICENSE.cxgb3-firmware
/lib/firmware/*

===============================================



I also recommend you to add this line in the top of your spec-file:

%define debug_package %{nil}


Although it changes nothing in the resulting spec, this one-liner cleans ~/rpmbuild/BUILD/%{name}-%{version} directory from annoying bogus debug-related files. It's just cosmetic - this particular advice may be safely ignored.



Another one note - you still don't eeprom-version and sram-version macros. Consider either applying them or removing from spec.

One important note - I will be totally offline from this Friday, so, please responce ASAP. Sorry for this hurry.

Thanks for the feedback.  I'll make the changes.  It's a but frustrating since I looked at existing firmware srpms and used them as a model, but that's fine....

> It's a but frustrating since I looked at existing firmware srpms

Large amount of Fedora/RHEL rpms didn't pass any reviews by the historical reasons - that's why "Merge Reviews" exists.

OK, another shot.  I'm by no means and spec-file expert (obviously) so the tips are really helpful.  Thanks, Peter.

Here's one more pass. :-)

http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware.spec
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-5.src.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-5.noarch.rpm

Too sad. but I still cannot set flags properly. In any case there is my 

REVIEW:

- rpmlint is not silent.

[petro@Sulaco SPECS]$ rpmlint ../RPMS/noarch/cxgb3-firmware-6.0.0-5.fc9.noarch.rpm 
cxgb3-firmware.noarch: E: non-readable /usr/share/doc/cxgb3-firmware-6.0.0/LICENSE.cxgb3-firmware 0600
cxgb3-firmware.noarch: W: incoherent-version-in-changelog 6.0.0-4 6.0.0-5.fc9
1 packages and 0 specfiles checked; 1 errors, 1 warnings.
[petro@Sulaco SPECS]$

[petro@Sulaco SPECS]$ rpmlint ../SRPMS/cxgb3-firmware-6.0.0-5.fc9.src.rpm 
cxgb3-firmware.src: W: strange-permission LICENSE.cxgb3-firmware 0600
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
[petro@Sulaco SPECS]$

* Please chmod 644 LICENSE.cxgb3-firmware before packaging it. Easy to fix.
* incoherent-version-in-changelog  means that however your package has version 6.0.0-5, latest changelog entry in spec-file still mentions about 6.0.0-4. Likewise, easy to fix.

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
- The package meets the Packaging Guidelines except the following two isses:

* Url field is pretty useless. When I tried to visit it, I've got the following message:

                                                                                                                                                                                                                                                                                                              403 Forbidden
                                                                                                                                                         Forbidden

   You don't have permission to access /drivers/firmware/t3/ on this server.
     _____________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________


    Apache/2.0.52 (Red Hat) Server at service.chelsio.com Port 80

* You should use absolute paths to sources. E.g. 

Source0: http://service.chelsio.com/drivers/firmware/t3/t3fw-%{version}.bin.gz

and not only 

Source0: t3fw-%{version}.bin.gz

And so on...
These two issues are also easy to fix.

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The text of the license(s) is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package must matches the upstream source:

[petro@Sulaco SOURCES]$ md5sum t3*
848535ef1e51a13c1ca6b6aafc39692a  t3b_protocol_sram-1.1.0.bin.gz
848535ef1e51a13c1ca6b6aafc39692a  t3b_protocol_sram-1.1.0.bin.gz.orig
bf7ba63b708bb8195d3f5f6b648516b9  t3b_tp_eeprom-1.1.0.bin.gz
bf7ba63b708bb8195d3f5f6b648516b9  t3b_tp_eeprom-1.1.0.bin.gz.orig
1bc3a4c572b353a4eb533f8fa7036ca1  t3c_protocol_sram-1.1.0.bin.gz
1bc3a4c572b353a4eb533f8fa7036ca1  t3c_protocol_sram-1.1.0.bin.gz.orig
d89ee69e368c5443a0d1e416f74204e5  t3c_tp_eeprom-1.1.0.bin.gz
d89ee69e368c5443a0d1e416f74204e5  t3c_tp_eeprom-1.1.0.bin.gz.orig
7f726afb3cacf8a6373ce991f5f086a7  t3fw-6.0.0.bin.gz
7f726afb3cacf8a6373ce991f5f086a7  t3fw-6.0.0.bin.gz.orig
[petro@Sulaco SOURCES]$ 

+ The package successfully compiles and builds into binary rpms on at least one supported architecture (ppc).
+ No build dependencies except for any that are listed in the exceptions section of the Packaging Guidelines.
+ No locales.
+ No shared library files
+ The package isn't designed to be relocatable.
- A package owns all directories that it creates (none, actually). It does not create a directory that it uses (/lib/firmware), so it should require a package which does create that directory (udev, see rpm -qf /lib/firmware). Quite senseless, but that's a Fedora Guidelines, so, please consider adding udev - simply add the following line:

Requires: udev

+ The package does not contain any duplicate files in the %files listing.
- Permissions on files are set properly, except LICENSE.cxgb3-firmware, see notes above about rpmlint messages. Please change it to 644.
+ The package has a %clean section, which contains rm -rf %{buildroot}.
+ The package consistently uses macros, as described in the macros section of Packaging Guidelines .
+ The package contains code, or permissable content.
+ No large documentation files.
+ All the package includes as %doc (LICENSE.cxgb3=firmware), does not affect the runtime of the application. 
+ No header files.
+ No static libraries.
+ No containing pkgconfig(.pc) files.
+ No library files with a suffix.
+ No need for devel packages
+ The package does not contain any .la libtool archives.
+ Not a  GUI application
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot}
+ All filenames in rpm packages are valid UTF-8.

Please change your spec in order to fix issues, mentioned above. 

This package is


APPROVED.

Updated as requested (ran this on an F9 box so I could use the latest rpmlint)

http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-6.fc9.noarch.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware-6.0.0-6.fc9.src.rpm
http://people.redhat.com/agospoda/cxgb3-firmware/cxgb3-firmware.spec

BTW just found that they ships firmware ver. 7.0.0 - do you plan to update this package to the latest version?

Not right now.  The current RHEL driver requires the older (6.0.0) firmware. :-)

> The current RHEL driver requires the older (6.0.0) firmware.

And what about Fedora branches?

Fedora and devel branches will need to stay in sync with whatever is shipping upstream....

Any progress since mid September?