Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Exclusion patterns proposal #54

Open
karalabe opened this issue Oct 26, 2016 · 3 comments
Open

Exclusion patterns proposal #54

karalabe opened this issue Oct 26, 2016 · 3 comments

Comments

@karalabe
Copy link
Contributor

While working with trash we noticed an interesting corner case that the concept of vendoring introduces but that trash cannot handle currently:

As vendoring essentially creates new import paths (that the compiler resolves internally, but new nonetheless), vendored types are different from the same types directly imported from the outside. This means that if I create a library, the API must never contain vendored types, as those are private to the library (similar to internal).

Now this in general is just good practice, that you don't expose internal stuff. There's one common catch though, the golang.org/x/net/context package. Go APIs in general always use this package when a context needs to be passed in, but vendoring it will make it impossible to create (since the API's real type will be mypackage/vendor/golang.org/x/net/context). The standard solution is to never vendor in golang.org/x/net/context, as it won't ever change in the foreseeable future.

Now you might say that Go 1.7+ standardized this context so there's no need for the external package any more, but while that's true, using it would break all builds prior to Go 1.7, which is a bit too much given that 1.7 is the newest. Maybe we can do that in a couple versions after 1.9 appears.

Given all this, the hackish solution would be to hard code that golang.org/x/net/context is always deleted... but that's ugly. A better solution that I'd propose is to add a second configuration group into the yaml file called blacklist (or anything really), which contains a list of packages that are always deleted during the cleanup phase.

If you think this would be a use case you'd like to have, I'd gladly code it up. Just want to get the green light first before putting any effort into it. Also if you have any particular constraints or wishes regarding it, I'd gladly listen to them :)

@imikushin
Copy link
Contributor

We've never hit the problem you're describing (probably because we always use trash to vendor dependencies), but I can see where it comes from.

Directly using a library that has vendored code and using any of that library's vendored packages directly is calling for trouble.

There are a couple workarounds:

  1. Use trash to vendor the library packages (that themselves have vendored code) into your app: it will strip all vendored code from the packages it vendors (during the cleanup phase), so the problem doesn't exist anymore.
  2. Don't vendor stuff into a library package: it's just too easy to leak dependency types in the API. E.g. ignore ./vendor, but check in vendor.conf This option might not be too bad

In any case, if you use trash as your app's vendoring tool, you have to add all (including transitive) dependencies to your app's vendor.conf.

That said, I'm not decided on this proposal, yet (a real life example would help). For now, it's more "no" than "yes".

@karalabe
Copy link
Contributor Author

The real life example:

We're the authors of go-ethereum, which currently stands at around 135K lines, with stripped down vendored libraries at 390K lines. That is a huge amount of dependencies.

Since we don't control end user applications, only our codebase, we can't just tell people to use trash themselves too. That is the main feature of vendor that you can have reliable codebases without relying on any other tool than go or the main repository than the one you check out. So this requirement rules out both proposed workarounds, as both break the requirement that no user side tool should be used.

My end goal is to allow go-ethereum to be go get-able and fully self contained. Exposing vendored types in an API is a known no-go, but exposing golang.org/x/net/context is an exception to that, where the recommended rules are to leave that one package out of vendoring. Hence why exclusion patterns are needed :)

@imikushin
Copy link
Contributor

@karalabe The code is in. Cutting a new release soon!

Thanks for sharing the use case: a concrete example really helps to look at things in perspective.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants