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/vgo: sync direct imports of transitive dependencies #25969

Closed
mwf opened this issue Jun 20, 2018 · 7 comments
Closed

x/vgo: sync direct imports of transitive dependencies #25969

mwf opened this issue Jun 20, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mwf
Copy link

mwf commented Jun 20, 2018

What version of Go are you using (go version)?

go version go1.10.1 darwin/amd64 vgo:2018-02-20.1

Latest vgo:

commit f574d316627652354b92fbdf885a2b2f7ea7dac9 (HEAD -> master, origin/master, origin/HEAD)
Author: Bryan C. Mills <bcmills@google.com>
Date:   Fri Jun 15 16:41:09 2018 -0400

    cmd/go: skip vgo.Init for the version subcommand

What did you do?

Please refer to https://github.com/mwf/goplay/tree/master/vgo/transitive

Consider such a flow:

  1. Project is empty, you create main.go with the only dependency of github.com/mwf/goplay/vgo/vtest/bar
package main

import (
	"github.com/mwf/goplay/vgo/vtest/bar"
)

func main() {
	println("bar", bar.Bar)
	println("foobar", bar.FooBar)
}
  1. Create a go.mod vgo mod -init and sync it vgo mod -sync. We get the only one dependency bar of latest version and foo as a transitive, included in bar's go.mod
$ cat go.mod
module github.com/mwf/goplay/vgo/transitive

require github.com/mwf/goplay/vgo/vtest/bar v0.1.0
$ vgo list -m
MODULE                                VERSION
github.com/mwf/goplay/vgo/transitive  -
github.com/mwf/goplay/vgo/vtest/bar   v0.1.0
github.com/mwf/goplay/vgo/vtest/foo   v0.1.0
  1. Some time passes, and you need to import the github.com/mwf/goplay/vgo/vtest/foo directly in your app.
    main.go:
package main

import (
	"github.com/mwf/goplay/vgo/vtest/bar"
	"github.com/mwf/goplay/vgo/vtest/foo"
)

func main() {
	println("bar", bar.Bar)
	println("foobar", bar.FooBar)
	println("foo", foo.Foo)
}
  1. And you want to sync the dependencies with vgo mod -sync

What did you expect to see?

I expect vgo mod -sync to update go.mod with new requirement github.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, keeping github.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, only foo is left. We would definitely want to drop bar from go.mod - and we delete it with vgo mod -droprequire=github.com/mwf/goplay/vgo/vtest/bar or manually. Running vgo mod -sync then resolves foo 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 run vgo 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.

@gopherbot gopherbot added this to the vgo milestone Jun 20, 2018
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 20, 2018
@bcmills
Copy link
Contributor

bcmills commented Jun 20, 2018

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.

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 mvs.UpgradeAll can end up dropping dependencies that we have to add back in, and go get -u becomes much more complicated (and probably iterative).

On the other hand, if we include these implied requirements, we have to be careful to keep them accurate. If module A imports C and requires module B@v1.1.0, and B@v1.1.0 requires C@v1.1.0, then module A must require C@v1.1.0 or higher: C@v1.0.0 would not satisfy the requirements of B@v1.1.0, so listing C@v1.0.0 as a requirement would be misleading.

@mwf
Copy link
Author

mwf commented Jun 20, 2018

On the other hand, if we include these implied requirements, we have to be careful to keep them accurate. If module A imports C and requires module B@v1.1.0, and B@v1.1.0 requires C@v1.1.0, then module A must require C@v1.1.0 or higher: C@v1.0.0 would not satisfy the requirements of B@v1.1.0, so listing C@v1.0.0 as a requirement would be misleading.

Agree, we should do it carefully.
But actually it should be easy enough, because as I said vgo mod -sync already does what we need - it adds an already resolved transitive dependency (max of mins) if you drop calls to B from your code leaving require B v1.1.0 in go.mod. - it will change to require C v1.1.0

So you should only translate this logic to the case when B is still used along with C :)

@rsc
Copy link
Contributor

rsc commented Jun 27, 2018

As @bcmills said, we realized that having direct imports is actually important,
and I now have a CL on the way to do exactly this. As @mwf said, it's fairly easy.

Still some cleanup before I send it out, but I wanted to update the issue to avoid any duplicated effort.

@rsc rsc self-assigned this Jun 27, 2018
@rsc rsc 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 Jun 27, 2018
@gopherbot
Copy link

Change https://golang.org/cl/121304 mentions this issue: cmd/go/internal/vgo: track directly-used vs indirectly-used modules

@mwf
Copy link
Author

mwf commented Jul 5, 2018

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.
Thanks in advance :)

gopherbot pushed a commit to golang/vgo that referenced this issue Jul 10, 2018
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>
@bcmills
Copy link
Contributor

bcmills commented Jul 11, 2018

@mwf, looks like it was merged in commit 552f8db34257fd0182aa02302bc29db1aeef7370.

@mwf
Copy link
Author

mwf commented Jul 11, 2018

Yeap, I'm already using it, thanks 👍

@golang golang locked and limited conversation to collaborators Jul 11, 2019
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants