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

cmd/go: allow forcing tags on/off during go mod vendor, tidy #25873

Open
kardianos opened this Issue Jun 13, 2018 · 15 comments

Comments

Projects
None yet
6 participants
@kardianos
Contributor

kardianos commented Jun 13, 2018

In https://golang.org/cl/117258 the "vendor ALL" was introduced to include all files when walking the source tree, except for ignore build tags. This is great.

In https://golang.org/cl/118317 the command introduces flags for "mod -fix" and "mod -sync" (but waits for another CL to implement them). These flags use the knowledge of complete source tree (when trimming or adding requirements) to operate.

Lastly, it is important to note that the "go.mod" exclude directive only operates on the current module. This is good and to the benefit of this proposal.

This proposal adds two things:

  • Add the ability to interpret an exclude directive as a tag.
  • Use the list of excluded tags in certain operations, such as "mod -vendor", "mod -sync", and "mod -fix", that operate over the entire source tree. These commands would operate as normal in the "ALL" traversal mode, but also ignore the excluded tags as defined in the previous point.

This would be used when the entire source tree (ALL) contains irrelevant platforms to the direct user of the module (common examples would be "exclude appengine" and "exclude solaris". Because exclude directives are ignored by other modules, they do not define limits of the module's use. In addition, these exclude tags are not used when running "build" or "install".

There are two optional features of this proposal as well, but not intrinsic to the base proposal:

  • Allow excluding test as a tag: use "exclude test" to ignore dependencies in "*_test.go" files.
  • Allow exclude negation: "exclude !windows".

The implementation in govendor supports these cases here:
https://github.com/kardianos/govendor/blob/master/context/tags.go

Two ways to specify tags would be to call any "exclude" that has no "/" a tag. Another way would be to prefix the tag with a "tag:".

/cc @rsc @dlsniper @bcmills

@gopherbot gopherbot added this to the vgo milestone Jun 13, 2018

@kardianos kardianos added the Proposal label Jun 13, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 13, 2018

This would be used when the entire source tree (ALL) contains irrelevant platforms to the direct user of the module (common examples would be "exclude appengine" and "exclude solaris".

Can you give some more detail about why that's useful?

If I understand correctly, it would avoid vendoring in (some subset of) unused source code, at the cost of making a particular module unusable with the excluded tags (presumably because it's already incompatible with them to begin with?). What fraction of the vendored code is that, in the common cases you mentioned?

@kardianos

This comment has been minimized.

Contributor

kardianos commented Jun 13, 2018

at the cost of making a particular module unusable with the excluded tags (presumably because it's already incompatible with them to begin with?).

The module would still be usable with the excluded tags.

  • If you were using the module directly (main or test) that contained the exclude tags, on a platform that had those tags, "vgo build" and "vgo install" would still behave as normal. They would not be affected by this. These would only affect the specified mod sub-command, such as "mod -sync", "mod -vendor", and "mod -fix".
  • If you were using the module that contained the exclude tags from another module, you would not be affected. The exclude tags would be a way of saying "I have no opinion on these dependency versions." If the consumer module did want to use one of these tags, they can still require them, as the exclude directives for the dependencies won't be used.

Can you give some more detail about why that's useful?

There are three common cases for excluding tags when calculating the dependency tree for dependency analysis (not building):

  • Tests (unfortunately) sometimes refer to large dependency trees. When I run "go mod -vendor" I don't care about those dependencies and I don't need the copied locally.
  • OS level dependencies, such as "solaris" often require a large external syscall package or other special packages. It is rare I will ever have that as a deploy target, and more critically, I've never tested on the platform, I can't say which version of the solaris USB package to use or attest to. I also don't need it pulled in locally to the vendor folder.
  • Hosting environment and platforms, such as "appengine". If this is a windows service, that may also be run or developed on Linux, I don't care about "appengine". I don't need the (large) tree of dependencies copied in. Nor does it even make sense to support this environment for specifying versions with "mod -sync".

As you can see, this is most useful for trimming what is copied into the vendor folder. It has a secondary benefit for not adding un-tested "require" directives for un-tested platforms with "-sync".

However, it is not a blacklist in anyway. "[v]go build" will continue to operate as-is. These exclude tags are only used in the "[v]go mod" sub-command.

What fraction of the vendored code is that, in the common cases you mentioned?

Very common. Many google network libraries for instance support appengine. Many apps that use these libraries are CLI applications or other non-appengine services that will never run on appengine. Note, this doesn't preclude an appengine application from using another modules that has "exclude appengine", it just requires the consuming module specify a few more requires for the appengine specific modules.

That being said, I have not done a quantitative analysis on the impact here.

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 13, 2018

If you were using the module directly (main or test) that contained the exclude tags, on a platform that had those tags, "vgo build" and "vgo install" would still behave as normal.

Sure, but the point of vgo vendor is to work with clients that aren't using vgo at all.

What happens to users of the package with go get? (I guess they fall back to unvendored copies of the necessary packages? I'm not very familiar with the current vendoring logic.)

@kardianos

This comment has been minimized.

Contributor

kardianos commented Jun 13, 2018

vgo vendor serves one purposes (in my mind): to provide a per-project cache. Working with pre-vgo is mostly satisfied by go1.10 and go1.9 point releases.

I think go get works as normal... ? I mean, I do this today with govendor.

@bcmills

This comment has been minimized.

Member

bcmills commented Jun 13, 2018

vgo vendor serves one purposes (in my mind): to provide a per-project cache.

It's not just a cache: it's a consistent version. If you go get an unvendored module (pre-vgo), you'll build it with the latest versions of its dependencies, whereas if you go get a vgo vendor'd module, you'll build it against the versions specified in the go.mod.

(I think that's what you mean by “I have no opinion on these dependency versions,” but that's meaningfully different from caching.)

@kardianos

This comment has been minimized.

Contributor

kardianos commented Jun 14, 2018

Related: #25881

@kardianos

This comment has been minimized.

Contributor

kardianos commented Jun 21, 2018

It is also useful to exlude dependencies for older versions of go too.

@DisposaBoy

This comment has been minimized.

DisposaBoy commented Jul 10, 2018

Any updates on this?

I have what appears to be the same issue: I have a file guarded by build tags that import a package that will only exist on the user's machine i.e. the same situation as the appengine tag, except that was fixed with more special cases as-if Go isn't overflowing with hacks and special-cases already.

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 10, 2018

In general I am going to push back very hard against adding new settings to go.mod. One by one they might be reasonable but the end result would be to turn go.mod into a complex config file, when one of my favorite things about Go is that code follows conventions instead of having configs that differ from project to project. Also, providing fine-grained control over default settings often masks the defaults simply being wrong. Maybe @DisposaBoy sees the conventions as hacks and special cases, but I see them as making Go projects consistent in a way they wouldn't be if the tool were endlessly customizable.

@kardianos pointed out these reasons this fine-grained control over tags might be needed:

  1. Tests might refer to large dependency trees.
  2. Being able to say "assume go1.6 tag is always true".
  3. Not caring about certain operating systems, like the windows build tag, or everything that isn't windows, or logrus using x/sys only for solaris.
  4. Not caring about dependencies of certain hosting configs, like 'appengine' (or maybe AWS etc)
  5. Having code where dependencies are missing until built on a different machine, also like 'appengine' (also noted by @DisposaBoy).

As of CL 122256 earlier today, test data is excluded from (1) unconditionally, so that's no longer a concern. This is an example of the default being wrong and getting a chance to correct it precisely because it's not possible to override.

I have been meaning to do something about (2) as well. If you're running with Go 1.10 it seems fine to assume that the go1.8 and earlier build tags are satisfied, possibly even go1.9.

I would be interested in numbers about the amount of code (3) and (4) are causing to be copied unnecessarily. What convinced me about (1) was that the test code added a dramatic size increase to the vendor directory when trying vgo on k8s.io/kubernetes.

That leaves (5). I would like to learn more about the case @DisposaBoy has in mind to see whether it's really like appengine or not and to figure out whether there's a general answer. The special case for "appengine/*" packages is unfortunate but not directly related to the appengine build tag. It's because those import paths predate GOPATH, so the App Engine compile servers essentially added those packages to a modified standard library. Is that what you are doing too @DisposaBoy? Assuming not, how is the go command supposed to build the files on the user's machine? Or if the go command isn't ever going to build the files on the user's machine, what does consume them? Do the paths begin with domain names or plain words?

One possible way to resolve (5) generally would be to say that anything that doesn't start with a dotted name and isn't covered by existing modules in go.mod will just be ignored during mod -sync. But I don't know whether that would address @DisposaBoy's situation or not.

@DisposaBoy

This comment has been minimized.

DisposaBoy commented Jul 10, 2018

@rsc If you want, I can try and make a repo demonstrating what I'm doing tomorrow, but it's essentially a compile-time plugin system.

In simple terms: If the tag enable-plugins is defined, the app imports the package my_plugin and calls functions inside it. Otherwise (and by default), it uses a stub.

In the real code this is all automated, but the effect is that a user can go get app and it just works, but if they want to extend the app, all they have to do is create the package my_plugin and use go get|install -tags use-plugins.

The problem is that when I try to vendor the dependencies, vgo mod -vendor ignores the build tag and tries to load the package then fails with the error:

resolving import "my_plugin"
vgo: import "my_plugin_importer" ->
	import "my_plugin": unrecognized import path "my_plugin" (import path does not begin with hostname)

For reasons I prefer to use the name my_plugin instead of example.com/my_plugin or similar, but I don't think using a domain would work anyway, because it'd just try to fetch that package and fail because it doesn't exist.
If I worked around that by creating the package, then the user couldn't override it.

@rsc rsc modified the milestones: vgo, Go1.11 Jul 12, 2018

@rsc rsc added the modules label Jul 12, 2018

@rsc rsc changed the title from x/vgo: proposal, allow exclude directives to include "tag:" prefix to cmd/go: proposal, allow exclude directives to include "tag:" prefix Jul 12, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 17, 2018

It sounds like the thing I suggested (ignoring non-domain-prefixed names in go mod -sync) would address @DisposaBoy's issue. I'll look into whether that's reasonable to do.

@renannprado

This comment has been minimized.

renannprado commented Jul 18, 2018

@rsc I just came across the same idea (this feature request).
I, unfortunately, don't have much time spend here, but I'll try to sum what's the problem I've just had.

I'm using the latest vgo from the vgo repository (not the one integrated in the golang repository).

I have just added github.com/jmoiron/sqlx dependency to my project and ran vgo mod -sync.
What happened after this surprised me a little:

go: finding github.com/jmoiron/sqlx latest
go: downloading github.com/jmoiron/sqlx v0.0.0-20180614180643-0dae4fefe7c0
go: finding github.com/go-sql-driver/mysql v1.4.0
go: downloading github.com/go-sql-driver/mysql v1.4.0
go: finding github.com/lib/pq latest
go: downloading github.com/lib/pq v0.0.0-20180523175426-90697d60dd84
go: finding github.com/mattn/go-sqlite3 v1.9.0
go: downloading github.com/mattn/go-sqlite3 v1.9.0

Then I greped the package for mysql, sqlite and pq:

grep -r "mysql"
sqlx_context.go:// or the go-mysql-driver/mysql drivers;  pq seems to be an exception here.  Detecting
bind.go:	case "mysql":
sqlx_context_test.go:	_ "github.com/go-sql-driver/mysql"
sqlx_context_test.go:		runner(ctx, mysqldb, t, create, drop)
sqlx_context_test.go:	if db.DriverName() == "mysql" {
sqlx_context_test.go:		// postgres and sqlite accept "", but mysql uses ``;  since Go's multi-line
sqlx_context_test.go:			if db.DriverName() == "mysql" {
sqlx.go:// or the go-mysql-driver/mysql drivers;  pq seems to be an exception here.  Detecting
sqlx_test.go:	_ "github.com/go-sql-driver/mysql"
sqlx_test.go:var mysqldb *DB
sqlx_test.go:		mysqldb, err = Connect("mysql", mydsn)
sqlx_test.go:		runner(mysqldb, t, create, drop)
sqlx_test.go:	if db.DriverName() == "mysql" {
sqlx_test.go:		// postgres and sqlite accept "", but mysql uses ``;  since Go's multi-line
sqlx_test.go:			if db.DriverName() == "mysql" {
.travis.yml:  - mysql
.travis.yml:  - mysql -e 'CREATE DATABASE IF NOT EXISTS sqlxtest;'
grep -r "sqlite"
sqlx_context.go:// FIXME: this does not really work with multi-statement files for mattn/go-sqlite3
bind.go:	case "sqlite3":
README.md:    // database drivers;  pq will exec them all, sqlite3 won't, ymmv
sqlx_context_test.go:	_ "github.com/mattn/go-sqlite3"
sqlx_context_test.go:		// postgres and sqlite accept "", but mysql uses ``;  since Go's multi-line
sqlx.go:// FIXME: this does not really work with multi-statement files for mattn/go-sqlite3
sqlx_test.go:	_ "github.com/mattn/go-sqlite3"
sqlx_test.go:		sldb, err = Connect("sqlite3", sqdsn)
sqlx_test.go:		// postgres and sqlite accept "", but mysql uses ``;  since Go's multi-line
grep -r "pq"
sqlx_context.go:// or the go-mysql-driver/mysql drivers;  pq seems to be an exception here.  Detecting
bind.go:	case "postgres", "pgx", "pq-timeouts", "cloudsqlpostgres":
Binary file .git/objects/pack/pack-a15f57f6015d751ad71d410e8c0ce40b09d9d855.pack matches
README.md:    _ "github.com/lib/pq"
README.md:    // database drivers;  pq will exec them all, sqlite3 won't, ymmv
sqlx_context_test.go:	_ "github.com/lib/pq"
sqlx.go:// or the go-mysql-driver/mysql drivers;  pq seems to be an exception here.  Detecting
sqlx_test.go:	_ "github.com/lib/pq"

As you can see, only test files are actually importing packages mysql, sqlite and pq, but they still got added as a dependency of my current project.
All of this to say: maybe we don't want an option to exclude dependencies, but what seems reasonable is to actually have a way to exclude test dependencies.

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 10, 2018

Re test dependencies, @renannprado, see my comment on #26913.
Including them is important, and in due time it will be nearly free.

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 10, 2018

I've been thinking more about @DisposaBoy's situation described in #25873 (comment), and while I'm not completely opposed to some more heuristics here, I don't think they would solve his situation. The general problem with exceptions is that if 'go mod vendor' and 'go mod tidy' (formerly 'go mod -sync') cannot understand where the imports come from, then neither can 'go build'. Putting in an exception would not change the fact that the code is unbuildable. So putting in an exception does not really solve anything.

Concretely:

In the real code this is all automated, but the effect is that a user can go get app and it just works, but if they want to extend the app, all they have to do is create the package my_plugin and use go get|install -tags use-plugins.

There is not going to be a way to "create the package my_plugin". In the old GOPATH world you created GOPATH/src/my_plugin. That door is closed in module world: you have to do something in go.mod now.

What I would suggest is to make a separate module in its own repo, with a path you actually control (maybe github.com/you/user-plugin) that actually exists and compiles and is the no-op plugin. Then plugins could be enabled always, they would just get the no-op plugin with no effect. At this point everything builds, there are no magic dangling imports, all the commands work.

But there is still some magic in the world.

Users can clone that repo, make changes, and then run, from within the clone,

go get your/app

The go get will add your/app to the repo's go.mod and build it in that context, and in that context, the modified clone is the de facto authority for github.com/you/user-plugin. The "go get", because it is executing inside the modified clone, will build your app using the modified clone of the plugin instead of the original no-op plugin. This gives the same effect as your compile-time plugin. It even lets users manage multiple different plugins (in different clones) easily, without having to make a new GOPATH for each one.

@rsc rsc changed the title from cmd/go: proposal, allow exclude directives to include "tag:" prefix to cmd/go: allow forcing tags on/off during go mod vendor Aug 10, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 10, 2018

Rereading the reasons from #25873 (comment), it looks like the main motivations left would be (3) and (4). These only really apply to vendoring. Now that go mod vendor is its own command, we could envision adding a flag to say 'assume these tags are on or off', like maybe -tags aws,!azure to force aws on and azure off. This would be a command-line flag, not extra configuration in go.mod, for the reasons I mentioned in the earlier comment.

Leaving for decision in Go 1.12.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 10, 2018

@rsc rsc added the NeedsDecision label Aug 10, 2018

@rsc rsc changed the title from cmd/go: allow forcing tags on/off during go mod vendor to cmd/go: allow forcing tags on/off during go mod vendor, tidy Aug 18, 2018

@bcmills bcmills modified the milestones: Go1.12, Go1.13 Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment