Cherry-pick proposal

Wed Apr 29 13:49:44 UTC 2020

That we do not run CI explicitly before such cherry picks does not mean
that there is no CI run at all. The CI is triggered when the cherry-
picked commit is merged. If we were checking the status of the 1.1.1
branch CI runs periodically (with a bot sending e-mails about failures
to openssl-project or another list perhaps?). We could then quickly fix
it if cherry-pick introduced a regression.

I also see only a single case where a cherry pick introduced regression
on the 1.1.1 branch during the April. So I do not think we have a
serious issue requiring the mandatory process change immediately.
On the other hand I have no hard problems with the Matthias' proposal
either.

Also perhaps we should just make a recommendation on which kind of
commits (although cherry-picking cleanly) should have a separate PR.
For example commits touching crypto implementations and the EVP layer
should have separate 1.1.1 PRs even though they cherry-pick cleanly. 

And of course there is no requirement to not do the cherry-pick PR even
if the cherry-pick is clean. It is up to the committer discretion to
decide whether it is needed or not.

Tomas

On Wed, 2020-04-29 at 13:28 +0000, Dr. Matthias St. Pierre wrote:
> I completely agree with Nicolas reasoning. But we should try to keep
> things simple and not
> add too many regulations. What do you think of the following
> formulation?
>  
> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 
> For change requests which target both the master and the
> OpenSSL_1_1_1-stable branch,
> the following procedure should be followed:
>  
> - First, a pull request needs to be opened against the master branch
> for discussion.
>   Only after that pull request has received the necessary amount of
> approvals,
>   a separate pull request can be opened  against the OpenSSL_1_1_1-
> stable branch.
>  
> - A separate pull request against the OpenSSL_1_1_1-stable branch is
> required.
>   This holds - contrary to common practice - even if the change can
> be cherry-picked
>   without conflicts from the master branch. The only exception from
> this rule are
>   changes which are considered 'CLA: trivial', like e.g.
> typographical fixes.
> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 
>  
> Matthias
>  
>  
> From: openssl-project <openssl-project-bounces at openssl.org> On Behalf
> Of Nicola Tuveri
> Sent: Wednesday, April 29, 2020 3:05 PM
> To: openssl-project at openssl.org
> Subject: Re: Cherry-pick proposal
>  
> I can agree it is a good idea to always require backport as a
> separate PR, with the following conditions:
> - unless it's a 1.1.1 only issue, we MUST always wait to open the
> backport-to-111 PR until after the master PR has been approved and
> merged (to avoid splitting the discussions among different PRs, which
> make review and revisiting our history very hard)
> - trivial documentation changes, such as fixing typos, can be
> exempted
> 
> It must be clear that although things have changed a lot in the inner
> plumbings, we have so far managed to keep crypto implementations very
> much in sync between 1.1.1 and master, by applying the policy of
> "first merge to master, then possibly backport".
> 
> What I am afraid of in Bernd's proposal, and recent discussions, is
> that committers might be tempted to open PRs changing implementations
> against 1.1.1 first (to avoid frequent rebases due to unrelated
> changes) and let the `master` PR stagnate indefinitely because it
> feels like too much hassle to keep up with the development pace of
> master if your PR collaterally changes certain files.
> 
> An example of what can go wrong if we open a 1.1.1 PR concurrently
> with a PR for master can be seen here:
> - `master` PR: https://github.com/openssl/openssl/pull/10828
> - `1.1.1` PR: https://github.com/openssl/openssl/pull/11411
> 
> I highlight the following problems related to the above example:
> - as you can see the `1.1.1` has been merged, even though the
> `master` one has stalled while discussing which implementation we
> should pick. (this was my fault, I should have applied the `hold`
> label after stating that my approval for 1.1.1 was conditional to
> approving the `master` counterpart)
> - discussion that is integral part of the decision process was split
> among the 2 PRs, with over 40 comments each
> - there is no clear link between the `master` PR and the `1.1.1` PR
> for the same feature (this makes it very difficult to review what is
> the state of the "main" PR, and if we or someone else in some months
> or years needs to go check why we did things the way we did, will
> have a hard time connecting the dots)
> 
> I also think that the proposal as written is a bit misleading: I
> would very like to keep seeing in 1.1.1 a majority of commits cherry-
> picked from commits merged to master, with explicit tags in the 1.1.1
> commit messages (this helps keeping the git history self-contained
> without a too strong dependency on GitHub).
> I would rephrase the proposal as follows:
> 
>     Merge to 1.1.1 should only happen after approval of a dedicated
> PR targeting the OpenSSL_1_1_1-stable branch.
> 
> Potential amendments that I'd like the OTC to consider are:
> a) before the end of the sentence add ", with the optional exclusion
> of trivial documentation-only changes"
> b) after the end of the sentence add "In composing backport pull
> requests, explicit cherry-picking (`git cherry-pick -x`) of relevant
> commits merged to `master` or another stable branch is recommended
> and encouraged whenever possible."
> c) adopt a more general statement:
> 
>     Merge to any stable branch should only happen after approval of a
> dedicated PR targeting specifically that branch.
> 
> 
> 
> 
> So, in summary, I would agree with the proposal, as I definitely
> think Bernd has a good point about running the 1.1.1 CI for things we
> think should be backported, but requires careful consideration of
> additional requirements to avoid duplicating review efforts,
> splitting discussions that should be kept together, or disrupting our
> processes making 1.1.1 and master diverge more than necessary.
> 
> 
> Cheers,
> 
> 
> Nicola
>  
> On Wed, 29 Apr 2020 at 14:08, Matt Caswell <matt at openssl.org> wrote:
> > The OTC have received this proposal and a request that we vote on
> > it:
> > 
> > I would like to request that we do not allow cherry-picks between
> > master
> > and 1.1.1-stable because these two versions are now very different,
> > if a
> > cherry-pick succeeds, there is no guarantee that the result will
> > work.
> > Because we have no CI for the cherry-pick. If a cherry-pick is
> > needed, a
> > new PR for 1.1.1 should be done and approved independently.
> > 
> > Before starting a vote I'd like to provide opportunity for
> > comments, and
> > also what the vote text should be.
> > 
> > Thanks
> > 
> > Matt
-- 
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]