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

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



On Fri, Sep 26, 2025 at 03:21:23PM +0100, Robie Basak wrote:
> # 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.

Thanks!

> 
> 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!

Thanks. I'm going to skip the no-further-action comments, unless there
is something to say in response.

> 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.
> 
> > [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.

In the package descriptions there are two mentions of Qualcomm:
- Qualcomm SoCs, which should be fine as it describes the owner of the
  CPUs present in those devices
- Qualcomm foo bar, which is the commercial name of the device. Devices
  marketed by Thundercomm have that vendor name instead.

Please correct me if I missed something or if I should change anything
here.

> 
> > [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.

That's a good point. If fastrpc is split into three daemon packages,
then each daemon package should 'Suggests: hexagon-dsp-binaries-Ndsp',
where 'N' is one of 'a', 'c', 'g' or 's', for each kind of DSPs.

WDYT? I can implement that straight away.

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

>From my POV it doesn't contain code, so it's not copyrightable. But
let's put it under the MIT exclusion together with config.txt

> [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?

It is described in README.source and (upstream) in the WHENCE file.
Please provide recommendations on how to express it in a better way.

> > [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.

My plan is to ask DebianOnMobile whether they are interested in adopting
this package. If not, I can continue handling it on my own.

> > [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.

Fixed, thanks. Yes, I think it's a good idea to prefix all commit
messages with debian/ or d/

> 
> > [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.

Here I followed the firmware-nonfree. I have provided the Source: field,
which points to the upstream repo with the tarballs in the releases
field.

> 
> > [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.

Adding Disclaimer: to debian/copyright, thank you.

> > [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.

I think I provided an explanation inside debian/rules. It requires
upstream Git repo as it performs checks of WHENCE vs config.txt vs Git.
These checks are performed when new firmware is being added to the
archive and they target making sure that the commit is self-consistent.
There is no need to run them at the packaging time.

> > [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.

Ack.

> 
> > [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.

For this question I have no idea, unfortunately. They might be used by
the SDK or by something else. I have opened an issue at the FastRPC
project, asking for clarifications. For now I'd prefer to keep them. We
can drop them later, if FastRPC developers respond that they are
unnecessary.

For the reference: https://github.com/qualcomm/fastrpc/issues/248

> 
> > [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

This is something new. I've changed the suite to experimental, But I
don't think it's requried or recommended to use ~exp for experimental.

> get the NEW review going without impacting firmware-nonfree until they
> have had an opportunity to respond.

Ack

> 
> ## 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.

Renaming it to regen.sh, hopefully it will be the last rename.

> [Needs Fixing]
> 
> Following your instructions in debian/README.source, from a completely
> clean tree (git clean -ffxd) from your commit f1df66f, I get:
> 
[build log, skipped]
> 
> Is this a bug in your script, is `debian/README.source` out of date, or
> am I missing something?

Yes, it was a bug, thanks. I've added an extra build step to unpack the
archive.

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

I've pushed the updated packaging source to salsa, packages are uploaded
to mentors and penging their script processing.

-- 
With best wishes
Dmitry


Reply to: