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)

[dmitshur]

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]

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.

[FiloSottile]

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]

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]

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.

[gopherbot]

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

[gopherbot]

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

[dmitshur]

@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]

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.