Comment 3 for bug 1946359

Revision history for this message
Lukas Märdian (slyon) wrote :

Hi Miriam!
I checked the vulkan-tools MIR for Jammy/Impish/Hirsute/Focal, which looks rather straight forwards, but requires a few additional changes before the MIR team can give the final ACK.

Also, I checked the vulkan MIR for Bionic, that seems to be much more complicated. Please see my remarks in the next comment. I will be requesting security review for this one, too.

------ vulkan-tools ------
[Summary]
TODO: MIR team ACK, pending some requested changes (see below).

This MIR is looking pretty good overall, I do not see any big security constraints and I think we do not need a security review. But the proposed autopkgtests should be integrated and a build-time test suite needs to be added in addition to integrating the LTO workaround (or fix) into the package itself. All of those changes need to be SRUed into the stable series down to Focal/Bionic.
This does NOT need a security review
List of specific binary packages to be promoted to main: vulkan-tools (we can probably skip the transitional vulkan-utils package).

Notes:
As this MIR is to be backdated to Hirsute/Focal (and Bionic for src:vulkan) we need the proposed changes to be SRUed to those series first. (see https://wiki.ubuntu.com/StableReleaseUpdates)

Required TODOs:
* Add build-time tests that fail the build if they don't pass
* Integrate the proposed autopkgtests
* avoid the lto-disabled list (for arm64), should be fixed (or worked around) inside the package itself
* SRU those changes down to the stable series (Impish/Hirsute/Focal/Bionic)

Recommended TODOs:
* Add a team bug subscriber
* Check build warning (in Focal version): "cube.cpp:2067:38: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type"
* Try to keep vulkan-tools more up-to-date in the future

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

[Dependencies]
OK:
* no other Dependencies to MIR due to this (glslang-tools is only a build-dep, all others are in main already)
* No dependencies in main that are only superficially tested requiring more tests now

[Embedded sources and static linking]
OK:
* no embedded source present
* no static linking

[Security]
OK:
* history of CVEs does not look concerning (no CVEs found)
* 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
* 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)

[Common blockers]
OK:
* does not FTBFS currently
* no translation present, but none needed for this case (user visible)?
* not a python/go package, no extra constraints to consider in that regard

Recommended:
* The package does not contain any autopkgtests. Those are being prepared but we want them to be inside the archive and SRUed to all relevant series (i.e. down to Focal/Bionic):
https://git.launchpad.net/~mirespace/+git/vulkan-tools/log/?h=impish-vulkan-tools-adding-dep8-tests

Problems:
* does NOT have a test suite that runs at build time and fails the build upon error. (Maybe use something similar to what is described in BUILD.md -> ### Linux Tests or run the proposed autopkgtest during build, too?)

[Packaging red flags]
OK:
* Ubuntu does carry a delta, but it is reasonable and maintenance under control (adding tests, will be submitted to Debian, too)
* symbols tracking not applicable for this kind of code.
* d/watch is present and looks ok (if needed, e.g. non-native)
* Upstream update history is good
* Debian/Ubuntu update history is slow, but reasonable
* 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
* Does not have Built-Using

Recommended:
* the current release is NOT packaged (upstream at v1.2.196, Debian at v1.2.189, Ubuntu at v1.2.162)

Problems:
* is on the lto-disabled list (for arm64), this should be fixed or the workaround should be included directly in the package

[Upstream red flags]
OK:
* no incautious use of malloc/sprintf (as far as I 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

Recommended:
* Focal version shows errors during build (that seems to be fixed in later versions): "cube.cpp:2067:38: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct vk::SubresourceLayout’; use assignment or value-initialization instead [-Wclass-memaccess]"
* There is some fishy usage of malloc (passing the argument count into malloc) in cube.c and cube.cpp inside the "WinMain" function, but this seems to be unused inside the linux build.