RE: Cherry-pick proposal
Independently of the outcome of the vote I think we should adopt the habit to wait with the merging of a backport PR until the original PR has been approved and merged. As an indicator and reminder, I added a new label (hold: wait for master), which I applied to #12417. https://github.com/openssl/openssl/labels/hold%3A%20wait%20for%20master Matthias > -Original Message- > From: openssl-project On Behalf Of Matt > Caswell > Sent: Thursday, July 9, 2020 11:13 AM > To: openssl-project@openssl.org > Subject: Re: Cherry-pick proposal > > > > On 02/06/2020 15:29, Matt Caswell wrote: > > > > There's been no further discussion on this for quite a while, so I will > > start an OTC vote based on the vote text proposed by Matthias and report > > back the results here. > > Sorry, I forgot to report back. The final result was: > > +1: 4 > -1: 4 > 0: 3 > > So the result was tied. According to our bylaws this means that the vote > does not pass. > > Matt
Re: Cherry-pick proposal
On 02/06/2020 15:29, Matt Caswell wrote: > > There's been no further discussion on this for quite a while, so I will > start an OTC vote based on the vote text proposed by Matthias and report > back the results here. Sorry, I forgot to report back. The final result was: +1: 4 -1: 4 0: 3 So the result was tied. According to our bylaws this means that the vote does not pass. Matt
Re: Cherry-pick proposal
On 29/04/2020 14:28, Dr. Matthias St. Pierre wrote: > - 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. > There's been no further discussion on this for quite a while, so I will start an OTC vote based on the vote text proposed by Matthias and report back the results here. Matt
Re: Cherry-pick proposal
Any change to the review gate check we have in place now that lowers it will certainly not get my support. If anything, that check before code gets approved should be raised, not lowered. Tim. On Thu, 30 Apr 2020, 1:24 am Salz, Rich, wrote: > I suspect that the primary motivation for this proposal is that PR’s > against master often become stale because nobody on the project looks at > them. And then submitters are told to rebase, fix conflicts, etc. It gets > disheartening. If that is the motivation, then perhaps the project should > address that root cause. Here are two ideas: > > > >1. Mark’s scanning tool complains to the OTC if it has been “X” weeks >without OTC action. I would pick X as 2. >2. Change the submission rules to be one non-author OTC member review >and allow OTC/OMC to put a hold for discussion during the 24-hour freeze >period. That discussion must be concluded, perhaps by a vote, within “Y” >weeks (four?). > > > > > > >
Re: Cherry-pick proposal
I suspect that the primary motivation for this proposal is that PR’s against master often become stale because nobody on the project looks at them. And then submitters are told to rebase, fix conflicts, etc. It gets disheartening. If that is the motivation, then perhaps the project should address that root cause. Here are two ideas: 1. Mark’s scanning tool complains to the OTC if it has been “X” weeks without OTC action. I would pick X as 2. 2. Change the submission rules to be one non-author OTC member review and allow OTC/OMC to put a hold for discussion during the 24-hour freeze period. That discussion must be concluded, perhaps by a vote, within “Y” weeks (four?).
Re: Cherry-pick proposal
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 +, 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 On Behalf > Of Nicola Tuveri > Sent: Wednesday, April 29, 2020 3:05 PM > To: openssl-project@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
RE: Cherry-pick proposal
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 On Behalf Of Nicola Tuveri Sent: Wednesday, April 29, 2020 3:05 PM To: openssl-project@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
Re: Cherry-pick proposal
I find the idea of *mandatory* separate PRs for master and 1.1.1 awful. There's still a lot of code that hasn't changed at all. CMS, BIO, ... We already have the policy that if we get a clean cherry-pick, we go with it, otherwise we make a separate PR. I've followed that guiding rule for a long time. As for always starting with master, I would say that it depends. If an issue has been identified in 1.1.1, I usually submit a PR against 1.1.1, because that's usually cleaner, i.e. the person making the issue can easily pick the fixing PR and try it out without having to wade through unrelated 3.0 things that may or may not work for them. Or to put it in harsher words, submitting a fix against master for an issue found in 1.1.1 is *user* *unfriendly*. With such a PR, I then cherry-pick to master if that's clean, or submit an up-port PR. Cheers, Richard On Wed, 29 Apr 2020 15:04:57 +0200, Nicola Tuveri wrote: > 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 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,
Re: Cherry-pick proposal
I think we changed enough things in the test infrastructure that there is a chance of creating subtle failures by merging cherry-picked commits from master directly. >From the burden perspective, from my point of view having a separate PR that ran all the CI without failures is actually a benefit: I can do some minimal testing on my machine before the final merge, instead of having to push a branch to my personal github fork to run travis and appveyor to test weird build options on platforms I don't have access to. If we stick to opening the PR for backporting only after master is completely approved, there shouldn't be too big a burden for the original reviewers either: we can use `fixup!` commits if trivial changes are required to clearly highlight what was changed compared to the original PR for master, and other than that it's just a matter of checking that nothing else changed from the originally approved changes. Approvals conditional to the CI passing can also help to further reduce the burden of the grace period for backport PRs. Nicola On Wed, 29 Apr 2020 at 14:24, Dr Paul Dale wrote: > My concern is are 1.1.1 and 3.0 really all that different? > The core is quite different but the cryptographic algorithms aren’t. CMS, > x509, …? > > I’d rather not introduce a burden where it isn’t necessary. > > Pauli > -- > Dr Paul Dale | Distinguished Architect | Cryptographic Foundations > Phone +61 7 3031 7217 > Oracle Australia > > > > > On 29 Apr 2020, at 10:08 pm, Matt Caswell 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 > > >
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 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 >
Re: Cherry-pick proposal
My concern is are 1.1.1 and 3.0 really all that different? The core is quite different but the cryptographic algorithms aren’t. CMS, x509, …? I’d rather not introduce a burden where it isn’t necessary. Pauli -- Dr Paul Dale | Distinguished Architect | Cryptographic Foundations Phone +61 7 3031 7217 Oracle Australia > On 29 Apr 2020, at 10:08 pm, Matt Caswell 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