Control: tags -1 moreinfo Hi Paul,
Thanks for your very detailed review of carl9170fw. I'm still making my changes to the package and will give you a poke and remove the moreinfo tag once I have an upload ready for re-review. > I don't think udebs are needed for firmware packages, none of the other > WiFi firmware packages in Debian have them. You might like to have a look at this mail from Ben Hutchings: https://lists.debian.org/msgid-search/179d6d32466dd13962a3aab251c45242fbf2d8ae.ca...@decadent.org.uk The reason that none of the other Wi-Fi firmware packages have them is because they're all non-free (with the exception of ath9k_htc). My understanding—which was most definitely not articulated by Ben—is that the Debian Installer has a mechanism for loading the (non-free) firmware from the ordinary .debs. Since the installer needs to have logic to look for non-free firmware on removable media at runtime anyway, that's quite a different situation to most that warrant udebs, where the udeb is a "built-in" of the installer. FYI, open-ath9k-htc-firmware is in NEW because my most recent upload likewise adds a udeb for it. > If the package is actually needed it should be named -udeb not -di, > since other udebs use -udeb. I wasn't sure if there was any established convention; my thread "Naming convention for udebs: -udeb/-installer suffix" didn't garner any pertinent responses. I've switched the name though. > Several files have missing/incorrect information in debian/copyright, > please do a full audit of the code looking for copyright/license info. > > * tools/include/list.h is LGPL > * tools/include/frame.h is partly from Linux, unknown copyright > * include/shared/eeprom.h also contains ISC code These are all good catches, I'm working on incorporating them and doing a slow and careful review. > DEB_BUILD_OPTION_PARALLEL doesn't appear to be a standard variable, > please switch to DEB_BUILD_OPTIONS=parallel=N instead, see Debian > Policy, section 4.9.1 and debhelper(7) and dpkg-buildpackage(1). I think you're mistaken here, you should take a look at /usr/share/dpkg/buildopts.mk which I include via an include directive in debian/rules. Basically, DEB_BUILD_OPTION_PARALLEL is a helper macro for the value of parallel from DEB_BUILD_OPTIONS; these are one and the same. There's a chance of the DEB_BUILD_OPTION_PARALLEL Makefile macro still being unset, so what this line does in my Makefile: DEB_BUILD_OPTION_PARALLEL ?= 1 is set it to 1 if it's not set in one's DEB_BUILD_OPTIONS. That way, calls like make -j$(DEB_BUILD_OPTION_PARALLEL) won't become make -j which would be very bad. > Some things that would be nice to fix at some stage: > > Nothing in debian/rules references .config so you can drop that from > before the execute_before_dh_auto_configure target. That's true. My intent was that, since it's a hidden directory, it would help remind one that that directory gets created. It seems to've only caused confusion, so I'll pull it. > It seems like the Homepage should be the carl9170.fw firmware wiki page > instead of the carl9170 driver wiki page. > > https://wireless.wiki.kernel.org/en/users/drivers/carl9170.fw Good catch, I will fix that. > I would express the license of include/shared/fwcmd.h etc as GPL-2-only > && ISC rather than putting a copy of the ISC license in a comment. I agree that this is sensible. > bug-presubj isn't well wrapped. I'm not sure if wrapped or unwrapped is > the best option for this file though. Indeed, the documentation is rather old and terse and doesn't address this issue. I'll probably ask the Reportbug folks and send them a MR updating the docs. > Please ask upstream to make a new release, it has been a very long time > since the last one. I will do after making some of the following important changes. > Please ask upstream to update their copies of code from the Linux > kernel and other external sources to the latest versions. I can probably help them with this. > Please ask upstream to send FindUSB-1.0.cmake & libusb-zeropacket.diff > to libusb upstream and then remove them from carl9170fw. I will ask, but with all due respect, I think this is lower priority and that I'll have limited ability to help them. > Please ask upstream to delete FindPackageHandleStandardArgs.cmake, > since that is now available from cmake upstream and from Debian cmake. > Potentially cmake_minimum_required will need to be bumped for this. Will do. > Please ask upstream to include the copyright information > for carlfw/src/memcpy.S and carlfw/src/memset.S in the files. Especially since it is copyleft, I will definitely prioritize this. > Please ask upstream to update the COPYRIGHT file. I will be happy to do this. > Please send upstream some changes that would allow building the > upstream source using a pre-packaged toolchain like the Debian one. > > Please also figure out how to eliminate the other debian/rules > workarounds. I do not have a grasp, let alone a good one, of CMake, so this may be a challenge. With respect to the former, searching for sh-elf-gcc in $PATH ought to be straightforward though. > Please add an upstream metadata file: > https://wiki.debian.org/UpstreamMetadata I actually think I'll do one better: I submitted upstream an AppStream metadata file a while ago, and although they haven't responded to it yet, perhaps my sending them other changes will get their attention. AppStream metadata and Debian upstream metadata files have considerable overlap, and in my personal opinion having good AppStream metadata makes an upstream metadata file unnecessary, but I'm willing to entertain arguments to the contrary. > I suggest these arguments to wrap-and-sort: > wrap-and-sort --short-indent --wrap-always --sort-binary-packages > --trailing-comma --dry-run After I'm done with everything else, I'll give this a whirl. Thanks again for everything!
signature.asc
Description: This is a digitally signed message part
