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.

Sign up for GitHub

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

Jump to bottom

std,cmd: add test to ensure that all bundled packages are in sync with go.mod version #41409

Closed
dmitshur opened this issue Sep 15, 2020 · 6 comments
Closed

std,cmd: add test to ensure that all bundled packages are in sync with go.mod version #41409

dmitshur opened this issue Sep 15, 2020 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.16

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 15, 2020 •
edited
Loading

We've had issues in the past where version skew could be introduced if people forgot to update one part of the distribution without also updating another (e.g., versions in src/go.mod and src/cmd/go.mod). A very effective solution was adding a test that fails when skew is detected.

I believe we still need to address this for net/http's bundled copies of golang.org/x/net/http2 and golang.org/x/net/internal/socks. Right now, it's possible to update the bundled copy independently from the src/go.mod version, but it's not clear that flexibility is something we want to keep.

See #41375 where this came up. Also relevant is #25285.

I plan to revisit my review of CL 189818 (which fixes issue #32031), the trade-offs have changed by now (especially given #41330).

/cc @toothrot @bcmills @FiloSottile
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 15, 2020
@dmitshur dmitshur added this to the Go1.16 milestone Sep 15, 2020
@dmitshur dmitshur changed the title net/http: h2_bundle.go (and socks_bundle.go) can get out of sync with src/go.mod's x/net/http2 version net/http: h2_bundle.go (and socks_bundle.go) can get out of sync with src/go.mod's x/net version Sep 15, 2020
@dmitshur dmitshur mentioned this issue Sep 15, 2020
Closed
@bcmills
Copy link
Contributor

bcmills commented Sep 15, 2020

#36852 is also related: ideally the TryBot should flag any CL that causes the selected version of x/net/http2 to diverge from the version from which h2_bundle.go was generated.

@dmitshur dmitshur mentioned this issue Sep 17, 2020
Open
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/255859 mentions this issue: misc/update: add program to update golang.org/x dependencies

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/256357 mentions this issue: cmd/updatestd: add program to maintain standard library modules

@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 24, 2020 •
edited
Loading

Right now, it's possible to update the bundled copy independently from the src/go.mod version, but it's not clear that flexibility is something we want to keep.

Discussion so far suggests we do not want to keep this flexibility. That means we need a test that will detect automatically when a regression happens.

I'll modify this issue to be about adding that test. The test should be skipped in -short test mode, so it will be executed on longtest builders (and via SlowBot requests).

This is similar/related to #36852 (go.mod and vendor directory being in sync) and to #36907 (go.mod of different modules being in sync).

A general approach to the test that I've discussed with @bcmills could look like this:

  1. Update x/tools in cmd/go.mod to include CL 189818 after it is submitted (/cc @FiloSottile).
  2. Use go generate -run=bundle -n all or equivalent to find packages that are bundled.
    • Currently, there are 2 packages bundled into net/http, they are golang.org/x/net/http2 and golang.org/x/net/internal/socks. But the test should catch if there new bundled packages.
  3. Use cmd/bundle as part of the test to write what the bundle should look like to a temporary directory.
  4. Detect if the bundle file in tree is different from the file generated as part of the test.

This approach can avoid needing to rely on git in the test.

@dmitshur dmitshur added the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 24, 2020
@dmitshur dmitshur changed the title net/http: h2_bundle.go (and socks_bundle.go) can get out of sync with src/go.mod's x/net version std,cmd: add test to ensure that all bundled packages are in sync with go.mod version Sep 24, 2020
@dmitshur
Copy link
Contributor Author

Managing backports can become much more expensive if skew is introduced in a new major Go release. This has happened in Go 1.15 (see #41375 (comment) and CL 258540), and I think it's important we have an automated measure so we can be confident it can't happen for future Go releases due to human error.

I'm going to make this a release-blocker for Go 1.16: we should either resolve this issue by adding a test, or if there isn't enough time, we must manually verify that there is no skew introduced in Go 1.16 release candidates and the final release.

CC @golang/release.

@dmitshur dmitshur mentioned this issue Oct 22, 2020
Closed
@dmitshur dmitshur self-assigned this Dec 5, 2020
@toothrot toothrot added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 and removed okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Dec 10, 2020
@dmitshur dmitshur mentioned this issue Dec 31, 2020
Open
gopherbot pushed a commit to golang/build that referenced this issue Jan 9, 2021
@dmitshur
cmd/updatestd: add experimental program to maintain standard library
789b929 
We need to update dependencies vendored into the standard library at
least two times during each major Go release cycle, which is tracked
by the recurring release-blocker issue golang.org/issue/36905.

Once we find agreement on the overall strategy to do this, it will be
easier, more reliable, and more scalable to do with a program than by
hand. This experimental program serves as a starting point to get there.
It has been used to generate CL 255860 and CL 266898 so far, and more
to explore the state of changes to vendored golang.org/x packages at
tip. Code review has identified opportunities for improvement, which
was filed as an issue. Upon further investigation, it was found that
those improvements are not viable to use in the short term (see
https://golang.org/issue/41589#issuecomment-712965746), but perhaps
will be worth revisiting in the future.

The scope of update performed at this time is to update golang.org/x
modules dependencies inside the standard library, re-run go generate
-run=bundle task, and update the Go language version in go.mod files.
It's expected this will evolve over time based on experience and new
findings.

For golang/go#36905.
For golang/go#41409.

Change-Id: I0e9d42675f1d8300f661545625f3b20e3704cdca
Reviewed-on: https://go-review.googlesource.com/c/build/+/256357
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@dmitshur dmitshur mentioned this issue Jan 14, 2021
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/283643 mentions this issue: cmd/internal/moddeps: check content of all modules in GOROOT

@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 16, 2021
@golang golang locked and limited conversation to collaborators Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@toothrot @dmitshur @bcmills @gopherbot and others