Bug#840515: RFS: open-ath9k-htc-firmware/1.4.0-25-gf6af791-1 ITP #751339
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 <linux@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:fercerpav@gmail.com
Reply to: