Proposal: allow importing of vendored package even not if not in the same subtree #15162

Closed
minux opened this Issue Apr 7, 2016 · 14 comments

Comments

Projects
None yet
8 participants
@minux
Member

minux commented Apr 7, 2016

Recent golang-dev discussion [https://groups.google.com/forum/#!topic/golang-dev/4FfTBfN2YaI]
mentions a problem that a library package vendors a package and also expose the (API) interfaces
from that vendored package in an exported interface.

Because outside packages can't import the vendored package, outside packages can't implement
the interface.

However, vendored package is not supposed to be a mechanism to hide packages (we have
internal packages for that purpose), I propose that we make cmd/go allow importing of
vendored package using the explicit import path ("path/to/pkg/vendor/pkgA"), unless the
vendored package is also part of internal packages ("path/to/pkg/internal/vendor/pkgA").

The rationale is that I think just forbidding library packages (as oppose to binaries) from
vendoring packages is not going to work and there are legitimate reasons to vendor a
package in a library package. Importing a vendored package this way is equivalent to
using a certain version of a package chosen by the maintainer of the vendoring package,
which also makes sense.

This change will also make vendoring and internal package more orthogonal (internal
to control visibility, and vendor to provide a mechanism for import path rewrite).

@minux minux added the Proposal label Apr 7, 2016

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Apr 7, 2016

Member

I think this is terrible and is fixing the wrong problem. This would mean that if etcd vendors context and exposes an vendored context in their API, two parties (let's call them A and B) could implement that interface by referring to the long ugly path/to/pkg/vendor/context but A and B would be unable to use that same implementation to implement interfaces taking a different context package, or even the real context package.

I think this would make the situation worse.

Member

bradfitz commented Apr 7, 2016

I think this is terrible and is fixing the wrong problem. This would mean that if etcd vendors context and exposes an vendored context in their API, two parties (let's call them A and B) could implement that interface by referring to the long ugly path/to/pkg/vendor/context but A and B would be unable to use that same implementation to implement interfaces taking a different context package, or even the real context package.

I think this would make the situation worse.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Apr 7, 2016

Contributor

I concur with @bradfitz.

I want everyone to stop trying to think about how to make a computer solve the most complicated form of this problem, and instead think about how you would go about explaining the correct way to use vendor/ to a newcomer to the language.

Software dependency management in Go projects is an issue that affects all of us, but it affects newcomers to the language who want to use Go but do not have the level of background experience to be able to evaluate sentences like

allow importing of vendored package using the explicit import path ("path/to/pkg/vendor/pkgA"), unless the vendored package is also part of internal packages ("path/to/pkg/internal/vendor/pkgA")

Please, before trying to make this complex situation even more complex, think about usability of the solution you are proposing to someone who has no experience writing Go.

Contributor

davecheney commented Apr 7, 2016

I concur with @bradfitz.

I want everyone to stop trying to think about how to make a computer solve the most complicated form of this problem, and instead think about how you would go about explaining the correct way to use vendor/ to a newcomer to the language.

Software dependency management in Go projects is an issue that affects all of us, but it affects newcomers to the language who want to use Go but do not have the level of background experience to be able to evaluate sentences like

allow importing of vendored package using the explicit import path ("path/to/pkg/vendor/pkgA"), unless the vendored package is also part of internal packages ("path/to/pkg/internal/vendor/pkgA")

Please, before trying to make this complex situation even more complex, think about usability of the solution you are proposing to someone who has no experience writing Go.

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Apr 7, 2016

Member
Member

minux commented Apr 7, 2016

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Apr 7, 2016

Contributor

Put it another way, if vendoring is just a form of import path rewriting,
why you can't directly import that rewritten package? Conventional
import path rewriting (if not used with internal package) allows direct
importing.

I agree with that logic. However, this introduces yet another decision that a newcomer has to make -- code has been placed in vendor/ and it now can be imported by it's full import path, or by the alias that the go tool provides via the vendor logic. This adds more choice and more confusion. I don't see this as an answer, because the root cause of all of these problems is code which used to have exactly one name in the import graph now has more than one, so type equality is busted.

Contributor

davecheney commented Apr 7, 2016

Put it another way, if vendoring is just a form of import path rewriting,
why you can't directly import that rewritten package? Conventional
import path rewriting (if not used with internal package) allows direct
importing.

I agree with that logic. However, this introduces yet another decision that a newcomer has to make -- code has been placed in vendor/ and it now can be imported by it's full import path, or by the alias that the go tool provides via the vendor logic. This adds more choice and more confusion. I don't see this as an answer, because the root cause of all of these problems is code which used to have exactly one name in the import graph now has more than one, so type equality is busted.

@bradfitz bradfitz added this to the Unplanned milestone Apr 7, 2016

@bradfitz bradfitz added the Vendoring label Apr 10, 2016

@dsymonds

This comment has been minimized.

Show comment
Hide comment
@dsymonds

dsymonds Apr 13, 2016

Member

I think the better and simpler solution is to treat vendor/ as an alias for internal/ for the purpose of checking whether an import is permitted, and then discourage people from using types in vendored packages in their APIs. I don't see a compelling need for a vendored package to be imported from outside the place where it is vendored.

Member

dsymonds commented Apr 13, 2016

I think the better and simpler solution is to treat vendor/ as an alias for internal/ for the purpose of checking whether an import is permitted, and then discourage people from using types in vendored packages in their APIs. I don't see a compelling need for a vendored package to be imported from outside the place where it is vendored.

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Apr 29, 2016

Member

discourage people from using types in vendored packages in their APIs. I don't see a compelling need for a vendored package to be imported from outside the place where it is vendored.

This strikes me as infeasible and undesirable. Packages exist to be used, embraced, extended. I can think of no better demonstration of the "compelling need" of this use case than the etcd and context.Context example.

Member

peterbourgon commented Apr 29, 2016

discourage people from using types in vendored packages in their APIs. I don't see a compelling need for a vendored package to be imported from outside the place where it is vendored.

This strikes me as infeasible and undesirable. Packages exist to be used, embraced, extended. I can think of no better demonstration of the "compelling need" of this use case than the etcd and context.Context example.

@dsymonds

This comment has been minimized.

Show comment
Hide comment
@dsymonds

dsymonds Apr 29, 2016

Member

On the contrary: while packages exist to be used, they don't exist to be copied. Go's type system does not play well with copies of the same package because their exported types are truly distinct types, and vendoring is fundamentally about copying a package. It works fine when that copy is for the private use of a package (or close-knit set of packages), but it's a different story when there is the need to interoperate.

Member

dsymonds commented Apr 29, 2016

On the contrary: while packages exist to be used, they don't exist to be copied. Go's type system does not play well with copies of the same package because their exported types are truly distinct types, and vendoring is fundamentally about copying a package. It works fine when that copy is for the private use of a package (or close-knit set of packages), but it's a different story when there is the need to interoperate.

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Apr 29, 2016

Member

Let's make it concrete. What would you advise the etcd maintainers to do in the above case?

Member

peterbourgon commented Apr 29, 2016

Let's make it concrete. What would you advise the etcd maintainers to do in the above case?

@dsymonds

This comment has been minimized.

Show comment
Hide comment
@dsymonds

dsymonds Apr 29, 2016

Member

I think the etcd maintainers should avoid using vendored types in their external APIs. For Context, they should import the official place for it (currently golang.org/x/net/context) and ditch their own copy. The package is very unlikely to break (so there's less point in vendoring it), but if it does change then callers of the etcd package will be stuck anyway (since they're the ones supplying the Context).

Member

dsymonds commented Apr 29, 2016

I think the etcd maintainers should avoid using vendored types in their external APIs. For Context, they should import the official place for it (currently golang.org/x/net/context) and ditch their own copy. The package is very unlikely to break (so there's less point in vendoring it), but if it does change then callers of the etcd package will be stuck anyway (since they're the ones supplying the Context).

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Apr 30, 2016

Member

If I understand you correctly, you propose the rule be

  • Vendor dependencies which are purely internal, i.e. not exposed in any of your exported functions, methods, or types
  • Don't vendor dependencies which are exposed in your exported functions, methods, or types; use upstream directly

I hope you see that this is totally infeasible — it makes reproducible builds impossible, which is the primary reason authors want vendoring. That context.Context "is very unlikely to break" is hope, and hope is not a strategy.

(edit: Forgive me, this reads as a little aggressive, which I don't really intend.)

Member

peterbourgon commented Apr 30, 2016

If I understand you correctly, you propose the rule be

  • Vendor dependencies which are purely internal, i.e. not exposed in any of your exported functions, methods, or types
  • Don't vendor dependencies which are exposed in your exported functions, methods, or types; use upstream directly

I hope you see that this is totally infeasible — it makes reproducible builds impossible, which is the primary reason authors want vendoring. That context.Context "is very unlikely to break" is hope, and hope is not a strategy.

(edit: Forgive me, this reads as a little aggressive, which I don't really intend.)

@dsymonds

This comment has been minimized.

Show comment
Hide comment
@dsymonds

dsymonds Apr 30, 2016

Member

Reproducible builds only really matter at the binary (main) level. If you're a binary, go ahead and vendor everything, and you have reproducible builds.

But if you're a package, other people are using you, and it is their responsibility to vendor the world if they care about such things. If they are vendoring everything, there's no need for you to vendor things that appear in your API since typically they'll need to be using that package anyway.

In more concrete terms: the etcd package has context.Context in its API. If there's a binary B that imports etcd, it probably also needs to import context (to be able to have a context.Context to pass to etcd). If B is not vendoring things, it then has two context.Contexts (the etc copy and the true upstream); if context.Context changes, it'll be broken because those copies will be out of sync, and etcd's vendoring didn't help at all, but probably only hindered since B will need to either use an old version of the true upstream, or else hand-edit etcd. If B is vendoring things, there was no need for etcd to vendor context.

Member

dsymonds commented Apr 30, 2016

Reproducible builds only really matter at the binary (main) level. If you're a binary, go ahead and vendor everything, and you have reproducible builds.

But if you're a package, other people are using you, and it is their responsibility to vendor the world if they care about such things. If they are vendoring everything, there's no need for you to vendor things that appear in your API since typically they'll need to be using that package anyway.

In more concrete terms: the etcd package has context.Context in its API. If there's a binary B that imports etcd, it probably also needs to import context (to be able to have a context.Context to pass to etcd). If B is not vendoring things, it then has two context.Contexts (the etc copy and the true upstream); if context.Context changes, it'll be broken because those copies will be out of sync, and etcd's vendoring didn't help at all, but probably only hindered since B will need to either use an old version of the true upstream, or else hand-edit etcd. If B is vendoring things, there was no need for etcd to vendor context.

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv May 1, 2016

Member

@dsymonds

Reproducible builds only really matter at the binary (main) level

Whilst I agree that for binaries it makes lots of sense to vendor everything, I question your point about reproducible builds not being relevant for libraries.

Surely for the developers of a library (group 1), having reproducible builds is important, as is ensuring a consistent development experience, i.e. everybody working from the same commit of external dependencies at a given point in time? In this situation, vendoring per the Go 1.5 definition is not the solution; GOPATH augmentation works best*.

For the developers of binaries (group 2) that have various external library dependencies, I agree with you, it's down to them to vendor (Go 1.5 approach) as they wish.

The problem of working out what versions (commits) of external dependencies to use at a given point in time is common to groups 1 and 2. I'm not aware of any tooling that helps answer this question right now (where "answer" means "give me the latest compatible commits of all my external dependencies", where "compatible" could be defined as "go test ./... has a zero exit code")

Dealing with multiple copies of external dependencies in a given repository (imagine a repository that has n binaries and m libraries) is a related but orthogonal problem, and is the subject of this proposal.

* incidentally, I've tried to sketch out what I mean by this approach here

Member

myitcv commented May 1, 2016

@dsymonds

Reproducible builds only really matter at the binary (main) level

Whilst I agree that for binaries it makes lots of sense to vendor everything, I question your point about reproducible builds not being relevant for libraries.

Surely for the developers of a library (group 1), having reproducible builds is important, as is ensuring a consistent development experience, i.e. everybody working from the same commit of external dependencies at a given point in time? In this situation, vendoring per the Go 1.5 definition is not the solution; GOPATH augmentation works best*.

For the developers of binaries (group 2) that have various external library dependencies, I agree with you, it's down to them to vendor (Go 1.5 approach) as they wish.

The problem of working out what versions (commits) of external dependencies to use at a given point in time is common to groups 1 and 2. I'm not aware of any tooling that helps answer this question right now (where "answer" means "give me the latest compatible commits of all my external dependencies", where "compatible" could be defined as "go test ./... has a zero exit code")

Dealing with multiple copies of external dependencies in a given repository (imagine a repository that has n binaries and m libraries) is a related but orthogonal problem, and is the subject of this proposal.

* incidentally, I've tried to sketch out what I mean by this approach here

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 22, 2016

Member

This proposal is on hold until we figure out other vendoring issues.

Member

bradfitz commented Aug 22, 2016

This proposal is on hold until we figure out other vendoring issues.

@adg adg self-assigned this Sep 26, 2016

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Oct 24, 2016

Contributor

This is all going to become irrelevant once we solve other dependency management-related issues. I'm going to decline this. Thanks for the effort @minux. 👍

Contributor

adg commented Oct 24, 2016

This is all going to become irrelevant once we solve other dependency management-related issues. I'm going to decline this. Thanks for the effort @minux. 👍

@adg adg closed this Oct 24, 2016

@ifraixedes ifraixedes referenced this issue in cycloidio/goat Jun 23, 2017

Closed

Change the package architecture #14

@bgrieder bgrieder referenced this issue in ethereum/go-ethereum Jul 16, 2017

Closed

go-ethereum as a library ? #14816

@golang golang locked and limited conversation to collaborators Oct 24, 2017

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