-
Notifications
You must be signed in to change notification settings - Fork 18k
x/vgo: sync direct imports of transitive dependencies #25969
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
Comments
Coincidentally, Russ and I were discussing this yesterday (in the context of CL 119575) and came to pretty much the same conclusion. If we don't list all of the direct dependencies, then On the other hand, if we include these implied requirements, we have to be careful to keep them accurate. If module |
Agree, we should do it carefully. So you should only translate this logic to the case when |
Change https://golang.org/cl/121304 mentions this issue: |
I'm sorry @rsc, but I see https://golang.org/cl/121304 is still not merged to master. Could we please reopen this issue till it's merged? Just want to track the progress here and check it when finally done. |
A cleanup pass in mvs.BuildList discards modules that are not reachable in the requirement graph as satisfied for this build. For example, suppose: A -> B1, C1 B1 -> D1 B2 -> nothing C1 -> nothing D1 -> nothing D2 -> nothing The effective build list is A, B1, C1, D1 (no cleanup possible). Suppose that we update from B1 to B2. The effective build list becomes A, B2, C1, D1, and since there is no path through those module versions from A to D, the cleanup pass drops D. This cleanup, which is not in https://research.swtch.com/vgo-mvs, aims to avoid user confusion by not listing irrelevant modules in the output of commands like "vgo list -m all". Unfortunately, the cleanup is not sound in general, because there is no guarantee all of A's needs are listed as direct requirements. For example, maybe A imports D. In that case, dropping D and then building A will re-add the latest version of D (D2 instead of D1). The most common time this happens is after an upgrade. The fix is to make sure that go.mod does list all of the modules required directly by A, and to make sure that the go.mod minimizer (Algorithm R in the blog post) does not remove direct requirements in the name of simplifying go.mod. The way this is done is to annotate the requirements NOT used directly by A with a known comment, "// indirect". For example suppose A imports rsc.io/quote. Then the go.mod looks like it always has: module m require rsc.io/quote v1.5.2 But now suppose we upgrade our packages to their latest versions. Then go.mod becomes: module m require ( golang.org/x/text v0.3.0 // indirect rsc.io/quote v1.5.2 rsc.io/sampler v1.99.99 // indirect ) The "// indirect" comments indicate that this requirement is used only to upgrade something needed outside module m, not to satisfy any packages in module m itself. Vgo adds and removes these comments automatically. If we add a direct import of golang.org/x/text to some package in m, then the first time we build that package vgo strips the "// indirect" on the golang.org/x/text requirement line. If we then remove that package, the requirement remains listed as direct (the conservative choice) until the next "vgo mod -sync", which considers all packages in m and can mark the requirement indirect again. Algorithm R is modified to be given a set of import paths that must be preserved in the final output (all the ones not marked // indirect). Maintenance of this extra information makes the cleanup pass safe. Seeing all directly-imported modules in go.mod and distinguishing between directly- and indirectly-imported modules in go.mod are two of the most commonly-requested features, so it's extra nice that the fix for the cleanup-induced bug makes go.mod closer to what users expect. Fixes golang/go#24042. Fixes golang/go#25371. Fixes golang/go#25969. Change-Id: I4ed0729b867723fe90e836c2325f740b55b2b27b Reviewed-on: https://go-review.googlesource.com/121304 Reviewed-by: Bryan C. Mills <bcmills@google.com>
@mwf, looks like it was merged in commit |
Yeap, I'm already using it, thanks 👍 |
What version of Go are you using (
go version
)?go version go1.10.1 darwin/amd64 vgo:2018-02-20.1
Latest
vgo
:What did you do?
Please refer to https://github.com/mwf/goplay/tree/master/vgo/transitive
Consider such a flow:
main.go
with the only dependency ofgithub.com/mwf/goplay/vgo/vtest/bar
vgo mod -init
and sync itvgo mod -sync
. We get the only one dependencybar
of latest version andfoo
as a transitive, included in bar'sgo.mod
github.com/mwf/goplay/vgo/vtest/foo
directly in your app.main.go:
vgo mod -sync
What did you expect to see?
I expect
vgo mod -sync
to updatego.mod
with new requirementgithub.com/mwf/goplay/vgo/vtest/foo v0.1.0
. Because it stopped being transitive and became a direct dependency.What did you see instead?
vgo mod -sync
does nothing, keepinggithub.com/mwf/goplay/vgo/vtest/foo
as transitive.Rationale
I think we should populate go.mod file with all direct dependencies found in the code.
Otherwise there could be bad consequences for long-term project support.
For example, we remove
bar
usage from code in some future release, onlyfoo
is left. We would definitely want to drop bar fromgo.mod
- and we delete it withvgo mod -droprequire=github.com/mwf/goplay/vgo/vtest/bar
or manually. Runningvgo mod -sync
then resolvesfoo
to the latest version, which is not we probably want.There is a way to handle it now - instead of
droprequire
or manual go.mod modification runvgo mod -sync
an it keeps the transitive version used previously. But better to support all the cases.Summarising, it's better to have all direct dependencies explicitly defined in go.mod instead of keeping in mind some correct way of dependency synchronization.
The text was updated successfully, but these errors were encountered: