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

x/build: update x/ dependencies in untagged x/ repos when applying tags #56530

Closed
bcmills opened this issue Nov 2, 2022 · 13 comments
Closed

x/build: update x/ dependencies in untagged x/ repos when applying tags #56530

bcmills opened this issue Nov 2, 2022 · 13 comments
Assignees
@dmitshur
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 2, 2022

Now that we are automatically tagging releases for many of the golang.org/x/... repos (#48523), we are incidentally updating the other x-repo dependencies in their go.mod files. However, some of our x repos that are not currently tagged (notably x/debug) also depend on the tagged x repos, and those currently require manual updates (as in https://go.dev/cl/446515).

When we add a new tag for an x repo, it would be nice to also update the untagged x repos that depend on it to prevent our internal dependencies from getting too stale.

(As @dmitshur pointed out to me, x dependencies are safe to upgrade because they have necessarily gone through Go's code review process, whereas upgrading non-x dependencies may require auditing upstream changes.)
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Nov 2, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 2, 2022
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Builders x/build issues (builders, bots, dashboards) labels Nov 2, 2022
@heschi heschi moved this to Planned in Go Release Nov 8, 2022
@FiloSottile
Copy link
Contributor

I found this issue because I wanted to ask why are we blindly updating x/ module inter-dependencies. I'm not sure consumers (us included) should be forced to update every (or, rather, an arbitrary subset) x/ repo every time they update one, if there is no actually related change.

For example, v0.3.0 of x/crypto depends on v0.2.0 of x/net because of the automatic upgrades, but the standard library is on v0.1.0. CL 353849 needs to bring in a recent x/crypto change. Without that upgrade, I could have just pulled x/crypto up in CL 353849. Instead, I now have to make a separate CL that also imports unrelated http2 changes in x/net.

@dmitshur
Copy link
Contributor

@FiloSottile This question is a better fit for issue #36905, if I understand it correctly. With the 1.20 freeze coming up next week, we will be updating the std and cmd modules to vendor the latest tip versions of all golang.org/x modules, which will include x/net's v0.2.0 and 4 commits since.

@FiloSottile
Copy link
Contributor

@dmitshur I don't think it's related to #36905: first, the stdlib case is just an example of why a consumer might not want to pull in unrelated updates when updating a x/ repo. Non-stdlib modules might also want to selectively update. Second, if the std bump comes at the start of the freeze, it won't help me with CL 353849 unless we make a freeze exception, because CL 353849 depends on the x/crypto bump.

This issue is about automatically updating x/ dependencies in untagged x/ repos. What I'm saying is that I don't understand why we are automatically updating x/ dependencies in any x/ repos at all.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 17, 2022 •
edited
Loading

In general it isn't feasible to test every x repo commit against every commit to each of its dependencies, so there will always be some amount of “untested skew” between the repos: that is, an accidental breaking change (or accidental dependency on an implementation detail) may break a package or test in another repo without causing the build for that repo to immediately fail.

Keeping the dependencies within our repos reasonably up-to-date limits that skew to a smaller interval of time and a smaller of interval of changes, making it more likely that the incompatibility will be noticed (and fixed) quickly and also reducing the number of commits that may need to be bisected in order to diagnose a regression.

(Note that we can update the x repos automatically because they all have the same code-review requirements. We cannot apply the same policy to automatically update third-party dependencies, because we may need to audit the changes in those dependencies before we can safely use them.)

@bcmills
Copy link
Contributor Author

bcmills commented Dec 8, 2022

As a data point, #57147 reported x/vuln tests that have been failing since early November on the android builders. The test failures appear to be fixed by upgrading the x/tools dependency in that module to the latest tagged release (done in CL 456122).

It would be nice if those kinds of fixes could happen automatically.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 9, 2022

Another data point (#57217): x/website is currently broken on freebsd/riscv64 due to a very out-of-date x/sys.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 9, 2022

...and, here is exactly the case in point.

If we were updating the x/website dependencies promptly upon tagging x/sys releases, this problem would have been discovered earlier.

@seankhliao seankhliao mentioned this issue Feb 26, 2023
Closed
@bcmills
Copy link
Contributor Author

bcmills commented Mar 31, 2023 •
edited
Loading

Another data point: CL 480936 updated the x/build dependency on x/net to get past vulnerability GO-2023-1571 (#57855), which would have been fixed automatically if the dependency had been upgraded when x/net was tagged.

@dmitshur dmitshur added the Builders x/build issues (builders, bots, dashboards) label Apr 21, 2023
@dmitshur dmitshur self-assigned this Apr 21, 2023
@dmitshur dmitshur moved this from Planned to In Progress in Go Release Apr 21, 2023
@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 21, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/487615 mentions this issue: internal/task: update golang.org/x dependencies in untagged repos

@bcmills bcmills mentioned this issue May 2, 2023
Closed
@dmitshur dmitshur moved this from In Progress to Planned in Go Release May 9, 2023
@dmitshur
Copy link
Contributor

One more data point: x/pkgsite is untagged so its x/mod dependency wasn't automatically updated. If this issue were resolved, #62031 might've been resolved sooner or avoided.

@dmitshur dmitshur moved this from Planned to In Progress in Go Release Aug 22, 2023
@dmitshur dmitshur mentioned this issue Aug 22, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/523155 mentions this issue: internal/tag: improve how fakes report non-existing file and no tags

gopherbot pushed a commit to golang/build that referenced this issue Aug 29, 2023
@dmitshur @gopherbot
internal/task: improve how fakes report nonexistent file and no tags
88aa460 
The TagXReposTasks.readRepo method calls ReadBranchHead and ReadFile,
and handles errors matching gerrit.ErrResourceNotExist that the real
Gerrit implementation returns. Improve the fakes accordingly, enough
to make the upcoming changes to the tagging workflow its tests happy.

For golang/go#56530.

Change-Id: I4d92d578210b20752e65bdc2719b74bc5c7259ff
Reviewed-on: https://go-review.googlesource.com/c/build/+/523155
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Release Sep 1, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/525655 mentions this issue: internal/task: opt x/vuln in for updates only

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/525657 mentions this issue: internal/task: tidy x/exp and x/telemetry nested modules too

gopherbot pushed a commit to golang/build that referenced this issue Sep 5, 2023
@dmitshur
internal/task: tidy x/exp and x/telemetry nested modules too
7f65e2b 
Nested modules with replace directives need to be tidied too,
else they end up in an inconsistent state and their builds fail.

Though it's tempting to automate the discovery of such modules,
it would take some code and these situations are fairly rare.
So for now add the missing ones to the hard-coded list manually;
we can change our minds about it later if we want.

For golang/go#56530.

Change-Id: Ic622d3f0080abc17410060c77978a3917f0be0b7
Reviewed-on: https://go-review.googlesource.com/c/build/+/525657
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@dmitshur dmitshur mentioned this issue Oct 10, 2023
Closed
gopherbot pushed a commit to golang/build that referenced this issue Nov 22, 2023
@dmitshur @gopherbot
internal/task: opt x/vuln in for updates only
d83deb5 
x/vuln opted out of automatic tagging, but there's no reason for it not
to receive automatic updates. Also see a motivating data point captured
in https://go.dev/issue/56530#issuecomment-1342988547.

Now that there's a more fine-grained method of controlling whether to
tag a repo, move the decision there and stop ignoring the entire vuln
repo.

For golang/go#59686.
For golang/go#56530.

Change-Id: I32815b3d52d7bd2e601b4eae0290008b33eefbec
Reviewed-on: https://go-review.googlesource.com/c/build/+/525655
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@golang golang locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dmitshur dmitshur

Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release
Archived in project
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@FiloSottile @dmitshur @bcmills @gopherbot