Merge ~paelzer/ubuntu/+source/dovecot:merge into ~usd-import-team/ubuntu/+source/dovecot:debian/sid
| Status: | Merged |
|---|---|
| Merge reported by: | Robie Basak |
| Merged at revision: | not available |
| Proposed branch: | ~paelzer/ubuntu/+source/dovecot:merge |
| Merge into: | ~usd-import-team/ubuntu/+source/dovecot:debian/sid |
| Diff against target: |
1569 lines (+1309/-23) 18 files modified
debian/99-mail-stack-delivery.conf (+48/-0) debian/changelog (+519/-0) debian/control (+23/-19) debian/dovecot-core.dirs (+2/-0) debian/dovecot-core.lintian-overrides (+0/-1) debian/dovecot-core.maintscript (+1/-0) debian/dovecot-core.ufw.profile (+23/-0) debian/mail-stack-delivery.README.Debian (+24/-0) debian/mail-stack-delivery.dirs (+2/-0) debian/mail-stack-delivery.postinst (+91/-0) debian/mail-stack-delivery.postrm (+35/-0) debian/mail-stack-delivery.preinst (+56/-0) debian/rules (+9/-3) debian/source_dovecot.py (+39/-0) debian/tests/control (+6/-0) debian/tests/general (+218/-0) debian/tests/testlib.py (+101/-0) debian/tests/testlib_dovecot.py (+112/-0) |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Robie Basak | 2016-06-21 | Approve on 2016-08-18 | |
|
Review via email:
|
|||
Description of the Change
Merge of Dovecot from Debian
Along that I also provided a no change rebuild of a depending package dovecot-antispam at bug 1524526. Once review approves and uploads this it would be great to pick that one up on top.
Changelog as in the Upload:
* Merge with Debian; Remaining Changes:
+ Add mail-stack-delivery
- add package in d/rules, d/control
- add d/*mail-
- d/mail-
config files to a new package namespace.
+ Drop build dependency on libstemmer-dev (universe)
+ Use Snakeoil SSL certificates by default
- d/control: Depend on ssl-cert
- add lsb base dependency
+ Add autopkgtest to debian/tests/*.
- also fixup the tests we had to work with recent dovecot versions
+ Add ufw integration:
- d/dovecot-
- d/rules: install profile in dovecot-core.
- d/control: dovecot-core - suggest ufw.
+ Add apport hook:
- d/rules, d/source_dovecot.py
+ Remove lintian override for drac
* Added Changes:
- Disable dovecot-lucene plugin as it had various issues, has universe
dependencies and is deprecated in favor of solr anyway (LP: #1524526).
* Dropped Changes:
- Add upstart job (that means we no more add it now)
- no more needed upgrade hangling of mail-stack-delivery related to
2.1.7-7 and 1.2.9-1ubuntu8 (both out of scope of any possible
paths now)
- d/dovecot-
in changelog)
- ntpdate Recommends (was missing in former Changelog, now superseeded
by timesyncd which is available by default)
Note: the importer helped a lot to find changelog-only and missing-
- 43baf2d... by ChristianEhrhardt on 2016-06-28
- 8be1eed... by ChristianEhrhardt on 2016-06-28
- dce051a... by ChristianEhrhardt on 2016-06-28
- 800699a... by ChristianEhrhardt on 2016-06-28
- b93ebfa... by ChristianEhrhardt on 2016-06-28
- 4a3e003... by ChristianEhrhardt on 2016-06-28
- 087b554... by ChristianEhrhardt on 2016-06-28
- 0b88d95... by ChristianEhrhardt on 2016-06-28
- 94ebfd6... by ChristianEhrhardt on 2016-06-28
| ChristianEhrhardt (paelzer) wrote : | # |
Hi Robie,
thanks a lot for the thorough review, going to address and fix them one by one.
Challenging is always good to learn, even if it means we might have a few rounds of review iteration.
Yeah integrating that lint check before submission will surely help to be on the same page.
Be sure to mention it in https:/
Back last year it still was the release at least in the repo I still had on disk and the doc said <ubuntu version>. Which is correct as I did use <ubuntu release>, but that said I not only fixed my tags but also added an explanation to the wiki for documentation.
I renamed my tags and hope that goes well without conflict with your locals or so when pushing for merge next time.
I see the benefit of having the changelog and the commits in order.
It is probably too much pain to do so for the logical split for the merge conflicts you already pointed out.
But at least we could - if that helps you - define to have the final merge changelog ordered.
First all "Remaining Changes" with changelog in order to the commits.
Second all "Added Changes" with changelog in order to the commits.
Because reordering text in the changelog is easy that could be done easily.
Would that help you as well or were you particularly interested in the logical split to be ordered?
For Dep8 I can tell you that it went through adt nicely for me, but double check is always better.
You are right about submitting to debian. I'll look into the delta after I addressed your feedback and will submit accordingly.
I already worked on addressing all your great inline feedbacks ...
But I put some tests on my list before uploading again.
P.S. also I run out of power and need to buy a new power adapter first :-/
| Robie Basak (racb) wrote : | # |
On Tue, Jun 28, 2016 at 07:19:45AM -0000, ChristianEhrhardt wrote:
> I see the benefit of having the changelog and the commits in order.
> It is probably too much pain to do so for the logical split for the merge conflicts you already pointed out.
> But at least we could - if that helps you - define to have the final merge changelog ordered.
> First all "Remaining Changes" with changelog in order to the commits.
> Second all "Added Changes" with changelog in order to the commits.
> Because reordering text in the changelog is easy that could be done easily.
> Would that help you as well or were you particularly interested in the logical split to be ordered?
This would help I think. Shall we try it and see? I end up looking at
three things side-by-side: the old logical delta, the new logical delta,
and the new changelog. Perhaps my pain is that all three were in
different orders. If "Remaining Changes" were in the same order as the
new logical delta order, that would make it easier to review.
> You are right about submitting to debian. I'll look into the delta
> after I addressed your feedback and will submit accordingly.
Sure, but don't worry about it this time. We can do it on the next
merge. As long as we're making incremental improvements each time, I
don't mind not doing it all at once.
> Thanks, I misinterpreted where this belongs to, but right now I think you as well :-)
>
> Due to your feedback I thought more about it.
> I think what is needed for d/p/dovecot_
>
> I think I found what this is really for, see:
> debian/
> debian/
Ah right, sorry. I wonder though if it is still needed now? Sometimes
it's a transitional thing. But now that sourcing init-functions is
standard, I'd expect something in Debian at a deeper level to depend on
the right thing so every single package doesn't have to do it (or, at
least, for dh_installinit to automatically add it). But I haven't
looked.
- c204b18... by ChristianEhrhardt on 2016-06-28
| ChristianEhrhardt (paelzer) wrote : | # |
> > Thanks, I misinterpreted where this belongs to, but right now I think you as
> well :-)
> >
> > Due to your feedback I thought more about it.
> > I think what is needed for d/p/dovecot_
> already in the build dependencies (and it is only used at build time in
> configure.ac).
> >
> > I think I found what this is really for, see:
> > debian/
> ensure that this file is present.
> > debian/
>
> Ah right, sorry. I wonder though if it is still needed now? Sometimes
> it's a transitional thing. But now that sourcing init-functions is
> standard, I'd expect something in Debian at a deeper level to depend on
> the right thing so every single package doesn't have to do it (or, at
> least, for dh_installinit to automatically add it). But I haven't
> looked.
While I agree that one would think it is always there it not necessarily is.
It is not auto-added to the list of dependencies in debian-sid.
I was able to leave the init script disfunctional in a debian-sid container by running
"apt-get remove lsb-base cron logrotate"
So I think it is a valid fix to add - as well as to pass to debian.
With all my breaks today I have a hard time to make progress, but I hope to get the testing done until this evening for a new push to the MP.
- 9903570... by ChristianEhrhardt on 2016-06-28
| ChristianEhrhardt (paelzer) wrote : | # |
Ok, now I tested it once more plus the new conffile handling in modified and unmodified case.
I think I'm ready to push the updated merge branch.
Please let me know if the tags arrived as they should
| ChristianEhrhardt (paelzer) wrote : | # |
pushed all changes, ready for the next round of review
| Robie Basak (racb) wrote : | # |
Here's a quick review. Just a couple of inline points. Sorry, I may never have told you about dh_builddeb(1) and maintscript files.
I want to finish for the day now, but am not quite finished reviewing. But I thought it might be useful to give you this now.
| ChristianEhrhardt (paelzer) wrote : | # |
Hi Robie,
it is clearly not your fault for not telling, maybe mine for not knowing, but in this case especially googles fault for "debian remove conffile" hitting plain dpkg-maintscrip
- 6d94694... by ChristianEhrhardt on 2016-07-01
| ChristianEhrhardt (paelzer) wrote : | # |
Hi,
almost forgot to push that, but just found in a console that also the adt completed in the meantime.
Before I already tested in a container that conffile removal/backup is still working.
| ChristianEhrhardt (paelzer) wrote : | # |
Ping on this review, I checked but as far as I could see all open issues were resolved.
It seems we just were both too busy in the meantime.


Thank you for tackling a really challenging merge!
You've done a good job of following the process with reconstruct, deconstruct and logical deltas, and this made it much easier for me to review.
Please note that our convention is to use {reconstruct, deconstruct, logical} /<version> (with standard dep14 substitutions). This allows us to store multiple tags associated with multiple versions for archival purposes. I've also written my lint tool to expect these tags so it breaks otherwise. Temporarily I just created the tags locally based on what you already had (you had the right tags, just not with the standard names). I hope to publish my lint tool soon, so it can be run before submitting a merge proposal.
A second note that has not occurred to me before: it would be helpful if the ordering of changelog entries matched the order of the git commits in the proposed branch. This would make it easier to review, but I appreciate that re-ordering can be quite painful because of merge conflicts, so I'm not sure if it's reasonable to require this in our process.
Further comments inline, with some required fixes. I haven't checked if this passes dep8, but will do that before uploading.
I think some of the delta we're applying is relevant to Debian and should be sent there or dropped. I also want to propose the deprecation of the mail-stack-delivery stuff, and then for us to drop that delta too to make the package simpler. But I'm not proposing to do any of this for this particular merge. You've cleaned this package up considerably, and I think it's OK to finish everything off in later iterations rather than trying to do everything all at once.