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 <[email protected]> On Behalf > Of Nicola Tuveri > Sent: Wednesday, April 29, 2020 3:05 PM > To: [email protected] > 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 <[email protected]> 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.]
