Bug 230096 - Review Request: iwl3945-firmware - Firmware for Intel� PRO/Wireless 3945 A/B/G network adaptors
Summary: Review Request: iwl3945-firmware - Firmware for Intel� PRO/Wireless 3945 A/...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Todd Zullinger
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-02-26 16:37 UTC by Matthias Saou
Modified: 2007-11-30 22:11 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-09-11 03:16:08 UTC
Type: ---
Embargoed:
tmz: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Matthias Saou 2007-02-26 16:37:29 UTC
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-ucode/iwlwifi-ucode.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-ucode/iwlwifi-ucode-2.14.1-1.src.rpm
Description:
This package contains the microcode required by the iwlwifi driver for Linux.
Usage of the firmware is subject to the terms and conditions contained
inside the provided LICENSE.iwlwifi-ucode file. Please read it carefully.

Comment 1 Ville Skyttä 2007-02-26 16:54:47 UTC
Even though the upstream tarball is named iwlwifi-ucode, calling the package
iwlwifi-firmware (with Provides: iwlwifi-ucode if seen useful) would be IMO be a
better choice for consistency with other similar packages.

Comment 2 Matthias Saou 2007-02-26 17:03:34 UTC
He he, exactly what I just asked in bug #217351 (the ipw2200-firmware review) :-)

If we think that what is contained in these packages isn't "wrongly" named when
being referred to as "firmware", then sure, we could call all of them "firmware".

Comment 3 Matthias Saou 2007-02-26 17:07:32 UTC
http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-firmware/iwlwifi-firmware.spec
http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-firmware/iwlwifi-firmware-2.14.1-2.src.rpm

* Mon Feb 26 2007 Matthias Saou <http://freshrpms.net> 2.14.1-2
- Initial RPM release.
- Rename from -ucode to -firmware.

Comment 4 Xavier Lamien 2007-02-26 20:49:23 UTC
your %changelog section doesn't quite good.

When made change and incrase your release, you must add a new one in changelog.
such as :

* Mon Feb 26 2007 user_name <email_adress> - 2.14.1-2
- Rename from -ucode to -firmware.

* Mon Feb 26 2007 user_name <email_adress> - 2.14.1-1
- Initiale RPM release.

I just had a look on your spec file and its looks to need some fix to meet the
packaging guidlines.

Other comment will within few hours.

Comment 5 Xavier Lamien 2007-02-27 03:44:13 UTC
So,

MUST Fix: release tag doesn't good, from what i written above, it's your second  
          official build for extras review and and correctly set to 2 but, must 
            be followed by distag %{?dist}.
MUST Fix: License tag is invalid
          Just use "Distributable"

MUST Fix: Group tag is invalid.
          Use "System Environment/Kernel" instead of "firmware"

SHOULD Fix: BuildRoot tag doesn't quite good.
            Use 
            %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
            instead of
            %{_tmppath}/%{name}-%{version}-%{release}-root

Typo:   # This is so that the noarch packages only appears for these archs
        And what about pcc arch ?

SHOULD:  just for the form, in %build section add comment "#nothing to build"
         for a clean review.

SHOULD:  In %install section.
         use "rm -rf" instead of %{__rm} -rf.
         same thing, use "install -p -Dm 0644" instead of %{__install} -D -p -m
0644.

SHOULD Fix:  Before copy a file to the right location, you must create 
             this one by using "mkdir -p %{buildroot}%{_lib}/firmware" command.
             (even if the -D option do the same thing).

MUST Fix:  Use "%{_lib}/firmware/*.ucode" instead of "/lib/firmware/*.ucode"

MUST Fix: %changelog -> see Comment#3.

Comment 6 Xavier Lamien 2007-02-27 03:46:44 UTC
typo: it's Comment#4

Comment 7 Bill Nottingham 2007-02-27 04:25:35 UTC
(In reply to comment #5)
> MUST Fix: License tag is invalid
>           Just use "Distributable"

Please read the firmware discussion on fedora-packaging.

> Typo:   # This is so that the noarch packages only appears for these archs
>         And what about pcc arch ?

This driver is invalid on PPC.

> MUST Fix:  Use "%{_lib}/firmware/*.ucode" instead of "/lib/firmware/*.ucode"

That's invalid for firmware. /lib is correct.

Comment 8 Matthias Saou 2007-02-27 09:54:12 UTC
Xavier : Being pedantic isn't always the best thing. You use the word "must" in
many places, some of which should be "should" instead, and others more
importantly, where you are plain wrong. For instance "you must use a %{?dist}
tag" is incorrect, and in this case, it's _deliberate_ to not use one, since it
allows hardlinking the package across multiple releases.

Your only valid comment is the one about the %changelog, but please realize that
it's pretty much useless to have multiple entries for the same day, especially
if they're so minor. The wrong thing would have been not to increment the release.

Packaging rules and guidelines are something really useful, but nothing will
ever beat using common sense as much as possible.

Comment 9 Xavier Lamien 2007-02-27 12:59:06 UTC
Hi Matthias,

indeed, there's some things can be ignored in review and from rpmlint ouput error.

> That's invalid for firmware. /lib is correct.

for %{_lib}, it works for me and no build error.

> Typo:   # This is so that the noarch packages only appears for these archs
>         And what about pcc arch ?
>
> This driver is invalid on PPC.

So, this : https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=179260
    should be read.


> For instance "you must use a %{?dist}
> tag" is incorrect, and in this case, it's _deliberate_ to not use one, since it
> allows hardlinking the package across multiple releases.

I understand that. also the fact it's a noarch package.
Now i wonder if it's accepted in CVS build procedure.

> Your only valid comment is the one about the %changelog, but please realize that
> it's pretty much useless to have multiple entries for the same day, especially
> if they're so minor. The wrong thing would have been not to increment the release.

I a little bit agree with you about that (i don't make multiple entries for the
same day when i build my own packages) but, it's important for review to avoid
confusions and to follow the work (changes, modification, ...) of the owner of
the package.

however, I maintains that the Group tag isn't good.


Comment 10 Matthias Saou 2007-02-27 13:08:52 UTC
(In reply to comment #9)

> > That's invalid for firmware. /lib is correct.
> 
> for %{_lib}, it works for me and no build error.

But %_lib = lib64 on x86_64, so your firmware file will end up in the wrong
place, since it needs to be in /lib/firmware on x86_64, not in /lib64/firmware,
pretty much like the kernel modules needs to be in /lib/modules/...

> > This driver is invalid on PPC.
> 
> So, this : https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=179260
>     should be read.

It's not that is doesn't build or work on ppc. It's just that it doesn't make
sense because it's a firmware for hardware which is only to be found in x86 and
x86_64 hardware (actually, almost certainly only x86_64 hardware, but since you
can install an x86 OS on it, it also makes sense to have available for the x86 OS).

> > For instance "you must use a %{?dist}
> > tag" is incorrect, and in this case, it's _deliberate_ to not use one, since it
> > allows hardlinking the package across multiple releases.
> 
> I understand that. also the fact it's a noarch package.
> Now i wonder if it's accepted in CVS build procedure.

Of course it is. You seem to be confusing a lot of "mandatory" vs. "suggested"
packaging points.

> > Your only valid comment is the one about the %changelog, but please realize that
> > it's pretty much useless to have multiple entries for the same day, especially
> > if they're so minor. The wrong thing would have been not to increment the
release.
> 
> I a little bit agree with you about that (i don't make multiple entries for the
> same day when i build my own packages) but, it's important for review to avoid
> confusions and to follow the work (changes, modification, ...) of the owner of
> the package.

You are right, but one should realize that this is pretty much irrelevant.

> however, I maintains that the Group tag isn't good.

It's a work in progress. Some specific packaging rules for firmwares are being
discussed right now. As of this very instant, it's the correct one. It might
change, in which case I'll change it too.


Comment 11 Xavier Lamien 2007-02-27 13:33:10 UTC
> But %_lib = lib64 on x86_64, so your firmware file will end up in the wrong
> place, since it needs to be in /lib/firmware on x86_64, not in /lib64/firmware,
> pretty much like the kernel modules needs to be in /lib/modules/...

indeed.


> It's not that is doesn't build or work on ppc. It's just that it doesn't make
> sense because it's a firmware for hardware which is only to be found in x86 and
> x86_64 hardware (actually, almost certainly only x86_64 hardware, but since you
> can install an x86 OS on it, it also makes sense to have available for the x86
OS).

OK ;-)

> You seem to be confusing a lot of "mandatory" vs. "suggested"
packaging points.

A lil' bit, you don't explicitly comment on you fisrt post the use of many thing
that doesn't match with packaging guidlines and it's why I put myself questions  ;-)

> It's a work in progress. Some specific packaging rules for firmwares are being
> discussed right now. As of this very instant, it's the correct one. It might
> change, in which case I'll change it too.

ok, i'll follow this discussion.

So, if we agree with all things from this pacakge, a full review can be done ?

Comment 12 Matthias Saou 2007-02-27 13:38:58 UTC
(In reply to comment #11)

> So, if we agree with all things from this pacakge, a full review can be done ?

If you see anything in the package that is mandatory to change or fix, you can
just say so now. But for the complete review and possible approval of the
package, it's best to wait for the outcome of the firmware packaging discussions.

Comment 13 Matthias Saou 2007-03-05 15:21:20 UTC
http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-firmware/

* Mon Mar  5 2007 Matthias Saou <http://freshrpms.net> 2.14.1-3
- Change group and license fields to reflect latest firmware guidelines.
- Replace "microcode" with "firmware" in summary and description.

Comment 14 Todd Zullinger 2007-03-15 18:55:51 UTC
Hi Matthias,

This package looks good.  Here's a review.  (I have one of these cards so I'm
looking forward to seeing a proper driver included in F7 (or so).  Thanks for
packaging the firmware!)

OK - rpmlint output

  $ rpmlint iwlwifi-firmware-2.14.1-3.src.rpm
  W: iwlwifi-firmware invalid-license Redistributable, no modification permitted
  E: iwlwifi-firmware hardcoded-library-path in
%{buildroot}/lib/firmware/iwlwifi-3945.ucode
  E: iwlwifi-firmware hardcoded-library-path in /lib/firmware/*.ucode
   
  All of these can be safely ignored for firmware packages

OK - follows naming guidelines (for firmware packages)
OK - specfile matches base package %{name}
OK - meets the Packaging Guidelines
OK - license is acceptable (for firmware packages)
OK - licence field is appropriate (for firmware packages)
OK - license file is included in %doc
OK - specfile is in American English
OK - specfile is legible
OK - source matches upstream (sha1: 29e90e7a6c5fe06eaad3780235a77ce0411d132f)
OK - package contains no duplicate files
OK - permissions on files set correctly
OK - specfile has proper %clean section
OK - consistent use of macros
OK - contains code or permissible content
OK - %doc files don't affect runtime
OK - doesn't own files or directories already owned by other packages

ACCEPT

Comment 15 Matthias Saou 2007-03-15 19:24:17 UTC
Thanks for your review, but this package isn't acceptable for inclusion into
Fedora until the kernel driver has been included, since it is mandatory for
"content" to go in, to have it be used by something that is part of the distro.

So, as soon as the Fedora development kernel gets the driver, I'll do the CVS
request and build the package.

Otherwise, maybe an exception could be made, because the driver (IIRC) is in
John Linville's kernels... but that's not up to me, and people who can get a
kernel from an external location can also get a firmware anyway ;-)

Comment 16 Nicolas Chauvet (kwizart) 2007-03-15 19:26:41 UTC
From https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228865#c13
From John W. Linville
Please try the test kernels here:

   http://people.redhat.com/linville/kernels/fc7/

These have the iwlwifi driver built-in.

Since this drivers isn't currently integrated into the official kernel nor the
fedora kernel (it could possibly be integrated into the fedora before the
vanilla kernel but it is more a question...)

I should add: both wpa_supplicant (wpa encryption) and hostapd (allowing master
mode for wireless devices) need to be patched or enabled for mac80211 stack, so
it will be possible to uses what's bring the mac80211 stack (exept for the
binary daemon question...)

Quick references:
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=230731
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=230449
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=230857

Comment 17 Todd Zullinger 2007-03-15 19:40:49 UTC
Bah, sorry I didn't know that.  It's rather a little chicken and egg like it
seems.  The driver is useless without the firmware, so I figured that getting
the firmware in could come first.

Comment 18 Todd Zullinger 2007-03-19 05:34:15 UTC
So does this make the firmware acceptable?

$ (repoquery --repoid=development --changelog kernel.i686; \
   repoquery --repoid=development -l kernel.i686) | grep -B1 'iwlwifi[ /]'
* Fri Mar 16 2007 John W. Linville <linville>
- Add snapshot of iwlwifi driver from www.intellinuxwireless.org
--
/lib/modules/2.6.20-1.2997.fc7/kernel/drivers/net/wireless/mac80211/iwlwifi
/lib/modules/2.6.20-1.2997.fc7/kernel/drivers/net/wireless/mac80211/iwlwifi/iwlwifi.ko


Comment 19 Matthias Saou 2007-03-19 09:15:07 UTC
I guess it does! :-)
Please confirm the review, and I'll make sure it gets into devel ASAP.

Comment 20 Todd Zullinger 2007-03-19 15:55:39 UTC
Confirmed.  Hopefully I have not missed any other details in my review. :)

Comment 21 Matthias Saou 2007-03-19 16:03:19 UTC
New Package CVS Request
=======================
Package Name: iwlwifi-firmware
Short Description: Firmware for Intel® PRO/Wireless 3945 A/B/G network adaptors
Owners: matthias
Branches: devel
InitialCC: 

Comment 22 Matthias Saou 2007-07-15 16:25:57 UTC
With the appearance of the new iwlwifi-4965-firmware (see bug #248224), this
package will need to be renamed from iwlwifi-firmware to iwlwifi-3945-firmware.

What should I do? Reopen this bug and request CVS changes here by setting the
flag to "?" again?

I've put the new files here (iwlwifi-3945-firmware-2.14.4-1):
http://ftp.es6.freshrpms.net/tmp/extras/iwlwifi-3945-firmware/

Comment 23 Nicolas Chauvet (kwizart) 2007-07-15 16:49:25 UTC
%description
This package contains the firmware required by the iwlwifi driver for Linux.
Should be ?!:
This package contains the firmware required by the iwl3945 driver for Linux.

Well, since it is true that the archive name is iwlwifi from intelwireless, the
driver used inside the kernel is named iwl3945.ko


Comment 24 Matthias Saou 2007-07-15 17:02:12 UTC
(In reply to comment #23)

> Should be ?!:
> This package contains the firmware required by the iwl3945 driver for Linux.

Fixed :-) (didn't bump the release for such a small change)

Comment 25 John W. Linville 2007-07-24 16:58:55 UTC
notting makes a good point here:

   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=248224#c10

Perhaps iwl3945-firmware is a better name?

Comment 26 Matthias Saou 2007-07-24 17:08:11 UTC
Yes, absolutely! Here is the re-renamed package :

http://ftp.es6.freshrpms.net/tmp/extras/iwl3945-firmware/

All we need to do now is :
1) Get the iwl4965-firmware package passed review.
2) Get iwlwifi-firmware (this package) renamed (cvs, bugzilla... the "usual").
3) Push both at the same time to at least devel (but possibly F-7 too).

Comment 27 Nicolas Chauvet (kwizart) 2007-07-24 21:40:22 UTC
Notting suggested that since the new iwl4965-firmware was not bundled with
iwlwifi-firmware, there is no need to have Provides: iwlwifi-firmware for
iwl4965-firmware also...
Thats a matter of choice, maybe tha'ts easier...
Iv'e tryed to experiment versionned firmwares (using alternatives methode, and
this should work also with a patched kernel using versionned firmwares...)

Comment 28 Matthias Saou 2007-08-22 09:25:50 UTC
New package, which includes the firmware version still required for the F7
kernels (and current devel?), and also the very latest version, probably
required soon by the devel kernels if not already :

http://ftp.es6.freshrpms.net/tmp/extras/iwl3945-firmware/iwl3945-firmware.spec
http://ftp.es6.freshrpms.net/tmp/extras/iwl3945-firmware/iwl3945-firmware-2.14.1.5-1.src.rpm

And I'm reopening the bug to make it easier to track it. I'll be asking for the
CVS rename now.

Comment 29 Matthias Saou 2007-08-22 09:30:33 UTC
Package Change Request
======================
Package Name: iwlwifi-firmware

The package need to be renamed to iwl3945-firmware per upstream split (iwl3945
and iwl4965). Please make the required changes, then I'll commit the new spec
files with the changed name and rebuild new packages for the current branches.

Comment 30 Warren Togami 2007-08-22 16:49:32 UTC
By the new procedure, we will no longer rename CVS modules.  I have added
iwl3945-firmware as a new package owned by you.  Please use the dead.package
procedure in the old CVS module.


Comment 31 Nicolas Chauvet (kwizart) 2007-09-11 00:35:20 UTC
This bug should be closed now ...


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