Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency on bitbucket.org/ww/goautoneg is the only mercurial dependency #17

Closed
pwaller opened this issue Oct 26, 2015 · 12 comments
Closed

Comments

@pwaller
Copy link
Contributor

pwaller commented Oct 26, 2015

Hi! In various places, I don't have mercurial available (for example, CoreOS). I've also taken to using git submodules for vendoring. Out of the 7 external packages required to import prometheus/client_golang, the only non-git dependency is bitbucket.org/ww/goautoneg. See https://godoc.org/github.com/prometheus/client_golang/prometheus?import-graph&hide=2.

ww/goautoneg has not had a commit since 2012. It's a very small amount of code: https://bitbucket.org/ww/goautoneg/src/75cd24fc2f2c2a2088577d12123ddee5f54e0675/autoneg.go?at=default&fileviewer=file-view-default

Any chance the source could be imported wholesale, and live where it is needed? (perhaps to /internal/autoneg, for example).


As a side note in the project I'm using this, there are 26 external repositories. All of which live in git repositories and happily live in the git submodule ecosystem. This is the first package in more than a year of development on this project where I've hit a non-git dependency.

@fabxc
Copy link
Contributor

fabxc commented Oct 26, 2015

Thanks, I completely agree.
That has actually been on my backlog for quite a while.

@pwaller
Copy link
Contributor Author

pwaller commented Oct 26, 2015

I can submit a PR if it helps. I have it working already over here.

@fabxc
Copy link
Contributor

fabxc commented Oct 26, 2015

That's perfect!

@pwaller
Copy link
Contributor Author

pwaller commented Oct 26, 2015

Done.

@fabxc fabxc closed this as completed in #18 Oct 26, 2015
@ingvagabund
Copy link

I don't think internal is proper directory for this use case. What about Godeps or vendor? This way the import path is "github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg" instead of "bitbucket.org/ww/goautoneg". Setting GOPATH to Godeps will solve the problem and the change is transparent for expfmt/encode.go.

@beorn7
Copy link
Member

beorn7 commented Mar 7, 2016

The problem is that vendoring within libraries is highly problematic.
No user of the common package needs to bother with the lengthy import path (and in fact, the magic meaning of internal makes sure nobody outside common even can use it).

For as long as vendoring within libraries isn't solved in general, I think we have treated this particular case as well as we can.

@fabxc
Copy link
Contributor

fabxc commented Mar 7, 2016

I wouldn't say highly problematic per se – with Go 1.6 (or 1.5 with vendor experiment) library level vendoring works just fine as long as the package doesn't expose types of the vendored packages.

Internal does not seem the right place anymore once we can be sure that the majority of users (and for common we mostly care about ourselves) uses Go 1.5 or higher.

Not seeing a benefit or restricting usage to a package that's not even ours. This is vendoring.

@aanm
Copy link

aanm commented Jun 8, 2016

I can't build my project which I believe is because of this...

godep version godep v71 (linux/amd64/go1.6) doesn't seems to import the github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg directory to vendor so I got a:

$ go install ./...
....
  imports github.com/prometheus/common/expfmt
  imports github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg: use of internal package not allowed

@beorn7
Copy link
Member

beorn7 commented Jun 9, 2016

@aanm Not sure what's going on there. The whole point is that we have not vendored goautoneg in the conventional way with any vendoring tool. We have simply copied the package into our source tree, and to make sure nobody uses it from outside of our tree, we have put it into internal.

godep in your case seems to try to handle it somehow, which would be a mistake on the side of godep.

@aanm
Copy link

aanm commented Jun 9, 2016

Thanks for the reply. I'm going to create an issue in godeps repo and a new
issue in this repo just to link both of them.
On Jun 9, 2016 9:45 AM, "Björn Rabenstein" notifications@github.com wrote:

@aanm https://github.com/aanm Not sure what's going on there. The whole
point is that we have not vendored goautoneg in the conventional way
with any vendoring tool. We have simply copied the package into our source
tree, and to make sure nobody uses it from outside of our tree, we have put
it into internal.

godep in your case seems to try to handle it somehow, which would be a
mistake on the side of godep.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFcwks8ZZ-bxqGwVDUEXgg9dwLNSFwzgks5qJ9KugaJpZM4GVhNy
.

@mkumatag
Copy link

Even I faced the issue mentioned #17 (comment) any update on this issue.?

@beorn7
Copy link
Member

beorn7 commented Jun 25, 2016

It seems the way is to do internal library vendoring for anything that isn't visible in any of the types exported by the library. We need to be reasonably confident that everybody uses Go1.6 or Go1.5 with the GOVENDOREXPERIMENT15 env variably set to 1.

The above should be done consistently for everything in the library for which the prerequisite is true.
I'll update #45 accordingly.

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

No branches or pull requests

6 participants