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

x/build/cmd/relnote: missed API changes #62376

Closed
rsc opened this issue Aug 30, 2023 · 23 comments
Closed

x/build/cmd/relnote: missed API changes #62376

rsc opened this issue Aug 30, 2023 · 23 comments
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

Somehow we missed the addition of binary.NativeEndian, which is listed in the API files, when preparing the Go 1.21 documentation. The existence in the API files marked with #57237 should have provoked relnote to insist on a reference to go.dev/issue/57237 in go1.21.html, but there is no such mention. #62349 tracks updating the docs. This issue is about fixing the process that led to the incomplete docs.

Just before the release we noticed that we'd forgotten to document the new runtime.PanicNilError, and again it seems like relnote should have noticed and caught that for us.

Marking a release blocker for Go 1.22 because we need to understand why we are dropping release notes on the floor. Either we are not using relnote often enough or correctly or there is a bug in it.

@rsc rsc added this to the Go1.22 milestone Aug 30, 2023
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Aug 30, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 30, 2023
@dmitshur dmitshur changed the title x/build/cmd/relnotes: missed API changes x/build/cmd/relnote: missed API changes Aug 30, 2023
@ianlancetaylor
Copy link
Member

Looking at cmd/relnote, I think it's this line:

				if issue := gh.Issue(num); issue != nil && !issue.ClosedAt.Before(cutoff) && hasLabel(issue, "Proposal-Accepted") {

Note `ClosedAt. The issue in question, #57237 was not closed. I think that it wasn't closed because there were two parts to it, one for encoding/binary and one for x/sys/cpu. The two independent CLs referenced it but didn't close it. I just closed it today as I think the work is complete.

@heschi heschi moved this to Planned in Go Release Sep 5, 2023
@dmitshur
Copy link
Contributor

I spotted another source of possible inaccuracies in relnote while looking into this earlier: it uses a fairly crude heuristic for computing the start and end of each major Go release cycle, exactly 6 months apart (source). That heuristic is still fairly accurate, but the development cycle is now slightly expanded due to recent changes including a late freeze and early thaw, which may also contribute to some items being missed if they were at the very start or end of a cycle.

@jba Are you interested in taking this issue since you're working on #64169 now and this seems related?

@dmitshur dmitshur removed their assignment Nov 15, 2023
@jba jba self-assigned this Nov 15, 2023
@jba
Copy link
Contributor

jba commented Nov 15, 2023

Yes, I'll own it.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/542855 mentions this issue: cmd/relnote: warn about unclosed proposals with CLs

gopherbot pushed a commit to golang/build that referenced this issue Jan 10, 2024
Occasionally a proposal is implemented, but the associated issue isn't
closed. To ensure we don't forget to write a release note in that
case, print a warning for all unclosed proposal issues that are
mentioned by at least one CL.

For golang/go#62376.

Change-Id: I3cca9065a3dd34af3c8bbbaf9c09dcdfaad2c9e1
Reviewed-on: https://go-review.googlesource.com/c/build/+/542855
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
@rsc
Copy link
Contributor Author

rsc commented Apr 22, 2024

Marking this as a release blocker for Go 1.23 because the general problem of missing API changes may still remain. I don't understand how relnote can work on the new markdown files in the RELNOTE=yes CL comment case, since there are no CL numbers in the new markdown files for it to look for. Or are there?

@jba
Copy link
Contributor

jba commented Apr 26, 2024

We shouldn't miss API changes, because there is a test in the main repo. We may miss other things, like changes to commands.

RELNOTE= comments are obsolete in the current design; authors should instead add a doc/next/*.md file, possibly with a TODO. The relnote todo command would print these out.

This wasn't communicated clearly in the README; fixed in https://go.dev/cl/582075.

The relnote todo will look for these, since they may keep happening (https://go.dev/cl/582097). As you say, we can't point to places in the markdown files that need to be modified, but at least we won't drop anything.

We should probably also ask authors of release notes about accepted proposals to put the issue number in a comment in the note. Then we can look for CLs that mention accepted proposals that don't appear in the notes. That is future work.

@dmitshur dmitshur moved this from Planned to In Progress in Go Release Apr 26, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 26, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/582097 mentions this issue: cmd/relnote: improve todo subcommand

gopherbot pushed a commit to golang/build that referenced this issue Apr 29, 2024
- The `todo` subcommand now looks at CLs with "RELNOTE=" comments,
  as well as TODO comments in doc/next/*.md files.

- The cutoff date must now be provided manually on the command line,
  avoiding errors in calculating it automatically.

- It is now an error to run `relnote` without a subcommand.

For golang/go#62376.

For golang/go#62376.

Change-Id: I34cf3d9dc7537daf7d0b479245b4c870106ac987
Reviewed-on: https://go-review.googlesource.com/c/build/+/582097
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@cherrymui
Copy link
Member

@jba @rsc is there anything else that needs to be done for this? Thanks.

@jba
Copy link
Contributor

jba commented May 8, 2024

This:

We should probably also ask authors of release notes about accepted proposals to put the issue number in a comment in the note. Then we can look for CLs that mention accepted proposals that don't appear in the notes.

For this cycle, we can at least look for CLs that mention accepted proposals. Then the release team can manually verify that they have release notes. I will improve relnote todo to do that, probably by the end of the week.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

Note that https://go-review.googlesource.com/c/build/+/410244 already added "look for CLs that mention accepted proposals", but it seems to have been disabled. Re-enabling it would be great.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

Also:

RELNOTE= comments are obsolete in the current design; authors should instead add a doc/next/*.md file, possibly with a TODO. The relnote todo command would print these out.

It's not reasonable to expect everyone to be up to speed on the new process. If people write RELNOTE= comments, we should still pick them up instead of blaming people for not knowing the process changed.

(RELNOTE=/RELNOTES= is an API, and we shouldn't make a breaking change here. It is fine to add new API, just not fine to delete old API.)

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

I also sent CL https://go-review.googlesource.com/c/build/+/584303 to restore the cutoff date calculation.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

I haven't written release notes yet for the synctimer proposal. I'd like to see relnote catch that before I do.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

I found https://go-review.googlesource.com/c/build/+/582097 which says:

The cutoff date must now be provided manually on the command line, avoiding errors in calculating it automatically.

This seems completely backward to me. If the automated calculation has a bug, then that's all the more reason to fix the bug instead of falling back to buggy humans doing work that a computer can do. I should be able to run relnote todo to check what I might need to do without having to get out a calendar.

If calendrical calculations aren't good enough, the code can look for release tags in the maintner logs. It clearly has all the information it could possibly need.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

Apologies for the issue comment storm. I seem to have jumped into the middle of work on relnote and it took me a few messages to get up to speed. I see now also that the RELNOTE= comments are processed again.

If we can get to the point where relnote todo (with no arguments) will tell me what release notes still need to be written, I know I have at least two proposal-related ones (synctimer and godebug). But I'd like to see the tool tell me what else I am missing too.

@jba
Copy link
Contributor

jba commented May 9, 2024

I just landed some CLs that I think do what you're asking. They will print a TODO for any accepted proposal mentioned by a CL merged after the cutoff. The old relnote cared about when the proposal issue was closed, but the new code does not: I think that any work done for a proposal, whether that proposal was closed years ago or is still open, is potentially worthy of a release note.

@jba
Copy link
Contributor

jba commented May 9, 2024

If the automated calculation has a bug, then that's all the more reason to fix the bug instead of falling back to buggy humans doing work that a computer can do.

I'll take that on. I was using the first line of https://go.dev/doc/devel/release#go1.22.0. @dmitshur Where can I get that information in a more machine-readable form?

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

Thanks. That info is in x/website/internal/history, but you can't import it. You can do the https fetch to go.dev though. It's template-generated so any simple regexp you use is likely to keep working.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

Or you could look in master for the git commit that tags the release and use that date. But I thought the problem was that the dev cycle might have started before the release?

There's not going to be one exact date that is right necessarily. It's better to be on the early side than the late side, since early means false positives, and those can be silenced, but false negatives are never noticed. The way we used to deal with false positives is to add a comment to the html file mentioning the CL or issue, so that the tool would see that it was 'handled'. Something like that probably works here too.

@dmitshur
Copy link
Contributor

dmitshur commented May 9, 2024

I agree that just using the release date of the .0 release isn't likely to be very useful. The updated release cycle, tree reopening is nominally scheduled for third week of July or January (see go.dev/s/release#…-work-on-the-next-release-begins). We have generally hit that target, but I don't think it's possible to guarantee it.

To add to Russ' comment above, maybe you can deal with false positives from looking a bit too far back by checking the release notes for the prior release. But you'd need to fetch them either from the x/website repo, or from go.dev itself, which has overhead.

There are probably other possible approaches. For instance, each development cycle normally begins with a CL that updates the constant in the internal/goversion package (e.g., CL 557155 did it for #64340 to reopen the tree for 1.23 development; this is tracked in #40705), maybe that's something to take advantage of.

@rsc
Copy link
Contributor Author

rsc commented May 9, 2024

This is something we can just fix once at the start of each cycle by manually overriding the few false positivies.
The cutoff date is also something we can hard-code once per cycle in the tool, or we could even read it out of the md files.

@dmitshur
Copy link
Contributor

@jba Checking on from a weekly release meeting. Is there more to do before considering this resolved? Thanks.

@jba
Copy link
Contributor

jba commented May 15, 2024

Not as far as I know.

@jba jba closed this as completed May 16, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Release May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Archived in project
Development

No branches or pull requests

6 participants