Red Hat Bugzilla – Bug 230096
Review Request: iwl3945-firmware - Firmware for Intel� PRO/Wireless 3945 A/B/G network adaptors
Last modified: 2007-11-30 17:11:58 EST
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.
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.
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".
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.
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.
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.
typo: it's Comment#4
(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.
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.
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.
(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.
> 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 ?
(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.
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.
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
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 ;-)
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
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.
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@redhat.com> - 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
I guess it does! :-) Please confirm the review, and I'll make sure it gets into devel ASAP.
Confirmed. Hopefully I have not missed any other details in my review. :)
New Package CVS Request ======================= Package Name: iwlwifi-firmware Short Description: Firmware for Intel® PRO/Wireless 3945 A/B/G network adaptors Owners: matthias@rpmforge.net Branches: devel InitialCC:
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/
%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
(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)
notting makes a good point here: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=248224#c10 Perhaps iwl3945-firmware is a better name?
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).
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...)
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.
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.
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.
This bug should be closed now ...