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

api: API check test should disallow API additions after an api/go1.N.txt is created (currently it does this only after a VERSION file is added) #43956

Closed
dmitshur opened this issue Jan 27, 2021 · 8 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jan 27, 2021

CL 276454 promoted api/next.txt to api/go1.16.txt, and all API additions in that file were reviewed in #43407.

When we started cutting Go 1.16 RC 1, we found out (via release testing) that there was a skew between api/go1.16.txt and the actual API.

This issue is about improving the process so we notice this sooner, to avoid the possibility of incurring a delay in the RC release process.

CC @golang/release, @ianlancetaylor.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jan 27, 2021
@dmitshur dmitshur added this to the Backlog milestone Jan 27, 2021
@dmitshur
Copy link
Contributor Author

The cause appears to be that as long as the runtime.Version() contains "devel" (which it does until a VERSION file is added on a release branch, something that happens during the rc1 release and its tests), API additions are considered okay, but API removals are not.

The fix here may to be change it so that both API additions and removals are not okay after the go1.N.txt file is made for a given release in development.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 27, 2021
@dmitshur dmitshur changed the title api: detect skew between api/go1.N.txt and actual API before starting to cut an RC api: API check test should disallow API additions after an api/go1.N.txt is created (currently it does this only after a VERSION file is added) Jan 27, 2021
@FiloSottile
Copy link
Contributor

I have a counter-proposal: remove next.txt entirely, and make API skew fail TryBots at all times.

The current lifecycle of an API change is:

  • make the API change in a CL, maybe inadvertently
  • optionally, someone updates next.txt because the all.bash output is getting annoying
    • this cycle it was @rsc, other cycles next.txt went completely unused
  • next.txt is copied wholesale into go1.X.txt without review
  • an issue is opened to review go1.X.txt

This has two major shortcomings: it lets API changes happen inadvertently in the original CL, and lets skew happen after the review. The latter would be addressed by the current proposal, the former wouldn't. The current process also involves manual steps that serve no purpose AFAICT.

If we eliminate next.txt and block any skew, then the lifecycle becomes:

  • make an API change and add a line to go1.X.txt in the same CL, acknowledging intent to add an API
  • an issue is opened to review go1.X.txt

If people would prefer not getting heckled by all.bash while experimenting, we can make all.bash just output the diff with no error, and only error out on the builders, so the go1.X.txt file can be changed only once before mailing the CL.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jan 27, 2021
@dmitshur
Copy link
Contributor Author

Copying @rsc's comment from #43407 (comment) here:

The api check intentionally doesn't fail on master, only on release branches, to avoid forcing any CL adding API to update next.txt in the same CL (needless merge conflicts). But anyone running all.bash should see it.

We could potentially flip a bit somewhere after the mid-freeze API review to make the test start failing on additions then. But we shouldn't make it fail when the tree is open.

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 28, 2021

Right, I disagree that forcing something as important as adding an API to explicitly opt-in is bad.

The merge (or revert) conflicts came up before about release notes, but we switched anyway to release notes in CLs, and I didn't hear about much annoyance from it.

I don't think anyone looks at the all.bash output, since we went whole cycles with an empty next.txt.

@dmitshur dmitshur self-assigned this Apr 29, 2021
@gopherbot
Copy link

Change https://golang.org/cl/315350 mentions this issue: cmd/api: disallow silent API additions after api/go1.n.txt is created

@gopherbot
Copy link

Change https://golang.org/cl/315349 mentions this issue: api: update next.txt

@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 29, 2021

@FiloSottile After taking some time to think about it carefully, I'm in agreement with the suggestion you made. I, too, now believe there is a good chance that removing our use of next.txt and requiring an api/go1.txt to exist and be complete during the entire development cycle has favorable trade-offs. The attention paid to API review would be be spread more evenly during the entire cycle, rather than more towards the end, which feels like an improvement. However it will affect all Go developers during the development cycle, and probably needs a little more discussion/feedback than what's happened in this issue so far (and we'd want to communicate it more widely to golang-dev@ to give people a heads up).

I think rather than describing it as a "counter-proposal" (which I interpret to mean we should do something incompatible/in a different direction), it's a suggestion to take two compatible steps in the same direction instead of one smaller step.

Where we currently are, API additions are reported quietly (no loud test failure):

  • from development tree opening all the way until RC 1

If we make the change originally suggested in #43956 (comment), it becomes:

  • from development tree opening all the way until Beta 1

If we make the change later suggested in #43956 (comment), then:

  • never

For now, I want to ensure we don't get a repeat of the situation that happened during the Go 1.16 cycle (what this issue was filed about), and it seems everyone's in agreement to disallow silent API additions after Beta 1. Let's do that first, since it's seemingly quick and non-controversial, and consider your suggestion afterwards. It can only apply to a future cycle anyway (too late for 1.17), so we can still consider it before the tree opens for Go 1.18 development.

I mailed CL 315350 which will fix this issue and I think we can get it in before Go 1.17 Beta 1. If that happens, let's follow up with your suggestion in a separate issue (or proposal).

One more thing: if CL 315350 is submitted, implementing your suggestion will become a matter of adding a "api/go1.n.txt" file earlier in the release cycle than Beta 1 (perhaps after all "early in cycle" CLs have landed). If we find it's too disruptive and not helping as expect, we can simply remove that file, and re-add it at Beta 1 again. So it's a small diff to try it and adjust based on findings.

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 Apr 29, 2021
@dmitshur dmitshur added this to In Progress in Go Release Team Apr 29, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 29, 2021
gopherbot pushed a commit that referenced this issue Apr 30, 2021
There's no reason not to, and it'll help me test an upcoming fix
for #43956. The API additions look reasonable to me, and they'll
go through a more comprehensive API audit during the freeze.

Change-Id: I0daa6e978b199d69568f5100fdfc1b4bcfaeaef2
Reviewed-on: https://go-review.googlesource.com/c/go/+/315349
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Go Release Team automation moved this from In Progress to Done May 3, 2021
@dmitshur
Copy link
Contributor Author

I found/realized another benefit of this change is that it reduces friction during early pre-release testing.

Previously, the API check would always fail since the VERSION file is written by the time release tests run, and it required either manually skipping over the false-positive, or testing on top of a "DO NOT SUBMIT because it's too early for it" CL that preemptively creates api/go1.n.txt file.

Now that test no longer runs before the api/go1.n.txt file is created, behavior during release tests is the same as during normal tests.

@golang golang locked and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Archived in project
Development

No branches or pull requests

3 participants