Code review comment for ~paelzer/ubuntu/+source/spice:merge-14.2-4-focal

Revision history for this message
Bryce Harrington (bryce) wrote :

I've run the autopkgtests on the ppa package in lxc, and looked at the failing test-display-streaming test - for me it doesn't even produce output, I guess because there's no $DISPLAY available? The bileto test results log also shows the test failed, although it shows up as a PASS in the excuses results(?!?) Guessing there's some setup work needed to get the remote display registered properly in lxc? (Might be useful for future reviewing to have a paint-by-numbers set of commands to run to get remote display enabled inside an lxc container.)

However, given that test-display-streaming appears to not have been hooked up before, and may not be getting necessary attention upstream, it may make sense to disable it. It looks like this would avoid needing to build some C code, which would also make test run time faster. Given that upstream appears to have an active and well-maintained CI system, the risk of something being missed by excluding this test seems minimal.

That said, given that the purpose of this tool is for accessing a remote desktop, some sort of smoke test to verify graphics are getting generated and transmitted from server to client would be appropriate for acceptance testing. But that seems a different scope than this merge from debian.

Anyway, everything else in the merge LGTM; things I've checked listed below. As to the failed test, while I lean towards disabling it, I don't feel strongly enough on that point to block landing the merge, esp. since it isn't showing up as a fail on the excuses page.

- [√] changelog entry correct, targeted to correct codename
- [√] no major upstream changes to consider
- [√] debian changes look safe
- [√] update-maintainer has been run
- [√] changes forwarded upstream/debian (if appropriate)
- [√] nothing else to drop
- [-] patches match what was proposed upstream
- [-] patches correctly included in debian/patches/series?
- [-] patches have correct DEP3 metadata

review: Approve

« Back to merge proposal