Thank you for the review! I've addressed review comments inline. With Christian sitting next to me we've been through my fixes and he's happy with them. I've updated git branches and updated the PPA for python-certbot ~ppa7 only, as that's the only one that contains a functional change from review feedback; the others are changelog clarifications only. I will test this ~ppa7. Assuming all is good, what's in the PPA, except for changelog text adjustments, will then go into the Xenial proposed pocket ready for wider testing. > TL;DR > - LGTM a few questions for clarification, but no nacks > > ---- > > I took logs for everything non trivial that I did (wonder about) on review. > + = LGTM > ? = ok, but I'd have a question > x = nack, this should be different > > python-acme: > ? rebase for 0.22.2-1ubuntu0.1 > - The bug #1777205 should get a xenial task before upload > - and then the bug be verified on the SRU > - please remember to buildpkg with -v0.4.1-1 Upstream confirmed that 1777205 never affected Xenial (except perhaps in proposed) so a task there would not be accurate. However I agree that it's prudent to verify that it doesn't regress in this SRU during SRU verification, so it's part of the test plan I proposed in comment 103 and I will copy that up to the bug description. > + changelog tweaks > + backport version string > Double ubuntu in the version seems odd, but it seems correct. > It is not part of [2] and an even more special case of [1] I decided to stick to backportpackage(1)'s behaviour as a de facto standard for these SRUs, as ugly as the result is in this case :-/ > x Add SRU explanation to changelog > The version is 0.22.2 the entry calls it "version 0.23 in 16.04" > Seems wrong, either fix or explain why it is that way. > I see that all others e.g. python-certbot is at 0.23 for real > will that mismatch be a problem? Changelog updated with an explanation in f4de847. > python-certbot: > + recommends certbot > ? dh_clean ok but why, would it hurt? > ? pkg-info.mk ok but why, would it hurt? These two changes minimise the diff against the backport source. > ? cli.ini > That is the one to keep the logrotations as is (ok) > I thought from our talk Xenial did that as well, but it didn't > There is nothing about it in pkg/ubuntu/xenial-devel:debian/rules > nor in pkg/import/0.14.2-1:debian/rules > I guess the old default was different, which is ok. > But do we now want/need any conffile cleanup in >=Bionic? > This would be independent to this SRU, but I wanted to ask. cli.ini is a new conffile that was introduced at some point in the certbot binary package only. AFAICT it never existed in the letsencrypt binary package, or at least not in the one in Xenial. We ship the conffile to turn off certbot's internal log rotation to avoid collision with logrotate behaviour. Since we're backporting from Bionic, this conffile will be introduced into Xenial as part of this SRU. The conffile continues to exist in the current development release, so I don't think any further or special conffile handling is needed, since we're only introducing it and not doing anything else with it. As discussed I've add commit 6885f14 to explain the introduction of cli.ini in python-certbot and likewise commit d4e7cff for python-letsencrypt. > + build rules > + install init no-op > ? Restore original timer service installation > The "original" used the older dh_systemd_enable/dh_systemd_start > I think your's work only with newer debhelper, it is not "the original" > I just found you clean that up later - would have been nice to squash those two :-/ Sorry for the pain. I deliberately preserved Michael's original submission in the git branch and kept my changes separate, so I didn't squash anything between these sets together. > + backport version string (that one I like more) > ? use dh_systemd > The old package used install -D --mode=644 for the timer > any reason you have chosen mkdir + cp instead ? It wasn't in the backport source so I had written it afresh. But I now see it was in the previous xenial-proposed upload, and it provides more specifically correct results, so I switched back to using install(1) instead in commit a890737. > That commit also adds whitespace damage above "# Install certbot.timer" The whitespace was intentional and I consider correct, even though it causes a warning highlight, since I wanted a "stanza gap" from the previous commands but it's still part of the same block that in Makefiles are indented by a tab. So the blank line should have a leading tab. I note that it does work without, however. I guess it's a style thing. I can change if you wish. I have changed this in 2a817e8. > + doc paths > + version/changelog changes > ? Add SRU explanation to changelog > you might mention changes to d/rules and d/compat to make the backport working here I deliberately didn't go into detail about the specifics of the backport that are relatively routine and intend for these to be covered by "did a backport". I think the bug and diffs can be inspected for the detail if needed. > python-letsencrypt: > + Remove redundant incorrect d/README.source > ? further changes to changelog ( I have a general statement about those below) Confusion resolved offline. > ? It is not clear to me how that "shimming" takes place, extending the changelog on that would be useful as well. Confusion resolved offline. > python-josepy: > ? changelog/version bumps in general ok > You mentioned the new certbot.timer and such in python-letsencrypt (which doesn't hold the timers) Confusion resolved offline. > Please choose: > - Why did you mention it there? > - Why didn't you mention it here? > I assume it is because of the package takeover letsencrypt->certbot. > But really, that could use some words (more about that below already) Confusion resolved offline. > python-letsencrypt-apache > + cleanup > + changelog > + version bumps > > python-certbot-apache: > + empty dir > + VCS > + cleanup (again ok but why) > + doc building > + version string changes > + doc-base paths > + SRU explanation (nothing more than what was said already) > > > I just realized that the summary changelogs all say: "Log rotation is switched to logrotate via /etc/logrotate.d/certbot" > This whole section could need more why it was done and where the old/new tunables are (cli.ini belonge to package X, the logrotate belongs to package Y - how it was before and now). > And since you carry that in all packages, copy it to all of them once you have a more explaining text. > > There were quite a few which took me some minutes to think, but > eventually LGMT but some cleanups. > > You have done many cleanups which are correct (dh_clean, readme.source, pkg-info.mk, ...). > Those seem good for Disco onwards, but not necessary for an SRU. > They correct, but not necessary, but they increase the amount of code to review or did you only pick those which were ok in Xenial and comparin X->B would have showed up as bad delta? > If that was the intention I'm sure you have the thanks of the SRU reviewer :-) It is my intention to minimise something, and in this case as it's primarily a backport I minimised the B->X delta. This does mean that the X->X SRU delta is more at risk of introducing a regression and we must examine it more carefully. Essentially we already knew that, of course, with this being a backport that includes deliberate multiple packaging changes. > In general the dependencies in d/control seem a bit soft, with all the renames I had expected more breaks/replaces. It is complicated, but it boils down to the upstream-provided shims remaining and a transitional letsencrypt package now requiring the new certbot binary package. This doesn't need any breaks/replaces in itself because the "replacement" certbot binary package provides only binaries on the filesystem at new paths. We happen to accidentally step around that part of the complexity because we're using new binary paths at the same time as the replacement binary packages. > Have you checked in the build logs that the generated python/misc:depends are as you'd want them? I will check this before uploading the final SRU. > All of the above is easy to clean up, so it should not be a huge bump in > bringing this to the SRU queue. > > Note: I'd recommend in general to drop the PPA string from git. Lessons- > learned of someone that had to ask to cancel an SRU with ~ppa in the > name :-) Keep it in "git stash" instead. I agree with this principle in general, but in this case I deliberately kept them to try and keep the shared git branches for so many source packages more easily and obviously mapped to the current and historical state of the PPA, as multiple developers are testing and verifying against it. > > [1]: https://lists.ubuntu.com/archives/ubuntu-devel/2018-July/040406.html > [2]: https://wiki.ubuntu.com/SecurityTeam/UpdatePreparation