Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

12.x reverts #31782

Closed
wants to merge 6 commits into from
Closed

Conversation

MylesBorins
Copy link
Contributor

This includes are revert of #31063 and 65e5a8a as both seemed to have caused regression in v12.16.0

#31752
#31249 (comment)

I believe that 65e5a8a should be able to land with a fix in the next 12.x release. As This includes are revert of #31063 is SemverMinor it would have to wait until April to get out in the next release.

As such I wanted to open this PR to get consensus on landing the reverts before doing so.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Feb 13, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 13, 2020

@MylesBorins MylesBorins mentioned this pull request Feb 13, 2020
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit 65e5a8a.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit f5cd6d7.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit 5e232f5.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit dea40f8.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit 5fcf542.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins added a commit that referenced this pull request Feb 14, 2020
This reverts commit fcbd2d2.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins
Copy link
Contributor Author

Landed in f0b2d87...2ea5c91

@MylesBorins MylesBorins deleted the 12.x-reverts branch February 14, 2020 07:30
@addaleax
Copy link
Member

@MylesBorins I think it sets a bad precedent that we’re reverting changes on v12.x only when they should also be reverted on master…

#31784 does that partially, and I’ll open a partial revert for 65e5a8a (along the lines that I’ve requested in the v12.16.1 proposal), but doing it on release staging is just bound to get messy and we should really avoid that if we can

@jasnell
Copy link
Member

jasnell commented Feb 14, 2020

Oy, yeah, when reviewing this I hadn't realized that the issue also affected master and 13.x... the PR text indicated a regression only in 12.x. These really should have been addressed in master first then backported to keep things from getting messy.

addaleax pushed a commit that referenced this pull request Feb 14, 2020
This reverts commit f5cd6d7.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 14, 2020
This reverts commit 5e232f5.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 14, 2020
This reverts commit dea40f8.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 14, 2020
This reverts commit 5fcf542.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 14, 2020
This reverts commit fcbd2d2.

PR-URL: #31782
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 15, 2020

Also, doesn't the 48 hour rule apply to release branches? This was opened and landed on the same day (edit: within 8 hours even.) That doesn't really give people time to comment.

@MylesBorins
Copy link
Contributor Author

@jasnell the choice to revert was exactly because other lines were also affected. There were no fixes added here, but rather reverting the changes that introduced the regressions before the fixes landed on 13.x and master in order to respect the LTS policy. Even if it "reintroduced" regressions that were fixed by the change, that behavior is at least what is currently expected on LTS.

@bnoordhuis to the best of my knowledge, and how I've personally been working for years, the 48 hour rule does not apply to release branches. The release branches are maintained by the release / LTS team and have their own rules that govern them. In the case of this reversion PR there was an attempt to get a fix out within 24 hours of recognizing that there were regressions on 12.x. If the 48 hour rule applied it wouldn't be possible to do quick releases and imho there would be significant friction introduced to the general process. The reason to open this PR is for quick sign off as well as auditability.

FWIW, if you would like to discuss changes to current process or think it would be beneficial to revisit the existing process I think it is totally worth doing so. Opening an issue in the release repo would be a good way to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants