Code review comment for ~mkukri/ubuntu/+source/presage:merge

Revision history for this message
Simon Quigley (tsimonq2) wrote :

A few items here that need fixing:
 - The changelog should have another once-over on it. Don't modify *past* changelog entries, but just double check it's complete. I know we discussed some of these elements over IRC, so maybe you're able to just check this one off.
 - We should really think about whether d/p/python.diff should still be in the delta. Is there now support for Python 3 in the Debian package, and we just need to pull that in? Please consider this, and share your thoughts.
 - Generally speaking, I really do like to see proper DEP-3 headers[1]. Lines like these:

+From: Ubuntu Developers <email address hidden>
+Date: Tue, 28 Nov 2023 11:12:03 +0000
+Subject: python

...really aren't verbose enough, and do not contain the proper formatting. An easy way to add a DEP-3 header to the existing patch on the stack is to remove the existing header, then run `quilt header --dep3 -e` which will give you a template to work with.
 - Does this build? Does it pass its autopkgtests? Could you provide more details on this?
 - Having different patch extensions (.diff and .patch) is somewhat annoying, but not a blocker. Please fix this (I *personally* prefer .patch).
 - I would slightly reword this, given that we're a bit past GCC-11: "d/p/c++17.patch: fix the build against GCC-11 by porting the code to C++17 (LP: 1988196)"

Thanks in advance, I look forward to reviewing your followup MP! (You could just re-subscribe sponsors to this one.)

[1] https://dep-team.pages.debian.net/deps/dep3/

review: Needs Fixing

« Back to merge proposal