[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#1101838: ITP: hexagon-dsp-binaries - Qualcomm Hexagon DSP binaries



# Re-review 26/9/25 (from commit f1df66f)

It looks like my previous review expired on mentors.debian.net, so I'll
incorporate the previous text here for context, and copy it to the ITP
bug so it stays around.

On re-review I think some points still need addressing, but many are
also resolved, either by your changes or as a result of our discussions
- thank you!

On the points on which you pushed back (OOB), you were right to ask on
debian-devel@ but unfortunately we have not received any feedback there.
I have consulted directly with one DD friend (as well as Loic who is
also a DD) to gauge how well my opinion matches others' thoughts. I've
incorporated them in my answers here.

> [Needs Fixing; subject to discussion] It seems odd to put "-binaries-'
> in the package names. And all binary Debian packages are binary :)  I
> don't think this will give a typical user any further information on
> what the binary package does, so I'd like to avoid using this word
> there. I think "hexagon-dsp-" is fine as a prefix, but it'd be nice to
> use something more specific than "binaries". Maybe "apps"? "tools"?

[No further action needed] Consultation answer: this seems OK, provided
that there's an explanation in the description, which there is.

> [Subject to discussion] Given developer reference 6.2.1, 2 and 3, is
> "Qualcomm" a necessary part of the package description? Can we make it
> more understandable by users? How are they going to determine which
> package to install?

You didn't address this, but I think it's OK to leave for now unless we
get review push back from others.

> [Subject to discussion] Is package splitting really necessary? We
> don't split linux-firmware or alsa-ucm-conf, and splitting here has
> resulted in a bunch of extra generated files related pain. How will
> depending packages depend on the correct binary package here? What's
> the user story on how they're going to get the correct packages
> installed?

Loic made the case that a user should be able to roll their own image
specifically for one board, even if our universal image for general
consumption won't, so I accept the splitting. We will still need to
resolve how to depend on these binaries from packages that need them,
but I can think of a way using something like "Depends:
hexagon-dsp-binaries | hexagon-dsp-binaries-specific" on the depending
side and a "Provides: hexagon-dsp-binaries-specific" on each split
package so the problem is tractable. I don't think we need to implement
that or discuss the details now.

> [Needs Fixing] debian/copyright: Makefile, scripts/install.sh and
> dist.sh are clearly marked MIT and need to be marked accordingly in
> debian/copyright. config.txt, scripts/check.py and
> scripts/filter_whence.py need their licences clarified upstream.
> Ideally there would be clarity from upstream in the source tree itself
> on what the licence is for source files not specifically marked.

I see this is mostly resolved - thanks.

[Needs Information] debian/copyright indicates that WHENCE is under
binary-redist-Qualcomm-media. Is that your intention?

[Needs Fixing] (for when we pull in a newer upstream)
https://github.com/linux-msm/hexagon-dsp-binaries/commit/dfcdfcf0de7f9846cd8274ddba332ce52f840168
introduces LICENSE.MIT, but now it isn't clear what the "default" is
because both proprietary and MIT licence files are present in the source
tree. So which licence applies to
`apq8096/Qualcomm/db820c/ADSP.8996.2.7.1-00138/AlacDecoderModule.so.1`,
for example, and how would I know that? It's now ambiguous. This relates
to the previous point: what licence applies to files that aren't
directly marked themselves?

> [Needs discussion] debian/copyright debian/*: there's a convention to
> use the same licence for debian/* as the upstream, to avoid conflicts
> when upstreaming patches or other packaging also useful to upstream,
> especially if somebody else contributes them. I'm not sure how this
> works with non-free. I'm surprised at your choice of GPL-2+ here
> though.

I see this is resolved - thanks.

> [Needs Information] debian/copyright debian/*: are you doing this in
> your own time, or as a Qualcomm employee? Who owns the copyright on
> your packaging work? If Qualcomm, then you likely cannot claim the
> copyright for yourself personally (or are you saying that,
> exceptionally, you can?), and they need to sign off on your choice of
> GPL-2+ for your packaging.

I see this is resolved - thanks.

> [Needs Fixing] lintian tag new-package-uses-date-based-version-number
> seems valid; please drop it and use the 0~ prefix as advised by
> https://lintian.debian.org/tags/new-package-uses-date-based-version-number.html 

I see this is resolved - thanks.

> [Needs Fixing] debian/upstream/metadata: why the commented out fields?

I see this is resolved - thanks.

> [Needs Information for the eventual uploader] Vcs-Browser: Where's our
> goal for where this will be maintained eventually? Are you planning to
> move this to a team on Salsa?

You didn't address this, but I think it's OK to leave for now.

> [Fix if you like, or not] Commit 84fd741: if the packaging git
> repository is only ever going to contain a debian directory, are you
> planning on prefixing all commits with 'debian/'? What does that
> achieve?

You didn't address this which is fine, but note that commit f1df6 that
does this has a typo that you may want to fix.

> [Fix if you like, or not] Convention is to use .in for the source of
> generated files, so maybe you could use lintian-overrides.in instead
> of lintian-template?

I see this is resolved - thanks.

> [Needs Fixing] README.source: Debian Policy 12.5 says the explanation
> of where the upstream sources were obtained must be specified in the
> copyright file, so this seems like the right place to put this
> information instead. Please move this information from
> debian/README.source to debian/copyright under a Header stanza Comment
> field.

[Needs Fixing] Looks like this still needs doing.

> [Needs Fixing] Debian Policy 12.5 says: "Packages in the contrib or
> non-free archive areas should state in the copyright file that the
> package is not part of the Debian distribution and briefly explain
> why.". I think this is intended to include non-free-firmware too,
> which is newer. Please add this statement and explanation.

[Needs Fixing] Looks like this still needs doing.

> [Needs Fixing; subject to discussion] The final Debian source package
> upload is required to have debian/control even though it is generated.
> The conventional hack is to actually check it in to git. That way the
> git tree will match the source package exactly. Otherwise a future
> sponsor will have difficulty in getting from the git repository to an
> uploaded package. This is subjective and not a Debian requirement, but
> I think it makes sense to follow existing convention here. See for
> example src:golang-1.24 and src:llvm-toolchain-19 as examples of where
> debian/control is generated. Therefore, please check in the generated
> files. In this case, since you're generating files that change name,
> it might be an idea to also add some tooling to clean up all generated
> files as a separate built target. Sorry this is ugly. I suspect this
> might mean that you will want to refactor the generation mechanism, so
> I haven't reviewed it in detail and will do that on the next review
> iteration.

You pointed out that you followed the pattern of firmware-nonfree and I
accepted that, so there's no further action needed here.

> [Needs Fixing] Noise from packaging templates should be removed, as
> explicitly mentioned in https://ftp-master.debian.org/REJECT-FAQ.html.
> At least the template comments and unused examples in debian/rules.

I see this is resolved - thanks.

> [Needs Fixing] lintian-template typos: "out expectations", "data"

I see this is resolved - thanks.

> [Needs Information]

> > # Upstream check.py can't work with Debian's packaging

> What do we need to do to get it to work?

[Needs Information] Please address this.

> [Fix if you like, or not] I suggest that this would be cleaner:

> -# Debian doesn't want extra license files
> -override_dh_auto_install:
> -       dh_auto_install
> -       find debian/tmp/usr/share/qcom -name "LICENSE.*" -delete
> +# Licences are already being provided via debian/copyright
> +override_dh_install:
> +       dh_install -XLICENSE.\*

I see this is resolved - thanks.

> [Needs Fixing] README.source typo: "tin the"

I see this is resolved - thanks.

> [Needs Fixing] /usr/share/qcom seems like an odd choice of namespace
> for the installed firmware files themselves. Can we use a namespace
> that is based on the package name instead? That will avoid collisions
> against other unrelated packages.

[Needs Fixing] In my consultation with another DD, this was the most
significant concern. Debian does not use /usr/share/<vendor>; that is
contrary to the intent of the FHS. We use /usr/share/<purpose>, often
aligning the purpose with the package name. The filesystem namespace is
ultimately owned by Debian for Debian packaging, not upstream. Even if
it's nice to align, our conventions for the filesystem namespace,
especially at package level where the namespace is shared, trump that.
Please use /usr/share/hexagon-dsp/ instead.

> [Needs Information] What purposes do the map*.txt files serve? Are
> they necessary to be present in the binary packages?

[Needs Information] Please address this. If we're going to the trouble
of a lintian override, I'd like an explanation of why it is needed to be
shipped in the binary package in the first place, why it cannot be in
/usr/share/doc/ if it's just users who might want to consult it, or for
it to be moved to debian/not-installed if it is not required.

> [Needs Information] The ITP mentions: "With this in place packagers
> can require that the version of the DSP binary package must match the
> linux-firmware-qcom-soc package version."; does this need
> implementing? I don't see the mentioned linux-firmware-qcom-soc
> package.

[Needs Fixing] FTR, this is discussed at
https://lists.debian.org/debian-devel/2025/08/msg00508.html,
https://lists.debian.org/debian-devel/2025/09/msg00215.html and
https://salsa.debian.org/kernel-team/firmware-nonfree/-/merge_requests/128.
OOB, we have agreed to defer this for now while we want for the
firmware-nonfree maintainers to respond. So for the time being, please
prepare this for upload to experimental instead (suffix the version
~exp1 and change the target suite to "experimental"). That way we can
get the NEW review going without impacting firmware-nonfree until they
have had an opportunity to respond.

## New review points

[Fix if you like, or not] autogen.sh isn't a great choice of name - such
a script is conventionally expected to generate a configure script (eg.
by running automake/autoconf) so is confusing.

[No action needed] We discussed out of band about Architecture: all vs.
Architecture: arm64 and Multi-Arch: foreign. Consultation answer: these
are equivalent for all relevant purposes, so though I prefer the latter,
I'm willing sponsor as it is on this point.

[Needs Fixing]

Following your instructions in debian/README.source, from a completely
clean tree (git clean -ffxd) from your commit f1df66f, I get:

```
$ debian/rules debian/control
dh config.txt
dh: error: "debian/control" not found. Are you sure you are in the correct directory?
make: *** [debian/rules:4: config.txt] Error 255
```

I have `../hexagon-dsp-binaries_20250808.orig.tar.gz` available.

Is this a bug in your script, is `debian/README.source` out of date, or
am I missing something?

I have yet to review the generation code since I'd like to see it
working first.


Reply to: