Bug #1973784 “[MIR] libldac” : Bugs : libldac package : Ubuntu

Sebastien Bacher (seb128) on 2022-05-19

description:	updated
Changed in libldac (Ubuntu):
status:	Incomplete → New
description:	updated

Revision history for this message

Sebastien Bacher (seb128) wrote on 2022-05-20:

#1

Updated the description, we have added autopkgtests now

description:

updated

Christian Ehrhardt  (paelzer) on 2022-05-24

Changed in libldac (Ubuntu):
assignee:	nobody → Lukas Märdian (slyon)

Revision history for this message

Lukas Märdian (slyon) wrote on 2022-05-25:

#2

Download full text (5.5 KiB)

Review for Package: src:libldac

[Summary]
LDAC, LDAC/ABR is a codec by Sony, used for bluetooth headsets. The encoder is
Apache-2.0 licensed and can be used by pulseaudio or pipewire to transmit audio.
From a MIR POV the upstream package doesn't seem to be super well maintained
(slow/sporadic updates), but it's a mature library, with a small, isolated
usecase, which does not need lots of updates. So seems to be OK.

MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does not need a security review.

List of specific binary packages to be promoted to main:
- libldacbt-enc2, libldacbt-abr2
Specific binary packages built, but NOT to be promoted to main: <None>

Notes:
#0 "seeded-in-ubuntu libldac" shows that the Kubuntu/Budgie/Mate/UbuntuStudio
seeds depend on this package already, IMO this is not a problem. I will
confirm with the rest of the MIR team.

Required TODOs:
#1 please run `update-maintainer` on the package, to update debian/control
#2 It is on the lto-disabled list (for s390x only). s390x is not supported by
   upstream and FTBFS, so this entry should just be removed from the list.
#3 does NOT have a test suite that runs at build time, and it only has one
   simple autopkgtest (marked "superficial"). This does not fully check the
   Q/A-testing requirements, IMO. Usually a proper autopkgtest would do, but
   here the autopkgtest is minimal and we don't have build-time tests...
   Are there any plans for end-to-end testing of the bluetooth codec/hardware?
   Could a end2end test plan/code/log be provided or stated in the comments?

Recommended TODOs:
#4 The package should get a team bug subscriber before being promoted
#5 build-time warnings should be resolved, working with upstream (see below)

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - checked with check-mir ("mini-soong" is a build-dependency and can stay in universe)
  - none of the (potentially auto-generated) dependencies (Depends
    and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring more tests now.

Problems:
- "seeded-in-ubuntu libldac" shows that it is being used in
Kubuntu/Budgie/Mate/UbuntuStudio already, IMO this is not a problem.

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard

Problems: None

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)

Problems: None

[Co...

Review for Package: src:libldac

[Summary]
LDAC, LDAC/ABR is a codec by Sony, used for bluetooth headsets. The encoder is
Apache-2.0 licensed and can be used by pulseaudio or pipewire to transmit audio.
From a MIR POV the upstream package doesn't seem to be super well maintained
(slow/sporadic updates), but it's a mature library, with a small, isolated
usecase, which does not need lots of updates. So seems to be OK.

MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does not need a security review.

List of specific binary packages to be promoted to main:
- libldacbt-enc2, libldacbt-abr2
Specific binary packages built, but NOT to be promoted to main: <None>

Notes:
#0 "seeded-in-ubuntu libldac" shows that the Kubuntu/Budgie/Mate/UbuntuStudio
   seeds depend on this package already, IMO this is not a problem. I will
   confirm with the rest of the MIR team.

Required TODOs:
#1 please run `update-maintainer` on the package, to update debian/control
#2 It is on the lto-disabled list (for s390x only). s390x is not supported by
   upstream and FTBFS, so this entry should just be removed from the list.
#3 does NOT have a test suite that runs at build time, and it only has one
   simple autopkgtest (marked "superficial"). This does not fully check the
   Q/A-testing requirements, IMO. Usually a proper autopkgtest would do, but
   here the autopkgtest is minimal and we don't have build-time tests...
   Are there any plans for end-to-end testing of the bluetooth codec/hardware?
   Could a end2end test plan/code/log be provided or stated in the comments?

Recommended TODOs:
#4 The package should get a team bug subscriber before being promoted
#5 build-time warnings should be resolved, working with upstream (see below)

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - checked with check-mir ("mini-soong" is a build-dependency and can stay in universe)
  - none of the (potentially auto-generated) dependencies (Depends
    and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring more tests now.

Problems:
- "seeded-in-ubuntu libldac" shows that it is being used in
  Kubuntu/Budgie/Mate/UbuntuStudio already, IMO this is not a problem.

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard

Problems: None

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)

Problems: None

[Common blockers]
OK:
- does not FTBFS currently
- does have a (non-trivial?!) test suite that runs as autopkgtest
- no new python2 dependency
- not a Python package
- not a Go package

Problems:
- does NOT have a test suite that runs at build time
  - test suite failure MUST fail the build upon error.
- bluetooth HW prevents end2end tests. Is there a test plan, code, log provided?

[Packaging red flags]
OK:
- Ubuntu does carry a delta, but it is reasonable and maintenance under control,
  submitted to Debian in https://bugs.debian.org/1011314
- symbols tracking is in place
- d/watch is present and looks ok (if needed, e.g. non-native)
- Upstream update history is slow, but OK for a small library like this
- Debian/Ubuntu update history is sporadic, but OK for a small library like this
- the current release is packaged (+ git snapshot on top of it)
- promoting this does not seem to cause issues for MOTUs that so far maintained
  the package
- no massive Lintian warnings
- d/rules is rather clean

Problems:
- please run `update-maintainer` on the package, to update debian/control
- It is on the lto-disabled list (for s390x only). s390x is not supported by
  upstream and FTBFS, so this entry should just be removed from the list

[Upstream red flags]
OK:
- no incautious use of malloc/sprintf (as far as we can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests)
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- not part of the UI for extra checks
- no translation present, but none needed for this case (user visible)?

Problems:
- warnings during the build:
In file included from src/ldaclib.c:37:
src/sigana_ldac.c: In function ‘ana_frame_status_ldac’:
src/sigana_ldac.c:178:24: warning: ‘a_status’ may be used uninitialized [-Wmaybe-uninitialized]
  178 |         return a_status[0];
      |                ~~~~~~~~^~~
src/sigana_ldac.c:133:9: note: ‘a_status’ declared here
  133 |     int a_status[LDAC_PRCNCH];
      |         ^~~~~~~~
src/sigana_ldac.c: In function ‘ana_frame_status_ldac’:
src/sigana_ldac.c:178:24: warning: ‘a_status’ may be used uninitialized [-Wmaybe-uninitialized]
  178 |         return a_status[0];
      |                        ^
src/sigana_ldac.c:133:9: note: ‘a_status’ declared here
  133 |     int a_status[LDAC_PRCNCH];
      |         ^

Changed in libldac (Ubuntu):
assignee:	Lukas Märdian (slyon) → nobody
status:	New → Incomplete

Revision history for this message

Lukas Märdian (slyon) wrote on 2022-05-25:

#3

I'd like to ask other fellow MIR team members some policy questions about the above:

#0 "seeded-in-ubuntu" policy: Why could this be a problem (as listed in https://wiki.ubuntu.com/MainInclusionProcess), do we need to update the wiki?

#2 testing/qa requirements: Should we accept a single "superficial" autopkgtest as sufficient testing or require a test-plan for additional end-to-end testing? It compiles the library (using pkgconfig) and runs a single function, so confirms that the library builds/loads/executes. There isn't much more that we can do to tests such a library at runtime. (cf: https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=1011314;filename=libldac.patch;msg=5)

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2022-05-25:

#4

Download full text (4.3 KiB)

Hi Lukas

TL;DR for your questions:
- yes we need the seeded-in-ubuntu statement, but wording might be improved
- It is ok to be superficial (or even not) tested if shown to be reasonably covered elsewhere

--- Details ---

> #0 "seeded-in-ubuntu" policy: Why could this be a problem

It is the inverse, the only place it is listed is when checking further dependencies.
```
TODO: - no other Dependencies to MIR due to this
TODO: - checked with check-mir
TODO: - not listed in seeded-in-ubuntu
TODO: - none of the (potentially auto-generated) dependencies (Depends
TODO: and Recommends) that are present after build are not in main
```

That check is meant to detect the following situation early on.
Package `foo` shows as component mismatch and is processed.
Then much later once all completed `foo` is promoted and then it shows up as additional component mismatch because `foo` depends on `bar` which also needs to be promoted.

The check here is meant to spot "hey you need to file and process `bar` as well" early on to avoid late surprises.

> #2 testing/qa requirements: Should we accept a single "superficial" autopkgtest as sufficient [...] ?

We tried to prepare for this with recent rule changes.
As you say sometimes there can only be done "so much" at the lib level (more a unittest than an actual workload).

The following sections are related:

```
RULE: - If no build tests nor autopkgtests are included, and/or if the package
RULE: requires specific hardware to perform testing, the subscribed team
RULE: must provide a written test plan in a comment to the MIR bug, and
RULE: commit to running that test either at each upload of the package or
RULE: at least once each release cycle. In the comment to the MIR bug,
RULE: please link to the codebase of these tests (scripts or doc of manual
RULE: steps) and attach a full log of these test runs. This is meant to
RULE: assess their validity (e.g. not just superficial)
TODO: - The package can not be tested at build or autopktest time because TBD
TODO: to make up for that here TBD is a test plan/automation and example
TODO: test TBD (logs/scripts)

RULE: - In some cases a solution that is about to be promoted consists of
RULE: several very small libraries and one actual application uniting them
RULE: to achieve something useful. This is rather common in the go/rust space.
RULE: In that case often these micro-libs on their own can and should only
RULE: provide low level unit-tests. But more complex autopkgtests make no
RULE: sense on that level. Therefore in those cases one might want to test on
RULE: the solution level.
RULE: - Process wise MIR-requesting teams can ask (on the bug) for this
RULE: special case to apply for a given case, which reduces the test
RULE: constraints on the micro libraries but in return increases the
RULE: requirements for the test of the actual app/solution.
RULE: - Since this might promote micro-lib packages to main with less than
RULE: the common level of QA any further MIRed program using them will have
RULE: to provide the same amount of increased testing.
TODO: - This package is minimal and will be tested in a ...

Hi Lukas

TL;DR for your questions:
- yes we need the seeded-in-ubuntu statement, but wording might be improved
- It is ok to be superficial (or even not) tested if shown to be reasonably covered elsewhere

--- Details ---

> #0 "seeded-in-ubuntu" policy: Why could this be a problem

It is the inverse, the only place it is listed is when checking further dependencies.
```
TODO: - no other Dependencies to MIR due to this
TODO:   - checked with check-mir
TODO:   - not listed in seeded-in-ubuntu
TODO:   - none of the (potentially auto-generated) dependencies (Depends
TODO:     and Recommends) that are present after build are not in main
```

That check is meant to detect the following situation early on.
Package `foo` shows as component mismatch and is processed.
Then much later once all completed `foo` is promoted and then it shows up as additional component mismatch because `foo` depends on `bar` which also needs to be promoted.

The check here is meant to spot "hey you need to file and process `bar` as well" early on to avoid late surprises.

> #2 testing/qa requirements: Should we accept a single "superficial" autopkgtest as sufficient [...] ?

We tried to prepare for this with recent rule changes.
As you say sometimes there can only be done "so much" at the lib level (more a unittest than an actual workload).

The following sections are related:

```
RULE: - If no build tests nor autopkgtests are included, and/or if the package
RULE:   requires specific hardware to perform testing, the subscribed team
RULE:   must provide a written test plan in a comment to the MIR bug, and
RULE:   commit to running that test either at each upload of the package or
RULE:   at least once each release cycle. In the comment to the MIR bug,
RULE:   please link to the codebase of these tests (scripts or doc of manual
RULE:   steps) and attach a full log of these test runs. This is meant to
RULE:   assess their validity (e.g. not just superficial)
TODO: - The package can not be tested at build or autopktest time because TBD
TODO:   to make up for that here TBD is a test plan/automation and example
TODO:   test TBD (logs/scripts)

RULE: - In some cases a solution that is about to be promoted consists of
RULE:   several very small libraries and one actual application uniting them
RULE:   to achieve something useful. This is rather common in the go/rust space.
RULE:   In that case often these micro-libs on their own can and should only
RULE:   provide low level unit-tests. But more complex autopkgtests make no
RULE:   sense on that level. Therefore in those cases one might want to test on
RULE:   the solution level.
RULE:   - Process wise MIR-requesting teams can ask (on the bug) for this
RULE:     special case to apply for a given case, which reduces the test
RULE:     constraints on the micro libraries but in return increases the
RULE:     requirements for the test of the actual app/solution.
RULE:   - Since this might promote micro-lib packages to main with less than
RULE:     the common level of QA any further MIRed program using them will have
RULE:     to provide the same amount of increased testing.
TODO: - This package is minimal and will be tested in a more wide reaching
TODO:   solution context TBD, details about this testing are here TBD
...
TODO: - does have a test suite that runs at build time
TODO:   - test suite fails will fail the build upon error.
TODO: - does have a non-trivial test suite that runs as autopkgtest
TODO: - if special HW does prevent build/autopkgtest is there a test plan, code,
TODO:   log provided?
TODO: - if a non-trivial test on this level does not make sense (the lib alone
TODO:   is only doing rather simple things), is the overall solution (app+libs)
TODO:   extensively covered i.e. via end to end autopkgtest ?
```

It has to be a case by case decision.
For example strictly speaking DPDK is a library, but extended tests make sense.
Another lib does nothing but escaping URLs, that might be tested at a higher level.
The sections above were added a while ago to make this clear.

Combined they allow:
- If testing at the lib makes no sense to state so and kind of "be allowed to be promoted without adding an odd test"
- but at the same time by being forced to state by what it is covered instead, we ensure that it  covered somewhere (otherwise lib would say need to be tested above; and above doesn't care => no test)

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2022-05-25:

#5

After a reread, I have nothing to add compared to what Christian mentioned :)

Revision history for this message

Sebastien Bacher (seb128) wrote on 2022-06-01:

#6

https://launchpad.net/ubuntu/+source/lto-disabled-list/26 fixes one of the todos

Sebastien Bacher (seb128) on 2022-06-02

description:

updated

Revision history for this message

Sebastien Bacher (seb128) wrote on 2022-06-02:

#7

Thanks Lukas for the review, I think it should be ready to be considered again now

> Required TODOs:
> #1 please run `update-maintainer` on the package, to update debian/control

We will do in the next upload (if that's not a sync since we forwarded the autopkgtest to Debian), I don't think that's worth wasting buildds time by itself now. Ok with you?

> #2 It is on the lto-disabled list (for s390x only). s390x is not supported by
> upstream and FTBFS, so this entry should just be removed from the list.

https://launchpad.net/ubuntu/+source/lto-disabled-list/26

> #3 does NOT have a test suite that runs at build time, and it only has one
> simple autopkgtest (marked "superficial"). This does not fully check the
> Q/A-testing requirements, IMO. Usually a proper autopkgtest would do, but
> here the autopkgtest is minimal and we don't have build-time tests...
> Are there any plans for end-to-end testing of the bluetooth codec/hardware?
> Could a end2end test plan/code/log be provided or stated in the comments?

We created a testplan for pipewire which includes a section specific to libldac/libfreeaptx, https://wiki.ubuntu.com/DesktopTeam/TestPlans/Pipewire

> Recommended TODOs:
> #4 The package should get a team bug subscriber before being promoted

desktop-packages has been subscribed now

> #5 build-time warnings should be resolved, working with upstream (see below)

I didn't find a bugtracker for upstream (it's part of android) but for now I've reported it to Debian and we will see if we can provide a patch

Changed in libldac (Ubuntu):
assignee:	nobody → Lukas Märdian (slyon)
status:	Incomplete → New

Revision history for this message

Lukas Märdian (slyon) wrote on 2022-06-02 (last edit on 2022-06-02):

#8

Yes, I agree. Let's downgrade #1 to a recommended TODO, as there is no upload needed otherwise.
LGTM. MIR team ACK.

No security review is needed, so feel free to pull it in as a dependency or seed the package.

Changed in libldac (Ubuntu):
status:	New → In Progress
assignee:	Lukas Märdian (slyon) → nobody

Revision history for this message

Sebastien Bacher (seb128) wrote on 2022-06-03:

#9

libldac 2.0.2.3+git20200429+ed310a0-4ubuntu1 in kinetic: universe/misc -> main
Override [y|N]? y
1 publication overridden.

Changed in libldac (Ubuntu):
status:	In Progress → Fix Released

Ubuntu
libldac package

[MIR] libldac

Bug Description

Other bug subscribers

Remote bug watches

Ubuntulibldac package

[MIR] libldac

Bug Description

Other bug subscribers

Remote bug watches

Ubuntu
libldac package