cmd/go: exclude vendor directories from wildcard matches? #11659

Closed
freeformz opened this Issue Jul 10, 2015 · 93 comments

Comments

Projects
None yet
@freeformz
Contributor

freeformz commented Jul 10, 2015

  1. What version of Go are you using (go version)? What operating system and processor architecture are you using?

go version go1.5beta1 darwin/amd64

  1. What did you do?

using this repo/branch as a test of vendor/: https://github.com/heroku/log-shuttle/tree/vendor2

GO15VENDOREXPERIMENT=1 go install -v ./...

What did you expect to see?

GO15VENDOREXPERIMENT=1 go install -v ./...
github.com/heroku/log-shuttle/vendor/github.com/pebbe/util
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics
github.com/heroku/log-shuttle/vendor/github.com/heroku/slog
github.com/heroku/log-shuttle/vendor/github.com/bmizerany/aws4
github.com/heroku/log-shuttle/vendor/github.com/nu7hatch/gouuid
github.com/heroku/log-shuttle
github.com/heroku/log-shuttle/cmd/log-shuttle

What did you see instead?

GO15VENDOREXPERIMENT=1 go install -v ./...
github.com/heroku/log-shuttle/vendor/github.com/nu7hatch/gouuid
github.com/heroku/log-shuttle/vendor/github.com/bmizerany/aws4
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics
github.com/heroku/log-shuttle/vendor/github.com/heroku/slog
github.com/heroku/log-shuttle/vendor/github.com/pebbe/util
github.com/influxdb/influxdb/influxql
github.com/heroku/log-shuttle/vendor/github.com/bmizerany/aws4/dydb
github.com/boltdb/bolt
github.com/gogo/protobuf/proto
github.com/heroku/log-shuttle/vendor/github.com/pebbe/util/isatty
github.com/heroku/log-shuttle
github.com/heroku/log-shuttle/cmd/log-shuttle
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/cmd/metrics-bench
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/cmd/metrics-example
github.com/armon/go-metrics
github.com/hashicorp/go-msgpack/codec
github.com/influxdb/influxdb/meta/internal
github.com/influxdb/influxdb/toml
golang.org/x/crypto/blowfish
github.com/influxdb/influxdb/snapshot
golang.org/x/crypto/bcrypt
github.com/golang/protobuf/proto
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/librato
github.com/hashicorp/raft
github.com/stathat/go
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/stathat
github.com/influxdb/influxdb/tsdb/internal
github.com/hashicorp/raft-boltdb
github.com/influxdb/influxdb/meta
github.com/influxdb/influxdb/tsdb
github.com/influxdb/influxdb/client
github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/influxdb
# github.com/heroku/log-shuttle/vendor/github.com/rcrowley/go-metrics/influxdb
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:19: undefined: client.ClientConfig
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:38: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:44: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:52: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:60: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:70: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:82: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:93: undefined: client.Series
vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go:106: client.WriteSeries undefined (type *client.Client has no field or method WriteSeries)

I expected go to skip un-needed stuff (non imported packages) in the vendor/ directory. Perhaps this is a problem with my expectations, but it was a surprise.

@ianlancetaylor ianlancetaylor added this to the Go1.5 milestone Jul 10, 2015

@ianlancetaylor ianlancetaylor changed the title from go install ./... attempts install of stuff in vendor/ when GO15VENDOREXPERIMENT=1 is used to cmd/go: go install ./... attempts install of stuff in vendor/ when GO15VENDOREXPERIMENT=1 is used Jul 11, 2015

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 14, 2015

Contributor

What package imports github.com/influxdb/influxdb/influxql ?

Contributor

adg commented Jul 14, 2015

What package imports github.com/influxdb/influxdb/influxql ?

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Jul 16, 2015

Contributor

@adg it's pulled in from vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go, which is not imported by the application. Technically I guess I could remove the file from vendor/, but I still think the behavior of ./... should not consider packages in vendor/ unless they are transitively imported by packages outside.

Contributor

freeformz commented Jul 16, 2015

@adg it's pulled in from vendor/github.com/rcrowley/go-metrics/influxdb/influxdb.go, which is not imported by the application. Technically I guess I could remove the file from vendor/, but I still think the behavior of ./... should not consider packages in vendor/ unless they are transitively imported by packages outside.

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 16, 2015

Contributor

That would require special-casing vendor in package pattern matching, which is something we decided not to do (at this point anyway).

Contributor

adg commented Jul 16, 2015

That would require special-casing vendor in package pattern matching, which is something we decided not to do (at this point anyway).

@adg adg changed the title from cmd/go: go install ./... attempts install of stuff in vendor/ when GO15VENDOREXPERIMENT=1 is used to cmd/go: exclude vendor directories from wildcard matches? Jul 16, 2015

@technosophos

This comment has been minimized.

Show comment
Hide comment
@technosophos

technosophos Jul 17, 2015

When I run go test ./..., it runs the tests for all of the packages in vendor/ as well as my project. Are you suggesting that this is the desired behavior (for now), @adg?

When I run go test ./..., it runs the tests for all of the packages in vendor/ as well as my project. Are you suggesting that this is the desired behavior (for now), @adg?

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Jul 17, 2015

Contributor

@technosophos +1 on that problem.

@adg IMO that makes the vendor experiment nearly useless (for me anyway). I'll either need to make godep much more intelligent wrt package handling and/or try to discern what to build through additional package introspection. It's also just a bad user experience IMO.

Contributor

freeformz commented Jul 17, 2015

@technosophos +1 on that problem.

@adg IMO that makes the vendor experiment nearly useless (for me anyway). I'll either need to make godep much more intelligent wrt package handling and/or try to discern what to build through additional package introspection. It's also just a bad user experience IMO.

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 17, 2015

Contributor

@technosophos yes, that is the correct behavior as of now.

@freeformz there is an easy work around. Instead of using ./... you can do:

go install $(go list ./... | grep -v /vendor/)

or use go test if you want to run tests.

Contributor

adg commented Jul 17, 2015

@technosophos yes, that is the correct behavior as of now.

@freeformz there is an easy work around. Instead of using ./... you can do:

go install $(go list ./... | grep -v /vendor/)

or use go test if you want to run tests.

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 17, 2015

Contributor

On the testing front, I think it is valuable to know that your dependencies' tests pass.

Contributor

adg commented Jul 17, 2015

On the testing front, I think it is valuable to know that your dependencies' tests pass.

@technosophos

This comment has been minimized.

Show comment
Hide comment
@technosophos

technosophos Jul 17, 2015

Yeah, I don't find any of this particularly disconcerting. I just wanted to verify that it was the intended behavior.

Yeah, I don't find any of this particularly disconcerting. I just wanted to verify that it was the intended behavior.

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Jul 17, 2015

Contributor

@adg Heh, thanks. I should have thought of that. :-(

Are there any plans to special case vendor/ at all, say in the 1.6 timeframe?

Contributor

freeformz commented Jul 17, 2015

@adg Heh, thanks. I should have thought of that. :-(

Are there any plans to special case vendor/ at all, say in the 1.6 timeframe?

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jul 17, 2015

Contributor

Possibly. That's what this issue is now tracking. We're going to gather people's experience with the 1.5 vendoring experiment before making such plans.

Contributor

adg commented Jul 17, 2015

Possibly. That's what this issue is now tracking. We're going to gather people's experience with the 1.5 vendoring experiment before making such plans.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Jul 17, 2015

Contributor

@freeformz After looking at this issue carefully, I would say the current behavior would be my desired behavior. While I understand Godep copies over the entire repository to vendor, I don't think that is what I would encourage. I think copying over each package as needed by the vendor tool is where the analysis work needs to be. At least that is what vendor will do.

Contributor

kardianos commented Jul 17, 2015

@freeformz After looking at this issue carefully, I would say the current behavior would be my desired behavior. While I understand Godep copies over the entire repository to vendor, I don't think that is what I would encourage. I think copying over each package as needed by the vendor tool is where the analysis work needs to be. At least that is what vendor will do.

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Jul 17, 2015

Contributor

@kardianos The issue really has nothing to do with Godep, even though Godep's current behavior triggers it.

Contributor

freeformz commented Jul 17, 2015

@kardianos The issue really has nothing to do with Godep, even though Godep's current behavior triggers it.

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Jul 18, 2015

Contributor

A couple reasons I would prefer go test ./... not include things in vendor:

When I choose to take on a dependency, I've already verified it serves my need. That includes verifying automated tests are run by that dependency's overall project, running them myself, or similar. With things in vendor likely changing infrequently relative to my project it doesn't feel right to run the same tests for unchanged things every time. This is the same logic I use for feeling comfortable using, say, net/http in my project without running its tests every time.

Second, It's possible a dependency's tests will never build, work, or be relevant in my particular environment. This could be due to the way the tests are written, differences in developing vs deploying platform, etc. I develop projects on my OS X laptop that use libcontainer that would probably be good examples for this.

Contributor

danp commented Jul 18, 2015

A couple reasons I would prefer go test ./... not include things in vendor:

When I choose to take on a dependency, I've already verified it serves my need. That includes verifying automated tests are run by that dependency's overall project, running them myself, or similar. With things in vendor likely changing infrequently relative to my project it doesn't feel right to run the same tests for unchanged things every time. This is the same logic I use for feeling comfortable using, say, net/http in my project without running its tests every time.

Second, It's possible a dependency's tests will never build, work, or be relevant in my particular environment. This could be due to the way the tests are written, differences in developing vs deploying platform, etc. I develop projects on my OS X laptop that use libcontainer that would probably be good examples for this.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Jul 18, 2015

Contributor

@dpiddy Tests that are specific to an environment shouldn't run on a different environment anyway. If they try to run file a bug against the package. I would also argue when copying packages over the vendor tool user should decide if they want to bring over the test files or not

The case where my reasoning falls down is where people choose to use a vcs features like sub-modules.

(Not directly trying to argue here, just state other sides.)

Contributor

kardianos commented Jul 18, 2015

@dpiddy Tests that are specific to an environment shouldn't run on a different environment anyway. If they try to run file a bug against the package. I would also argue when copying packages over the vendor tool user should decide if they want to bring over the test files or not

The case where my reasoning falls down is where people choose to use a vcs features like sub-modules.

(Not directly trying to argue here, just state other sides.)

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Jul 20, 2015

Contributor

@kardianos good point, tools could do that.

Re the original issue of go install ./... installing things from vendor, that sounds really confusing and surprising. "Why did $GOPATH/bin/x change unexpectedly? Oh, I ran go install ./... in some completely unrelated project that vendors an old version of x three dependencies deep."

Vendoring tools could also skip main packages I suppose.

Contributor

danp commented Jul 20, 2015

@kardianos good point, tools could do that.

Re the original issue of go install ./... installing things from vendor, that sounds really confusing and surprising. "Why did $GOPATH/bin/x change unexpectedly? Oh, I ran go install ./... in some completely unrelated project that vendors an old version of x three dependencies deep."

Vendoring tools could also skip main packages I suppose.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Jul 20, 2015

Contributor

@dpiddy Of note, I've had at least one person explicitly want to vendor "main" packages. What the kardianos/vendor tool does it to categorize packages by a "status":

Missing (packages referenced but not found)
Standard (in standard library)
Local (package is under "project" and not rewritten or in vendor dir)
External (package is outside of "project" but referenced)
Internal (package is rewritten under "internal" but still a vendor package)
Unused (package is rewritten under "internal" or in vendor folder but unused)
Program (package is a "main" package but rewritten or in vendor folder)
Vendor (package is in vendor folder)

These are two overlapping issues I hope to address soon (location and status), but that's the general idea. Then my tool can work on package status rather then just paths. For instance, if vendor added an install command, you could run "vendor install -status local".

I'm not convinced that adding a "status" concept to the go tool would ever be a desirable thing. "go" already supports "std". The syntax "go install ./...!local" could be interesting I think.

Contributor

kardianos commented Jul 20, 2015

@dpiddy Of note, I've had at least one person explicitly want to vendor "main" packages. What the kardianos/vendor tool does it to categorize packages by a "status":

Missing (packages referenced but not found)
Standard (in standard library)
Local (package is under "project" and not rewritten or in vendor dir)
External (package is outside of "project" but referenced)
Internal (package is rewritten under "internal" but still a vendor package)
Unused (package is rewritten under "internal" or in vendor folder but unused)
Program (package is a "main" package but rewritten or in vendor folder)
Vendor (package is in vendor folder)

These are two overlapping issues I hope to address soon (location and status), but that's the general idea. Then my tool can work on package status rather then just paths. For instance, if vendor added an install command, you could run "vendor install -status local".

I'm not convinced that adding a "status" concept to the go tool would ever be a desirable thing. "go" already supports "std". The syntax "go install ./...!local" could be interesting I think.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jul 21, 2015

Contributor

This has come up repeatedly, but it seems like a bad idea.
Wildcards are wildcards.
Vendoring is vendoring.
They're completely separate things.
One should not influence the other.
If there is junk in a vendor/ subtree,
I think that's an argument for deleting, not for ignoring.

Contributor

rsc commented Jul 21, 2015

This has come up repeatedly, but it seems like a bad idea.
Wildcards are wildcards.
Vendoring is vendoring.
They're completely separate things.
One should not influence the other.
If there is junk in a vendor/ subtree,
I think that's an argument for deleting, not for ignoring.

@rsc rsc closed this Jul 21, 2015

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jul 21, 2015

Contributor

I missed Andrew's comment earlier about using this issue to track feedback. That's fine, but if so it should not be marked as milestone Go 1.5. Reopening and reclassifying.

Contributor

rsc commented Jul 21, 2015

I missed Andrew's comment earlier about using this issue to track feedback. That's fine, but if so it should not be marked as milestone Go 1.5. Reopening and reclassifying.

@rsc rsc reopened this Jul 21, 2015

@rsc rsc modified the milestones: Go1.6, Go1.5 Jul 21, 2015

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Jul 21, 2015

Contributor

@rsc Thanks for re-opening. I hope other's provide additional feedback ... My general thoughts on your last comments are: I "technically" agree with you, but it makes for a poor user experience.

Contributor

freeformz commented Jul 21, 2015

@rsc Thanks for re-opening. I hope other's provide additional feedback ... My general thoughts on your last comments are: I "technically" agree with you, but it makes for a poor user experience.

@jacobsa

This comment has been minimized.

Show comment
Hide comment
@jacobsa

jacobsa Jul 23, 2015

Contributor

Having go test ./... run dependencies' tests seems attractive to me in theory, but I just tried it in practice and it was a pain in the ass. Mostly because it means you need to pull in the transitive closure of their dependencies, which is large and which godep doesn't seem to support well.

Contributor

jacobsa commented Jul 23, 2015

Having go test ./... run dependencies' tests seems attractive to me in theory, but I just tried it in practice and it was a pain in the ass. Mostly because it means you need to pull in the transitive closure of their dependencies, which is large and which godep doesn't seem to support well.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Jul 23, 2015

Contributor

@jacobsa Sure, but don't confuse the issue of "go test ./..." with a tool, godep. There (will be) many tools. There will be some that analyze dependencies well.

Contributor

kardianos commented Jul 23, 2015

@jacobsa Sure, but don't confuse the issue of "go test ./..." with a tool, godep. There (will be) many tools. There will be some that analyze dependencies well.

@jacobsa

This comment has been minimized.

Show comment
Hide comment
@jacobsa

jacobsa Jul 26, 2015

Contributor

You're right, sorry. In theory the current tool support isn't so important, but nevertheless it still is a pragmatic issue today (since as far as I can tell godep is the only tool that comes close to supporting the expected format today).

Contributor

jacobsa commented Jul 26, 2015

You're right, sorry. In theory the current tool support isn't so important, but nevertheless it still is a pragmatic issue today (since as far as I can tell godep is the only tool that comes close to supporting the expected format today).

@itsjamie

This comment has been minimized.

Show comment
Hide comment
@itsjamie

itsjamie Jul 28, 2015

Contributor

My expected behavior from other languages would be that all wildcard matching would by default apply, but the user should have the ability to easily exclude a folder in a CLI flag or dot file, like gitignore.

git add . takes into account what I've ignored even though it essentially wildcards the whole subtree.

go test ./... --ignore ./vendor wouldn't include packages in vendor.

If not done in the Go tool itself, try to make the "right" values part of the public API so it can be easily extended.

My 2c, Thanks!

Contributor

itsjamie commented Jul 28, 2015

My expected behavior from other languages would be that all wildcard matching would by default apply, but the user should have the ability to easily exclude a folder in a CLI flag or dot file, like gitignore.

git add . takes into account what I've ignored even though it essentially wildcards the whole subtree.

go test ./... --ignore ./vendor wouldn't include packages in vendor.

If not done in the Go tool itself, try to make the "right" values part of the public API so it can be easily extended.

My 2c, Thanks!

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Jul 28, 2015

Contributor

I'd like to reopen the discussion on naming the vendor folder, _vendor as
many tools already ignore this pattern.
On 28 Jul 2015 8:45 pm, "Jamie Stackhouse" notifications@github.com wrote:

My expected behavior from other languages would be that all wildcard
matching would by default apply, but the user should have the ability to
easily exclude a folder in a CLI flag or dot file, like gitignore.

git add . takes into account what I've ignored even though it essentially
wildcards the whole subtree.

go test ./... --ignore ./vendor wouldn't include packages in vendor.

If not done in the Go tool itself, try to make the "right" values part of
the public API so it can be easily extended.

My 2c, Thanks!


Reply to this email directly or view it on GitHub
#11659 (comment).

Contributor

davecheney commented Jul 28, 2015

I'd like to reopen the discussion on naming the vendor folder, _vendor as
many tools already ignore this pattern.
On 28 Jul 2015 8:45 pm, "Jamie Stackhouse" notifications@github.com wrote:

My expected behavior from other languages would be that all wildcard
matching would by default apply, but the user should have the ability to
easily exclude a folder in a CLI flag or dot file, like gitignore.

git add . takes into account what I've ignored even though it essentially
wildcards the whole subtree.

go test ./... --ignore ./vendor wouldn't include packages in vendor.

If not done in the Go tool itself, try to make the "right" values part of
the public API so it can be easily extended.

My 2c, Thanks!


Reply to this email directly or view it on GitHub
#11659 (comment).

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Aug 2, 2015

Contributor

@jacobsa Please look into github.com/kardianos/govendor . It now supports the vendor folder as well. It also flattens any nested dependencies and only picks packages that you use.

Contributor

kardianos commented Aug 2, 2015

@jacobsa Please look into github.com/kardianos/govendor . It now supports the vendor folder as well. It also flattens any nested dependencies and only picks packages that you use.

@jacobsa

This comment has been minimized.

Show comment
Hide comment
@jacobsa

jacobsa Aug 3, 2015

Contributor

@kardianos: Thanks, I quite like the tool. It also imports tests that I don't want; I'll file a bug.

Contributor

jacobsa commented Aug 3, 2015

@kardianos: Thanks, I quite like the tool. It also imports tests that I don't want; I'll file a bug.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Aug 15, 2015

Contributor

github.com/kardianos/govendor can now ignore test files and arbitrary build tags. If you don't want to run tests, then we don't vendor them.

Contributor

kardianos commented Aug 15, 2015

github.com/kardianos/govendor can now ignore test files and arbitrary build tags. If you don't want to run tests, then we don't vendor them.

@pkieltyka

This comment has been minimized.

Show comment
Hide comment
@pkieltyka

pkieltyka Aug 21, 2015

btw.. for those impatient, this helps run tests and excluding vendor/..

GO15VENDOREXPERIMENT=1 go test $(GO15VENDOREXPERIMENT=1 go list ./... | grep -v '/vendor/')

go list does adhere to the env and excludes the vendor/ directory.. pretty inconsistent / busted implementation :/

btw.. for those impatient, this helps run tests and excluding vendor/..

GO15VENDOREXPERIMENT=1 go test $(GO15VENDOREXPERIMENT=1 go list ./... | grep -v '/vendor/')

go list does adhere to the env and excludes the vendor/ directory.. pretty inconsistent / busted implementation :/

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Aug 26, 2015

Contributor

One other thing to keep in mind. As much as we want everyone to use Go 1.5
immediately, experience has shown that there are many go programs out there
being built with older versions on a if it ain't broke, don't touch it,
basis.

For those older versions, and more importantly library writers, who want to
adopt the versioning experiment in their code, using _vendor would allow
those library writers to try this feature without being overwhelmed with
support requests from users on older versions.

Thanks

Dave

On Thu, 27 Aug 2015 09:21 Daniel Theophanes notifications@github.com
wrote:

@pkieltyka https://github.com/pkieltyka Maybe I suck at marketing...
I've made one that can remove tests here: github.com/kardianos/govendor .
Not trying to promote it here, but once you have a tool that works well,
adding a test filter isn't too hard. It only copies over needed or declared
packages so go install ./... also works because main packages aren't
copied over by default either. I'm sure other tools could do similar things.


Reply to this email directly or view it on GitHub
#11659 (comment).

Contributor

davecheney commented Aug 26, 2015

One other thing to keep in mind. As much as we want everyone to use Go 1.5
immediately, experience has shown that there are many go programs out there
being built with older versions on a if it ain't broke, don't touch it,
basis.

For those older versions, and more importantly library writers, who want to
adopt the versioning experiment in their code, using _vendor would allow
those library writers to try this feature without being overwhelmed with
support requests from users on older versions.

Thanks

Dave

On Thu, 27 Aug 2015 09:21 Daniel Theophanes notifications@github.com
wrote:

@pkieltyka https://github.com/pkieltyka Maybe I suck at marketing...
I've made one that can remove tests here: github.com/kardianos/govendor .
Not trying to promote it here, but once you have a tool that works well,
adding a test filter isn't too hard. It only copies over needed or declared
packages so go install ./... also works because main packages aren't
copied over by default either. I'm sure other tools could do similar things.


Reply to this email directly or view it on GitHub
#11659 (comment).

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Aug 27, 2015

Contributor

@davecheney I understand you are saying using "_vendor", which existing go tool will ignore, will result in fewer support requests to package authors. But the same is true for the go 1.5 tool as well in the context of this issue because go test ./... and go install ./... will also do the same thing as older versions. So while I think I understand what you are saying, I don't follow your logic.

It would appear you think that tools shouldn't exclude test files should the author request it. Is that true? Could you explain why?

Contributor

kardianos commented Aug 27, 2015

@davecheney I understand you are saying using "_vendor", which existing go tool will ignore, will result in fewer support requests to package authors. But the same is true for the go 1.5 tool as well in the context of this issue because go test ./... and go install ./... will also do the same thing as older versions. So while I think I understand what you are saying, I don't follow your logic.

It would appear you think that tools shouldn't exclude test files should the author request it. Is that true? Could you explain why?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Aug 27, 2015

Contributor

@kardianos

But the same is true for the go 1.5 tool as well in the context of this issue because go test ./... and go install ./... will also do the same thing as older versions. So while I think I understand what you are saying, I don't follow your logic.

I don't believe this is true, Go 1.4 will see .../vendor/... as a valid path and attempt to build, test and install code it find inside it. If the path were .../_vendor/... it would be ignored by Go 1.4.2. Am I misunderstanding how Go 1.4 works ?

It would appear you think that tools shouldn't exclude test files should the author request it. Is that true? Could you explain why?

I am sorry that I gave that impression. I am saying the opposite.

Contributor

davecheney commented Aug 27, 2015

@kardianos

But the same is true for the go 1.5 tool as well in the context of this issue because go test ./... and go install ./... will also do the same thing as older versions. So while I think I understand what you are saying, I don't follow your logic.

I don't believe this is true, Go 1.4 will see .../vendor/... as a valid path and attempt to build, test and install code it find inside it. If the path were .../_vendor/... it would be ignored by Go 1.4.2. Am I misunderstanding how Go 1.4 works ?

It would appear you think that tools shouldn't exclude test files should the author request it. Is that true? Could you explain why?

I am sorry that I gave that impression. I am saying the opposite.

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Aug 28, 2015

Contributor

@kardianos I don't want to limit Go users into having to use a specific tool in order to properly vendor things, unless that tool also ships as part of the go toolchain. FWIW: I'd be perfectly happy if govendor was that tool, although it doesn't provide for all of my needs.

Contributor

freeformz commented Aug 28, 2015

@kardianos I don't want to limit Go users into having to use a specific tool in order to properly vendor things, unless that tool also ships as part of the go toolchain. FWIW: I'd be perfectly happy if govendor was that tool, although it doesn't provide for all of my needs.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Aug 28, 2015

Contributor

@freeformz I'm not suggesting using a single tool. I'm suggesting other tools work a little bit harder.

Contributor

kardianos commented Aug 28, 2015

@freeformz I'm not suggesting using a single tool. I'm suggesting other tools work a little bit harder.

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Aug 28, 2015

Contributor

Any chance we could see a change to _vendor in 1.5.1? Would a CL for that be helpful?

Contributor

danp commented Aug 28, 2015

Any chance we could see a change to _vendor in 1.5.1? Would a CL for that be helpful?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 28, 2015

Member

@dpiddy, we have an extremely high bar for changes in point releases (1.n.m): security bugs, or bugs with no possible workaround, and only if they're a new regression (not something we've had for the past few major releases). Tweaking an experimental feature meets none of the criteria.

Member

bradfitz commented Aug 28, 2015

@dpiddy, we have an extremely high bar for changes in point releases (1.n.m): security bugs, or bugs with no possible workaround, and only if they're a new regression (not something we've had for the past few major releases). Tweaking an experimental feature meets none of the criteria.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Aug 28, 2015

Member
Member

minux commented Aug 28, 2015

@freeformz freeformz closed this Aug 28, 2015

@freeformz freeformz reopened this Aug 28, 2015

@joshuarubin

This comment has been minimized.

Show comment
Hide comment
@joshuarubin

joshuarubin Aug 28, 2015

Contributor

@minux It also doesn't scale well to have to list every package by hand in a Makefile (for example) so that make test will test all non-vendored packages. That introduces a human element that will certainly be forgotten. Obviously this can be scripted, but it is very ugly. Here are some lines of a Makefile I have for a go1.5 vendored project:

ALL_DIRS=$(shell find . \( -path ./vendor -o -path ./.git \) -prune -o -type d -print)
GO_PKGS=$(foreach pkg, $(shell go list ./...), $(if $(findstring /vendor/, $(pkg)), , $(pkg)))

lint:
    @golint ./... | grep -v '^vendor\/' || true
    @go vet ./... 2>&1 | grep -v '^vendor\/' | grep -v '^exit\ status\ 1'

test:
    @for pkg in $(GO_PKGS); do \
        cmd="go test -v $$pkg"; \
        eval $$cmd; \
        if test $$? -ne 0; then \
            exit 1; \
        fi; \
    done
Contributor

joshuarubin commented Aug 28, 2015

@minux It also doesn't scale well to have to list every package by hand in a Makefile (for example) so that make test will test all non-vendored packages. That introduces a human element that will certainly be forgotten. Obviously this can be scripted, but it is very ugly. Here are some lines of a Makefile I have for a go1.5 vendored project:

ALL_DIRS=$(shell find . \( -path ./vendor -o -path ./.git \) -prune -o -type d -print)
GO_PKGS=$(foreach pkg, $(shell go list ./...), $(if $(findstring /vendor/, $(pkg)), , $(pkg)))

lint:
    @golint ./... | grep -v '^vendor\/' || true
    @go vet ./... 2>&1 | grep -v '^vendor\/' | grep -v '^exit\ status\ 1'

test:
    @for pkg in $(GO_PKGS); do \
        cmd="go test -v $$pkg"; \
        eval $$cmd; \
        if test $$? -ne 0; then \
            exit 1; \
        fi; \
    done
@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Aug 28, 2015

Contributor

This isn't just about testing. Here are my thoughts to date....

When vendor/ is the name of the vendoring directory and a package spec of ./... matches everything in that directory as well...

The issues I have with this are:

  1. Most people I've talked to don't expect the package spec ./... to match stuff in vendor/. This indicates to me that it's counter intuitive. I will likely have to do a lot more education about packages / paths / etc. This isn't really a bad thing, but most people I talk to are already confused and/or frustrated by the way $GOPATH works. I don't want to add to that confusion.
  2. It's impossible to test only the packages that I'm writing with a repo level ./.... I care about testing the packages I'm writing more regularly than testing the stuff I've vendored. I'll test the stuff I vendor when I originally vendor it and/or when I upgrade the vendored deps. IMO/E more people follow this workflow than the test everything all the time workflow. I'm worried that this will end up leading people to encode test Makefile or similar targets to avoid having to remember / type the go test $(go list ./... | grep -v vendor/) stuff. I thought we wanted to avoid that?
  3. Additionally some package's tests require additional setup which I may not have on my local machine / environment.
  4. I will require additional meta data about what to install when someone pushes code to me. Right now I can assume that go install ./... will install all of the packages in the pushed repo. Godeps handles this case just fine by recording the package spec(s) used during godep save, but I can tell you most people just use ./...
  5. Similarly for projects with multiple main packages for commands, it will require additional documentation where it's now possible to simply say go get github.com/my/proj/.... This is probably pretty minor though.
  6. As per @davecheney's comment: Not everyone is using Go 1.5. vendor/ will cause issues for users of Go versions < 1.5.

Some, but not all of the above stuff can be handled by vendoring tools. IMO if we're going to cause this much disruption then Go itself should provide a vendoring tool such as govendor/godeps/something new/whatever as part of the standard tooling (i.e. go vendor).

Contributor

freeformz commented Aug 28, 2015

This isn't just about testing. Here are my thoughts to date....

When vendor/ is the name of the vendoring directory and a package spec of ./... matches everything in that directory as well...

The issues I have with this are:

  1. Most people I've talked to don't expect the package spec ./... to match stuff in vendor/. This indicates to me that it's counter intuitive. I will likely have to do a lot more education about packages / paths / etc. This isn't really a bad thing, but most people I talk to are already confused and/or frustrated by the way $GOPATH works. I don't want to add to that confusion.
  2. It's impossible to test only the packages that I'm writing with a repo level ./.... I care about testing the packages I'm writing more regularly than testing the stuff I've vendored. I'll test the stuff I vendor when I originally vendor it and/or when I upgrade the vendored deps. IMO/E more people follow this workflow than the test everything all the time workflow. I'm worried that this will end up leading people to encode test Makefile or similar targets to avoid having to remember / type the go test $(go list ./... | grep -v vendor/) stuff. I thought we wanted to avoid that?
  3. Additionally some package's tests require additional setup which I may not have on my local machine / environment.
  4. I will require additional meta data about what to install when someone pushes code to me. Right now I can assume that go install ./... will install all of the packages in the pushed repo. Godeps handles this case just fine by recording the package spec(s) used during godep save, but I can tell you most people just use ./...
  5. Similarly for projects with multiple main packages for commands, it will require additional documentation where it's now possible to simply say go get github.com/my/proj/.... This is probably pretty minor though.
  6. As per @davecheney's comment: Not everyone is using Go 1.5. vendor/ will cause issues for users of Go versions < 1.5.

Some, but not all of the above stuff can be handled by vendoring tools. IMO if we're going to cause this much disruption then Go itself should provide a vendoring tool such as govendor/godeps/something new/whatever as part of the standard tooling (i.e. go vendor).

@pkieltyka

This comment has been minimized.

Show comment
Hide comment
@pkieltyka

pkieltyka Aug 28, 2015

I second @freeformz's points FWIW..

I second @freeformz's points FWIW..

@benma

This comment has been minimized.

Show comment
Hide comment
@benma

benma Aug 28, 2015

@minux

And If you just want to run all the tests, I think knowing that the vendored packages also works correctly is also desirable.

If I add dependency X that is only a sub-package of a larger package Y, the whole thing ends up in the vendored folder, even if I only need the subpackage and its dependencies. If I test the whole project with go test ./..., it tests the whole package Y. I don't need to test all of Y, and worse, it will fail if I did not add its recursive dependencies, which I don't need for package X.

You may ask why I add the all of Y if I only need X. The reason is that I use git submodules to add the repo, and go get behaves the same way.

benma commented Aug 28, 2015

@minux

And If you just want to run all the tests, I think knowing that the vendored packages also works correctly is also desirable.

If I add dependency X that is only a sub-package of a larger package Y, the whole thing ends up in the vendored folder, even if I only need the subpackage and its dependencies. If I test the whole project with go test ./..., it tests the whole package Y. I don't need to test all of Y, and worse, it will fail if I did not add its recursive dependencies, which I don't need for package X.

You may ask why I add the all of Y if I only need X. The reason is that I use git submodules to add the repo, and go get behaves the same way.

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Aug 28, 2015

Contributor

I share all of @freeformz's concerns. I think it is telling that at least godep has always used an ignored _workspace path, even when using godep save -r which made saved dependencies valid import paths as the vendoring experiment does.

@bradfitz that is what I figured. I fear if it takes a major release to change the experiment's behavior enough momentum (whether because it's good or not) will be built with the current behavior that it will be unlikely to ever change.

Contributor

danp commented Aug 28, 2015

I share all of @freeformz's concerns. I think it is telling that at least godep has always used an ignored _workspace path, even when using godep save -r which made saved dependencies valid import paths as the vendoring experiment does.

@bradfitz that is what I figured. I fear if it takes a major release to change the experiment's behavior enough momentum (whether because it's good or not) will be built with the current behavior that it will be unlikely to ever change.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 28, 2015

Member

My main concern, before I can start to think about others, is that of the import paths. As I understand example.com/foo/bar/vendor/example.com/foo/baz is not a valid import path. example.com/foo/baz is a valid import path of a Go package that happens to be vendored inside example.com/foo/bar.

Are other people okay with the idea that go list all will print out a bunch of invalid import paths (in addition to valid ones)? How should other tools deal with these invalid import paths?

I thought the point of vendoring was to put a copy of the original package (e.g., example.com/foo/baz) inside your VCS tree, but not change it in any way. No import path rewriting required - the vendored package still has the same original import path. You should be able to vendor/not vendor any package as you wish. It seems to do that but also change import paths of the packages being vendored.

Member

dmitshur commented Aug 28, 2015

My main concern, before I can start to think about others, is that of the import paths. As I understand example.com/foo/bar/vendor/example.com/foo/baz is not a valid import path. example.com/foo/baz is a valid import path of a Go package that happens to be vendored inside example.com/foo/bar.

Are other people okay with the idea that go list all will print out a bunch of invalid import paths (in addition to valid ones)? How should other tools deal with these invalid import paths?

I thought the point of vendoring was to put a copy of the original package (e.g., example.com/foo/baz) inside your VCS tree, but not change it in any way. No import path rewriting required - the vendored package still has the same original import path. You should be able to vendor/not vendor any package as you wish. It seems to do that but also change import paths of the packages being vendored.

@wardn

This comment has been minimized.

Show comment
Hide comment
@wardn

wardn Sep 2, 2015

I've been dealing with the repercussions of the vendoring experiment for a few weeks now. I like having fine-grained control over my dependencies, and it’s awesome to be able to commit the vendor source along with my project source without having to change all the vendored import paths.

I think I voice the same concern as most of the non-googlers here, which is that I didn’t expect to have golint, go vet, and go test blow up in my face when I moved everything to the vendor directory. It’s frustrating to have to contort my workflow to accommodate external code that didn’t previously require it.

However, I wouldn’t have known about many of these problems with the third-party libraries had I not moved them into the vendor directory. It’s been a pain to get everything to play nice, and I understand that not everyone has the luxury of being able to fix other-people’s-code, but the new methodology incentivizes quality code… which is one reason I believe Go is as successful as it is today.

If I compare the two solutions from a long-term perspective, I see two very different outcomes. Allowing people to ignore bad code results in a complex solution whereby some code is whitelisted, some is blacklisted, the lists require regular maintenance, and you have no guarantee of a minimum level of quality. The other solution is to develop good code, fork bad code, and take the penalty of due diligence up front.

I’m not saying either solution is right or wrong, but since this issue is still open for feedback, I’m of the opinion that the short-term pain of broken libraries will eventually subside as the authors feel the pain and/or new authors emerge. I believe that the long-term result will be a better foundation from which to build future solutions.

wardn commented Sep 2, 2015

I've been dealing with the repercussions of the vendoring experiment for a few weeks now. I like having fine-grained control over my dependencies, and it’s awesome to be able to commit the vendor source along with my project source without having to change all the vendored import paths.

I think I voice the same concern as most of the non-googlers here, which is that I didn’t expect to have golint, go vet, and go test blow up in my face when I moved everything to the vendor directory. It’s frustrating to have to contort my workflow to accommodate external code that didn’t previously require it.

However, I wouldn’t have known about many of these problems with the third-party libraries had I not moved them into the vendor directory. It’s been a pain to get everything to play nice, and I understand that not everyone has the luxury of being able to fix other-people’s-code, but the new methodology incentivizes quality code… which is one reason I believe Go is as successful as it is today.

If I compare the two solutions from a long-term perspective, I see two very different outcomes. Allowing people to ignore bad code results in a complex solution whereby some code is whitelisted, some is blacklisted, the lists require regular maintenance, and you have no guarantee of a minimum level of quality. The other solution is to develop good code, fork bad code, and take the penalty of due diligence up front.

I’m not saying either solution is right or wrong, but since this issue is still open for feedback, I’m of the opinion that the short-term pain of broken libraries will eventually subside as the authors feel the pain and/or new authors emerge. I believe that the long-term result will be a better foundation from which to build future solutions.

@kr

This comment has been minimized.

Show comment
Hide comment
@kr

kr Sep 2, 2015

Contributor

I was asked to provide my opinion here, so here it is.

I agree with @rsc's comment in #11659 (comment).

Further, it seems renaming the directory to _vendor would make it harder to operate on the vendored packages. For example, it's useful (at least to me) to be able to run go list ./vendor/... and see everything that's been vendored.

Personally, I think the current behavior is ok. It hasn't really been a problem for me (though I understand why some find it inconvenient). For example, when I run interactive commands to test or vet packages, I type go test or go test ./foo ./bar or maybe go test ./foo/... since I generally want to test only one or two packages at a time. Our automated testing system tests all our packages, using a command like go test $(go list ./...|grep -v vendor). This would be inconvenient to type by hand, but there is more than one way to make it convenient. Complicating the behavior of the go command probably isn't the best way.

Contributor

kr commented Sep 2, 2015

I was asked to provide my opinion here, so here it is.

I agree with @rsc's comment in #11659 (comment).

Further, it seems renaming the directory to _vendor would make it harder to operate on the vendored packages. For example, it's useful (at least to me) to be able to run go list ./vendor/... and see everything that's been vendored.

Personally, I think the current behavior is ok. It hasn't really been a problem for me (though I understand why some find it inconvenient). For example, when I run interactive commands to test or vet packages, I type go test or go test ./foo ./bar or maybe go test ./foo/... since I generally want to test only one or two packages at a time. Our automated testing system tests all our packages, using a command like go test $(go list ./...|grep -v vendor). This would be inconvenient to type by hand, but there is more than one way to make it convenient. Complicating the behavior of the go command probably isn't the best way.

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Sep 2, 2015

Contributor

Thanks for all of your input. I think everything has been covered here that could be covered.

I'm satisfied with the outcome, although I still think it ignores usability a bit, so I'm going to close this issue (since I opened it).

Contributor

freeformz commented Sep 2, 2015

Thanks for all of your input. I think everything has been covered here that could be covered.

I'm satisfied with the outcome, although I still think it ignores usability a bit, so I'm going to close this issue (since I opened it).

@freeformz freeformz closed this Sep 2, 2015

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 6, 2015

Member
Member

minux commented Sep 6, 2015

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 6, 2015

Member

That doesn't help with the issue where go list all outputs invalid import paths (sorry, I'm repeating myself).

Member

dmitshur commented Sep 6, 2015

That doesn't help with the issue where go list all outputs invalid import paths (sorry, I'm repeating myself).

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Sep 6, 2015

Member
Member

minux commented Sep 6, 2015

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 6, 2015

Member

But I said go list all specifically, not just go list.

I have multiple tools that use the output of go list all to navigate and open Go packages. For example, if I want to go to github.com/russross/blackfriday, I expect there to be just one. With vendoring as is, there may be 10, 20 or 30+ matches for blackfriday in various vendor folders.

All tools that prefer to preserve previous behavior of not including vendored package copies (e.g., like they did not include the vendored packages in Godeps/_workspace subfolders) need to be updated to perform go list all and then try to filter out vendored copies. Even if GO15VENDOREXPERIMENT is not enabled.

Member

dmitshur commented Sep 6, 2015

But I said go list all specifically, not just go list.

I have multiple tools that use the output of go list all to navigate and open Go packages. For example, if I want to go to github.com/russross/blackfriday, I expect there to be just one. With vendoring as is, there may be 10, 20 or 30+ matches for blackfriday in various vendor folders.

All tools that prefer to preserve previous behavior of not including vendored package copies (e.g., like they did not include the vendored packages in Godeps/_workspace subfolders) need to be updated to perform go list all and then try to filter out vendored copies. Even if GO15VENDOREXPERIMENT is not enabled.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Sep 6, 2015

Contributor

@shurcooL In hopes of being helpful, you might try to migrate your tools to use something else. For instance you could try http://godoc.org/github.com/kardianos/govendor/context create a new context then call ctx.Status(), then filter all the returned items by their "status" which includes if they are in a vendor directory or not. If that doesn't quite fit your need, but is close, feel free to open an issue.

Contributor

kardianos commented Sep 6, 2015

@shurcooL In hopes of being helpful, you might try to migrate your tools to use something else. For instance you could try http://godoc.org/github.com/kardianos/govendor/context create a new context then call ctx.Status(), then filter all the returned items by their "status" which includes if they are in a vendor directory or not. If that doesn't quite fit your need, but is close, feel free to open an issue.

lyoshenka added a commit to lyoshenka/heroku-buildpack-go that referenced this issue Sep 7, 2015

@awishformore

This comment has been minimized.

Show comment
Hide comment
@awishformore

awishformore Jan 14, 2016

I have been doing research and setting up a continuous integration & testing pipeline for Go during most of the day, on different tools, and this is an issue that really bothers me.

I love Go for it's minimalist and clean approach. In line with that, I use the clean architecture approach (or something close to it) for most of my projects. This means I will usually have four subpackages, with the main package in the root folder. Sometimes, there are some small additional tools in the cmd folder as well. I was under the impression that this is a configuration that most projects use.

I now have to use a dirty workaround to get CI to build and test properly, all due to the fact that ./.. includes folders in vendor. This is counter-intuitive and not well thought-out at all. Which is contrary to what I've come to expect and appreciate about Go. The default case should not require any workaround.

There should be a coherent approach where all go tools exclude vendor when using ./... if the GO15VENDOREXPERIMENT environment variable is set to 1. That would be consistent. As someone mentioned before, you can still do "cd vendor && go test ./..." if you want to include the subpackages. But you will have to be explicit about it, which is fine since it constitutes a non-default workflow imo.

If the go tool deals with vendoring using the environment flag (and by default in Go 1.6), then it should also deal with the resulting consequences and solve them in a coherent way. Let's keep it simple, folks.

For now, I'm forced to switch back to godep to keep a clean non-hacky script in my repository.

I have been doing research and setting up a continuous integration & testing pipeline for Go during most of the day, on different tools, and this is an issue that really bothers me.

I love Go for it's minimalist and clean approach. In line with that, I use the clean architecture approach (or something close to it) for most of my projects. This means I will usually have four subpackages, with the main package in the root folder. Sometimes, there are some small additional tools in the cmd folder as well. I was under the impression that this is a configuration that most projects use.

I now have to use a dirty workaround to get CI to build and test properly, all due to the fact that ./.. includes folders in vendor. This is counter-intuitive and not well thought-out at all. Which is contrary to what I've come to expect and appreciate about Go. The default case should not require any workaround.

There should be a coherent approach where all go tools exclude vendor when using ./... if the GO15VENDOREXPERIMENT environment variable is set to 1. That would be consistent. As someone mentioned before, you can still do "cd vendor && go test ./..." if you want to include the subpackages. But you will have to be explicit about it, which is fine since it constitutes a non-default workflow imo.

If the go tool deals with vendoring using the environment flag (and by default in Go 1.6), then it should also deal with the resulting consequences and solve them in a coherent way. Let's keep it simple, folks.

For now, I'm forced to switch back to godep to keep a clean non-hacky script in my repository.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 14, 2016

Contributor

I think there are two cases:

  1. A package in a vendor tree that is actually used (directly or
    indirectly) by the code outside the vendor tree. In this case I think you
    would want to test the package, just as you test all your other
    dependencies. If code you depend on is broken, you want to find out in the
    most precise test possible, not wait until a test of the main program fails
    mysteriously.
  2. A package in a vendor tree that is not actually used (directly or
    indirectly) by the code outside the vendor tree. In this case, obviously no
    one cares and the test need not run. But in this case I don't understand
    why tools are bringing that package into the vendor tree in the first
    place. I would argue that, for many reasons, dead (unused) code should not
    be copied into a vendor tree. Code that is in your vendor tree has to be
    maintained, updated, and so on. If it's not needed, it should just be
    omitted.

I think it's a mistake to skip the test in (1). And I think (2) indicates a
problem in the vendoring process.

The counter-argument to this is that you can't omit individual packages
when you're using git submodules. But when you're using git submodules I
think you're tying yourself to the target repository quite a bit more and
so it may still be appropriate to test the entire thing.

Contributor

rsc commented Jan 14, 2016

I think there are two cases:

  1. A package in a vendor tree that is actually used (directly or
    indirectly) by the code outside the vendor tree. In this case I think you
    would want to test the package, just as you test all your other
    dependencies. If code you depend on is broken, you want to find out in the
    most precise test possible, not wait until a test of the main program fails
    mysteriously.
  2. A package in a vendor tree that is not actually used (directly or
    indirectly) by the code outside the vendor tree. In this case, obviously no
    one cares and the test need not run. But in this case I don't understand
    why tools are bringing that package into the vendor tree in the first
    place. I would argue that, for many reasons, dead (unused) code should not
    be copied into a vendor tree. Code that is in your vendor tree has to be
    maintained, updated, and so on. If it's not needed, it should just be
    omitted.

I think it's a mistake to skip the test in (1). And I think (2) indicates a
problem in the vendoring process.

The counter-argument to this is that you can't omit individual packages
when you're using git submodules. But when you're using git submodules I
think you're tying yourself to the target repository quite a bit more and
so it may still be appropriate to test the entire thing.

@awishformore

This comment has been minimized.

Show comment
Hide comment
@awishformore

awishformore Jan 14, 2016

@rsc I disagree even with your first point. If you vendor your dependencies, you will usually pin a specific version, so it is nonsense to run the tests each and every time you push a commit and your CI kicks in. You should test your dependencies once, manually, each time you update them. That's completely besides the point of automatic testing of your own commits and pull requests.

@rsc I disagree even with your first point. If you vendor your dependencies, you will usually pin a specific version, so it is nonsense to run the tests each and every time you push a commit and your CI kicks in. You should test your dependencies once, manually, each time you update them. That's completely besides the point of automatic testing of your own commits and pull requests.

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Jan 14, 2016

Contributor

Agree with @awishformore, similar to what I said in #11659 (comment).

How many projects that use net/http run those tests every time? Would that be reasonable? I don't think so, because presumably you've verified that the Go release you're using is good and you know net/http isn't changing out from under you.

I feel things in vendor/ can be treated similarly: on add/change, verify it works for you, then be sure the vendored code doesn't change.

Contributor

danp commented Jan 14, 2016

Agree with @awishformore, similar to what I said in #11659 (comment).

How many projects that use net/http run those tests every time? Would that be reasonable? I don't think so, because presumably you've verified that the Go release you're using is good and you know net/http isn't changing out from under you.

I feel things in vendor/ can be treated similarly: on add/change, verify it works for you, then be sure the vendored code doesn't change.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 14, 2016

Contributor

As others have noted, there is an easy workaround if you just want to test
everything but vendor:

go test $(go list ./... | grep -v /vendor/)

Depending on your configuration, it may be appropriate or worthwhile to use
that in your CI system. My point is that it may also be appropriate or
worthwhile to include those tests, so it is not appropriate for ./... to
categorically exclude certain directories. Different people use vendor
differently. Some people do make local changes, or do want to include it in
"go vet ./..." or other commands.

I also still think that for the specific case of testing, if we could solve
test caching then that would help considerably.

Contributor

rsc commented Jan 14, 2016

As others have noted, there is an easy workaround if you just want to test
everything but vendor:

go test $(go list ./... | grep -v /vendor/)

Depending on your configuration, it may be appropriate or worthwhile to use
that in your CI system. My point is that it may also be appropriate or
worthwhile to include those tests, so it is not appropriate for ./... to
categorically exclude certain directories. Different people use vendor
differently. Some people do make local changes, or do want to include it in
"go vet ./..." or other commands.

I also still think that for the specific case of testing, if we could solve
test caching then that would help considerably.

@awishformore

This comment has been minimized.

Show comment
Hide comment
@awishformore

awishformore Jan 14, 2016

@rsc Please don't qualify it as an "easy" workaround. It's a dirty workaround for something that should work out-of-the-box. Why? Because someone decided that the Go tool should deal with vendoring, and since it does, the default way should correspond to the most intuitive and common approach. The workaround should be required if you do something out of the ordinary, like testing dependencies on each build or, like you mention, customizing dependencies. In that case, a workaround (i.e. cd vendor && go test ./...) seems perfectly fine. I don't understand why you argue to make the default more annoying in favour of some edge cases. One of the most wonderful things about Go is that defaults usually work in intuitive ways and hardly ever annoy me.

@rsc Please don't qualify it as an "easy" workaround. It's a dirty workaround for something that should work out-of-the-box. Why? Because someone decided that the Go tool should deal with vendoring, and since it does, the default way should correspond to the most intuitive and common approach. The workaround should be required if you do something out of the ordinary, like testing dependencies on each build or, like you mention, customizing dependencies. In that case, a workaround (i.e. cd vendor && go test ./...) seems perfectly fine. I don't understand why you argue to make the default more annoying in favour of some edge cases. One of the most wonderful things about Go is that defaults usually work in intuitive ways and hardly ever annoy me.

@kr

This comment has been minimized.

Show comment
Hide comment
@kr

kr Jan 15, 2016

Contributor

You should test your dependencies once, manually, each time you update them.

It's more convenient and simpler (for both the user and for the go tool's implementation) to test them every time you run your own tests. I'd love to know, concretely, why some folks are so resistant to this idea. Is it because running more tests is slower? (If so, see @rsc's last comment above.)

It's a dirty workaround for something that should work out-of-the-box.

From my point of view, it does work out of the box. That's because I like to run the tests for my dependencies. Of course, anyone is free to prefer otherwise, but that preference doesn't necessarily imply that the go tool's design is wrong.

Contributor

kr commented Jan 15, 2016

You should test your dependencies once, manually, each time you update them.

It's more convenient and simpler (for both the user and for the go tool's implementation) to test them every time you run your own tests. I'd love to know, concretely, why some folks are so resistant to this idea. Is it because running more tests is slower? (If so, see @rsc's last comment above.)

It's a dirty workaround for something that should work out-of-the-box.

From my point of view, it does work out of the box. That's because I like to run the tests for my dependencies. Of course, anyone is free to prefer otherwise, but that preference doesn't necessarily imply that the go tool's design is wrong.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Jan 15, 2016

Member

That's because I like to run the tests for my dependencies.

In case it's helpful, here's an easy workaround if you want to test dependencies:

go test $(go list -f '{{join .Deps "\n"}}' ./...)
Member

dmitshur commented Jan 15, 2016

That's because I like to run the tests for my dependencies.

In case it's helpful, here's an easy workaround if you want to test dependencies:

go test $(go list -f '{{join .Deps "\n"}}' ./...)
@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Jan 15, 2016

Contributor

@kr build time is certainly a factor with current Go versions. I also gave my reasons in #11659 (comment) and other comments.

Perhaps my mental model of what vendor/ is is just wrong. I consider it to be an extension of the stdlib for my project. If that is true, should I be running tests for everything I've used from the stdlib every time I test my project?

Contributor

danp commented Jan 15, 2016

@kr build time is certainly a factor with current Go versions. I also gave my reasons in #11659 (comment) and other comments.

Perhaps my mental model of what vendor/ is is just wrong. I consider it to be an extension of the stdlib for my project. If that is true, should I be running tests for everything I've used from the stdlib every time I test my project?

@danp

This comment has been minimized.

Show comment
Hide comment
@danp

danp Jan 18, 2016

Contributor

Of course I'm forgetting that an easy way to exclude vendored tests is to do just that: exclude them when vendoring things. At least godep and govendor already support this. Same for unwanted main packages.

Curious to see how things like go install github.com/foo/bar/... play out once 1.6 is released.

Contributor

danp commented Jan 18, 2016

Of course I'm forgetting that an easy way to exclude vendored tests is to do just that: exclude them when vendoring things. At least godep and govendor already support this. Same for unwanted main packages.

Curious to see how things like go install github.com/foo/bar/... play out once 1.6 is released.

@natefinch

This comment has been minimized.

Show comment
Hide comment
@natefinch

natefinch Jan 20, 2016

Contributor

The fact that everyone is proposing workarounds just points to the fact that this should be fixed. Renaming it to _vendor just works. If you want to test or install stuff from your vendor directory, you can by just cd'ing into that directory.... but if you don't want to, then you can go whatever ./... just like you do today, and it'll work just like it does today. Making it "vendor" breaks things. I can't imagine anyone wanting that to be the default.

Keep go simple. Make the vendor directory _vendor so that tools skip it. I shouldn't have to use some annoyingly arcane command to just build and test my code.

Contributor

natefinch commented Jan 20, 2016

The fact that everyone is proposing workarounds just points to the fact that this should be fixed. Renaming it to _vendor just works. If you want to test or install stuff from your vendor directory, you can by just cd'ing into that directory.... but if you don't want to, then you can go whatever ./... just like you do today, and it'll work just like it does today. Making it "vendor" breaks things. I can't imagine anyone wanting that to be the default.

Keep go simple. Make the vendor directory _vendor so that tools skip it. I shouldn't have to use some annoyingly arcane command to just build and test my code.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 20, 2016

Member

"vendor" is not being renamed. That ship has long sailed.

I think everything that can be said on this issue has been said in the many comments above. I think it's time we move this to other forums (for general discussion) or new bugs (for specific new bugs that aren't just a dup of this one). Ultimately this bug is a policy/behavior question and not actually a bug report.

Member

bradfitz commented Jan 20, 2016

"vendor" is not being renamed. That ship has long sailed.

I think everything that can be said on this issue has been said in the many comments above. I think it's time we move this to other forums (for general discussion) or new bugs (for specific new bugs that aren't just a dup of this one). Ultimately this bug is a policy/behavior question and not actually a bug report.

@golang golang locked and limited conversation to collaborators Jan 20, 2016

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