Hey Paul, Many thanks for your review, please see some answers below.
On Thu, Oct 13, 2016 at 11:55:29AM +0800, Paul Wise wrote: > > Cc: Oleksij Rempel <li...@rempel-privat.de> > > Please use the X-Debbugs-CC pseudo-header when submitting bugs instead Noted. Nice feature. > > I am looking for a sponsor for my package "open-ath9k-htc-firmware" > > Correct me if I'm wrong but IIRC either yourself or Oleksij have > commit/release access upstream? Well, basically Oleksij co-maintains the upstream repository with Adrian Chadd (former QCA employee). I have commit rights for the packaging branch in Oleksij's personal repo. > The comments for the overrides for lintian tag source-is-missing > should indicate which file is the source for each prebuilt file. I > would have one comment per instance of the tag. In this particular case it's unclear if the source is really missing or not. If you say the upstream sources absolutely MUST be repackaged if they include binaries that we can't get the sources for even if the said binaries are removed automatically before building and are not needed for the process anyhow, well, I'll try to investigate the details and will come back with the answer. > Just one comment saying they are removed at build time is enough for > all of the overrides for lintian tag > source-contains-prebuilt-binary. OK. > Personally, I would suggest removing all of these files from the > upstream git repository and always building them from source. If you > need to keep them, put them in tarballs in the github releases. Not practical, please see below. > Personally, I would recommend the generated files docs/*.png be > removed from the upstream git repo and rendered at build time from > their source SVG files if they are needed. Noted. Probably not anything to worry about if we'll have to repack upstream sources anyway due to proprietary objects, the *.png will be removed along the way. > I think you should also remove the prebuilt files before > dh_auto_configure so that they can never get used by a build, even a > manual one where `debian/rules clean` is not run. ... > I would use a debian/clean file instead of override_dh_clean. Noted. > What is the reason for removing the docs/ and sboot/ directories in > override_dh_clean? AFAICS both of these contain source files. IMO you > should just remove the generated files. Since neither are needed for building the actual firmware, it's easier to remove them completely rather than mess with individual files. The device hardware includes not only the microcontroller but also some RAM and ROM. The tricky part here is that ROM is a mask ROM one, that is, truly read-only. During the development a set of generic useful functions was identified and QCA produced a batch of ICs with those functions present in the mask ROM memory. The main firmware is stored on a host device and is loaded dynamically to target's RAM. To cut down on the amount of RAM needed, it makes use of those generic functions stored in ROM. sboot/ directory contains (probably partial, will have to investigate this further, if absolutely needed; probably repacking the upstream tarball would be easier though) sources for the said ROM as well as the binaries that are supposed to be identical to the ROM's contents. During the build some header files and linker scripts are used to provide external linkage to the generic functions, the said headers and scripts are placed in the target_firmware/ directory, so sboot/ can be removed completely. For some debugging purposes it might be beneficial to inspect the sources of the mentioned functions and sometimes even the resulting binaries. So sboot/ should probably stay in the upstream repository for the reference purposes. Making upstream package it separately sounds like additional hassle for no practical gain (except for the Debian packaging purposes probably), it's way easier to have a single git repository contain everything useful for development and debugging. > The cmake part of the build process does not print compiler > invocation. Debian requires full compiler flags/output in build logs. > For cmake usually debhelper automatically passes > -DCMAKE_VERBOSE_MAKEFILE=ON but it doesn't seem to be working here? Will investigate. > The debian/watch file needs adjusting for the new source package name: > s/firmware-ath9k-htc/open-ath9k-htc-firmware/ > > Personally I would leave "debian uupdate" out of the watch file > because it can be annoying for people who just want to download. Indeed. I'm new to this, so just copied the example from the uscan manpage. > Please get the FSF address corrected in the upstream copyright info. This one makes me wonder if it'd be the correct way to go. I assume only the copyright holder can do this, and spending time contacting eCos developers (and probably lawyers) just to get it right for some really old code seems impractical. I'd consider this to be an upstream issue unrelated to Debian packaging. Please correct me if I'm wrong. > debian/cross-toolchain.mk needs copyright/license info filled in, > preferably in both the header of it and in debian/control. Heh, this is a bit problematic since openbios maintainers missed that part. Should I try contacting them for the clarifications? > The Uploaders field is empty. I would have expected to your name there > if you intend to co-maintain it with Oleksij. Noted. > I would recommend running this command (from the devscripts package) > so that future diffs of the Debian packaging are easier to read: > > wrap-and-sort --short-indent --wrap-always --sort-binary-packages > --trailing-comma > > The Vcs-* fields should point at the repository containing the Debian > packaging, not upstream: > > https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-VCS-fields > > The Conflicts/Suggests fields are empty, you can remove them from > debian/control. > > For merged bugs, you only need to close one of them and the rest will > be closed too. All noted. > What is the reason for renaming the upstream firmware files? Does the > Linux kernel detect the new names? > > What is the reason for debian/ath9k_htc.conf? Why is it > Debian-specific instead of being committed upstream? The situation with versions is complicated. IMHO we'll need to prepare a firmare-ath9k-htc-1.5.0 (including the version in the name) package when 1.5.0 is released. So if end user install it and a suitable kernel, it'll get picked up automatically. For that package no ath9k_htc.conf modrobe file would be needed. For an intermediate version like the one we have now, it makes sense to label it "dev" and to add the file to make it work out of the box. The rationale for this upstream decision on versioning is that the firmware API might need changing for performance and other reasons and a corresponding change introduced to the kernel driver at the same time. One might want to have several different kernel versions installed and those versions might need different ath9k_htc firmware versions. > I'd recommend either passing --parallel at the end of the args to dh, > or upgrading to debian/compat 10, which uses that by default. I've read that compat 10 is an experimental format currently, but if you say it's ok to use it, great. > Please add some upstream metadata: > > https://wiki.debian.org/UpstreamMetadata Noted. > I'd suggest signing all tarballs, tags and commits with OpenPGP and > having uscan verify the tarball sigs: > > https://wiki.debian.org/Creating%20signed%20GitHub%20releases > https://mikegerwitz.com/papers/git-horror-story > https://wiki.debian.org/debian/watch#Cryptographic_signature_verification > https://help.riseup.net/en/security/message-security/openpgp/best-practices Not sure if upstream will agree as it might introduce an additional burden to the development. > $ env PERL5OPT=-m-lib=. cme check dpkg > Warning in 'copyright Format' value > 'http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/': > Format uses insecure http protocol instead of https > you can try 'cme fix dpkg' to fix the warnings shown above Heh, the header was copied verbatim from https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ . Would be nice to have it fixed there too. Thanks again, and happy hacking! -- Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software! mailto:fercer...@gmail.com