Code review comment for ~paelzer/ubuntu/+source/qemu:merge-6.0-impish

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI - I have resolved the problem with sdl-audio - it now is (as planned) in a loadable module and part of qemu-system-gui. That is also what I have proposed to Debian.

> - Commit 7a528a4750097 (qemu-kvm to systemd unit) mentions some changes in d/qemu-kvm.service
> but I see no changes in this file. Should we remove this from the commit message? I see this is
> not present in the changelog.

This is now in debian/qemu-system-common.qemu-kvm.service but otherwise still correct (and part of this commit already).
I've updated the commit - it was already correct in the changelog.

> - Commit 030a11ff79fd6 (Distribution specific machine type) mentions d/p/ubuntu/lp-1761372-*
> but those files do not exist anymore. Are you keeping this just to reference LP: 1761372?

Indeed this was folded into the base patch a long time ago.
Adapted commit message and changelog.

> - It was not clear to me why the fuse support was disabled. Could you
> add some reasoning about that?

It is the usual problem that the lib isn't in main.
I hope paride can file a MIR for it to train on that for his core-dev application.
But as we know then it will still take a while.
I've updated the changelog and commit to reflect that.
Thanks for the hint!

> - Commit ad80fc7f0c272 (d/rules: clear all (current and former) modules on purge)
> has a typo in the description: s/there might me/there might be/

Fixed, thanks.
Also fixed in the related Debian MR

> - I'd also use capitalized letters at the beginning of every entry in the
> changelog, some of them are using lowercase and others uppercase.

Done, all uppercase now except those that begin with filenames.

> Once you have addressed the comments above feel free to upload it, I am
> already marking it as approved since nothing major needs a fix AFAICT.

Thanks!

The last bit left from my testing is the auto-start of the run-qemu.mount unit.

« Back to merge proposal