Code review comment for ~paelzer/ubuntu/+source/qemu:lp-1921468-1887535-1921665-GROOVY

Revision history for this message
Utkarsh Gupta (utkarsh) wrote :

* Changelog:
  - [✔] changelog entry correct version and targeted codename
  - [✔] changelog entries correct
  - [✔] update-maintainer has been run

* New Delta:
  - [✔] patches match what was proposed upstream
  - [✔] patches correctly included in debian/patches/series
  - [✔] patches have correct DEP3 metadata
  - [?] patches are all correct

A quick note here: I prefer to run `quilt refresh` after a patch is applied to refresh a patch, this removes the unnecessary noise from all the patches.

The first 3 patches seems to be partially refreshed (which is good!), but the last two (namely, lp-1921665-1-block-Require-aligned-image-size-to-avoid-assert.patch and lp-1921665-2-file-posix-Allow-byte-aligned-O_DIRECT-with-NFS.patch) haven't been refreshed. I know this is based on preferences, et al, but this is one thing that I always try to adapt to. Trivial (but of significance for me), so you could ignore as well, but I'd love it if you could do a complete refresh of all the 5 patches! :)

* Build/Test:
  - [✔] build is ok
  - [✔] verified PPA package installs/uninstalls
  - [✔] autopkgtest against the PPA package passes

* Extra:
  - [?] lintian warnings

I get a bunch of lintian warning for spelling errors; cf: Maybe worth fixing it now or later? Or in fact, just forward the Debian specific ones to mjt and he can fix them. And maybe, we can fix the Ubuntu specific ones?

Another very, very trivial lintian comment is about trailing whitespaces in debian/changelog. You could run:
sed -i -e 's@[[:space:]]*$@@g' debian/changelog
to get rid of them but I guess worth sending this pointer to mjt maybe?

That said, this MP looks good and you can upload as-is but maybe worth considering my points above?

review: Approve

« Back to merge proposal