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

Jump to bottom

cmd/go: workspace-mode graph pruning needs to follow upgraded roots #49763

Closed
bcmills opened this issue Nov 23, 2021 · 5 comments
Closed

cmd/go: workspace-mode graph pruning needs to follow upgraded roots #49763

bcmills opened this issue Nov 23, 2021 · 5 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.18

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 23, 2021 •
edited

In CL 362754 we added a workspace graph-pruning mode, to fix a bug that we identified through code review (added in the work_prune test script in that CL).

I've been thinking some more about that scenario, and I've identified a related problem arising from the interaction between workspaces and graph pruning.

The module graph in that example has edges like:

example.com/a -> example.com/b v1.0.0 -> example.com/q v1.1.0
example.com/p -> example.com/q v1.0.0

But what if example.com/q itself has edges to new, not-previously-imported dependencies?

example.com/q v1.1.0 -> example.com/r v1.0.0

With current workspace pruning semantics, that edge will be pruned out: it is pruned out of a's dependency graph (because it is not relevant to any package imported by a), and it is not present in b's dependency graph (because b requires a different version of q entirely).

The result is that a by itself may be tidy, and p by itself may be tidy, but loading the dependencies of p from the combined module graph is missing a dependency.
@bcmills
Copy link
Contributor Author

bcmills commented Nov 23, 2021 •
edited

One possible fix is to change modload.updateWorkspaceRoots do do essentially the same iteration as the upgrade loop in modload.updatePrunedRoots.

However, that loop has a pretty unfortunate running time: any time a dependency is upgraded it reloads the full graph to recompute MVS results, and upgrades can potentially form long spirals. In a single-module setting, that's not terrible because we can record the result in the go.mod file for next time, but in workspace mode we don't have any obvious place to record it.

@bcmills bcmills closed this as completed Nov 23, 2021
@bcmills bcmills changed the title cmd/go: workspace mode cmd/go: workspace-mode graph pruning needs to follow upgraded roots Nov 23, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Nov 23, 2021

Another possible fix is to change the workspace pruning mode to do the expansion itself: after each round of loading from the roots, re-enqueue the selected version of each root if that version's dependencies have not yet been loaded.
(That would take the form of an extra condition and perhaps another layer of looping in modload.readModGraph.)

If we end up encountering a spiral of upgrades, that doesn't allow us to prune out as many intermediate versions, but it does allow us to avoid reloading the graph multiple times — and upgrade spirals seem like they will be fairly short in practice and are relatively easy for the user to cut off by running go work sync.

CC @matloob

@bcmills bcmills reopened this Nov 23, 2021
@bcmills bcmills added modules NeedsFix The path to resolution is known, but the work has not been done. labels Nov 23, 2021
@bcmills bcmills added this to the Go1.18 milestone Nov 23, 2021
@bcmills bcmills added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels Nov 23, 2021
@cherrymui cherrymui removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Dec 14, 2021
@cherrymui
Copy link
Member

Any updates on this? Thanks. (checking in as this is a release blocker.)

@gopherbot
Copy link

Change https://golang.org/cl/370663 mentions this issue: cmd/go: examine dependencies of main modules in workspace mode

@bcmills
Copy link
Contributor Author

bcmills commented Dec 16, 2021

We pair-programmed this today (in CL 370663), and if it passes in the longtest builders it should resolve this issue for the release.

@golang golang locked and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@bcmills @gopherbot @cherrymui