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

Consider using go's native vendoring mechanism #128

Closed
s-urbaniak opened this Issue Jul 6, 2016 · 24 comments

Comments

Projects
None yet
7 participants
@s-urbaniak

s-urbaniak commented Jul 6, 2016

Hello,

We recognized that starting from that commit:

f50fc7d#diff-21ce9dcd10d134f8b0f327ac1ee4aa4dR24

ql started vendoring the go4.org/lock library as well as the github.com/cznic/exp/lldb library in a way that does not play well with other vendoring tools, i.e. glide.

Can you elaborate on the reasoning why you chose "vendored" instead of the go "vendor" directory? I can personally think of many, but I have the impression, that vendor pruning in consuming applications becomes more ubiquitous as vendor tool support improves.

xref rkt/rkt#2875

@calmh

This comment has been minimized.

Show comment
Hide comment
@calmh

calmh Jul 6, 2016

This being an importable library, it should not vendor at all as far as I understand?

calmh commented Jul 6, 2016

This being an importable library, it should not vendor at all as far as I understand?

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 6, 2016

Right, if ql envisions to be a library-only it should strongly consider not to vendor at all.

s-urbaniak commented Jul 6, 2016

Right, if ql envisions to be a library-only it should strongly consider not to vendor at all.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jul 6, 2016

Owner

I have never used any vendoring mechanism, which means I have never had any reason to read anything about how it works. When I was asked to vendor some packages with ql, I used "vendored" in hope to not interfere with any vendoring mechanism I have no knowledge about.

I will appreciate if you can please explain to me in more detail what

does not play well with other vendoring tools, i.e. glide.

means. Thank you.

Right, if ql envisions to be a library-only it should strongly consider not to vendor at all.

That is my opinion as well, but then the Debian guy (hi @onlyjob) asked for, first making releases (#121) and, later, for the vendoring (#122). Note that #121 mentions rkt as well, so I guess you guys might want to coordinate ;-)

Owner

cznic commented Jul 6, 2016

I have never used any vendoring mechanism, which means I have never had any reason to read anything about how it works. When I was asked to vendor some packages with ql, I used "vendored" in hope to not interfere with any vendoring mechanism I have no knowledge about.

I will appreciate if you can please explain to me in more detail what

does not play well with other vendoring tools, i.e. glide.

means. Thank you.

Right, if ql envisions to be a library-only it should strongly consider not to vendor at all.

That is my opinion as well, but then the Debian guy (hi @onlyjob) asked for, first making releases (#121) and, later, for the vendoring (#122). Note that #121 mentions rkt as well, so I guess you guys might want to coordinate ;-)

@calmh

This comment has been minimized.

Show comment
Hide comment
@calmh

calmh Jul 6, 2016

Debian usually removes any vendoring in things it packages, so i have no idea what that is about.

calmh commented Jul 6, 2016

Debian usually removes any vendoring in things it packages, so i have no idea what that is about.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 6, 2016

Hey @cznic, thanks a lot for the quick response :-) Unfortunately the vendoring situation in the Go ecosystem is not in the best shape right now, I agree :-(

What I meant with:

does not play well with other vendoring tools, i.e. glide.

Tools like https://github.com/Masterminds/glide actually are aware of nested vendored dependencies. So if you import lock and lldb, these tools will "flatten" nested vendored dependencies from:

prj - vendor - ql - vendor - lldb
                          \- lock

to

prj - vendor - ql
            \- lock
            \- lldb

Since in the case of "lock" and "lldb" you put those dependencies in a custom "vendored" folder, consumers of "ql" will automatically get these, which on first eye-sight is great 👍

But if the sample prj project also uses the lock library for other purposes other than ql, it must vendor it also. But here is the problem: It will vendor lock directly and transitively from your project, hence it will be vendored twice.

My suggestion in the case of ql is to use a tool like https://github.com/Masterminds/glide to pin the used external libraries, but not check in the vendor directory. This mode of operation is supported by glide as far as I understand.

If help is needed I am more than willing to prepare a PR :-)

s-urbaniak commented Jul 6, 2016

Hey @cznic, thanks a lot for the quick response :-) Unfortunately the vendoring situation in the Go ecosystem is not in the best shape right now, I agree :-(

What I meant with:

does not play well with other vendoring tools, i.e. glide.

Tools like https://github.com/Masterminds/glide actually are aware of nested vendored dependencies. So if you import lock and lldb, these tools will "flatten" nested vendored dependencies from:

prj - vendor - ql - vendor - lldb
                          \- lock

to

prj - vendor - ql
            \- lock
            \- lldb

Since in the case of "lock" and "lldb" you put those dependencies in a custom "vendored" folder, consumers of "ql" will automatically get these, which on first eye-sight is great 👍

But if the sample prj project also uses the lock library for other purposes other than ql, it must vendor it also. But here is the problem: It will vendor lock directly and transitively from your project, hence it will be vendored twice.

My suggestion in the case of ql is to use a tool like https://github.com/Masterminds/glide to pin the used external libraries, but not check in the vendor directory. This mode of operation is supported by glide as far as I understand.

If help is needed I am more than willing to prepare a PR :-)

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jul 6, 2016

Owner

@s-urbaniak Thank you for the explanation, appreciated. I even think I understood it ;-)

First two things which jumped to my mind are

  • Need to know @onlyjob 's opinion first as I don't want to [possibly] break what was made for his earlier request (#122) without him approving such change.
  • Is there any [good] option to, if at all using vendoring, use the mechanism supported directly be the Go build system, without dependencies on 3rd party tools (glide)?

If help is needed I am more than willing to prepare a PR :-)

I will try to use that offer as much as I can B-)

But we must first find a consensus on the next steps.

Owner

cznic commented Jul 6, 2016

@s-urbaniak Thank you for the explanation, appreciated. I even think I understood it ;-)

First two things which jumped to my mind are

  • Need to know @onlyjob 's opinion first as I don't want to [possibly] break what was made for his earlier request (#122) without him approving such change.
  • Is there any [good] option to, if at all using vendoring, use the mechanism supported directly be the Go build system, without dependencies on 3rd party tools (glide)?

If help is needed I am more than willing to prepare a PR :-)

I will try to use that offer as much as I can B-)

But we must first find a consensus on the next steps.

@calmh

This comment has been minimized.

Show comment
Hide comment
@calmh

calmh Jul 6, 2016

Go vendoring should not be done by libraries (I.e., non-main packages), at all, ever. If there's a specific version of a dependency that this package requires I think the only solution today is to state that in the readme and let the consuming project vendor as appropriate.

calmh commented Jul 6, 2016

Go vendoring should not be done by libraries (I.e., non-main packages), at all, ever. If there's a specific version of a dependency that this package requires I think the only solution today is to state that in the readme and let the consuming project vendor as appropriate.

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab Jul 6, 2016

Go vendoring should not be done by libraries (I.e., non-main packages), at all, ever. If there's a specific version of a dependency that this package requires I think the only solution today is to state that in the readme and let the consuming project vendor as appropriate.

That's what @s-urbaniak proposal's would achieve. Having a glide manifest but no /vendor dir will mean that consumer apps will be pinned to the version specified here, but no actual code would be vendored in this repo. That would be automatic and tool-supported, as opposed to having this human-only info in the README.

lucab commented Jul 6, 2016

Go vendoring should not be done by libraries (I.e., non-main packages), at all, ever. If there's a specific version of a dependency that this package requires I think the only solution today is to state that in the readme and let the consuming project vendor as appropriate.

That's what @s-urbaniak proposal's would achieve. Having a glide manifest but no /vendor dir will mean that consumer apps will be pinned to the version specified here, but no actual code would be vendored in this repo. That would be automatic and tool-supported, as opposed to having this human-only info in the README.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 6, 2016

@calmh yes, that is exactly the consensus in Go currently, hence my proposal to leave only metadata about dependencies checked in.

s-urbaniak commented Jul 6, 2016

@calmh yes, that is exactly the consensus in Go currently, hence my proposal to leave only metadata about dependencies checked in.

@onlyjob

This comment has been minimized.

Show comment
Hide comment
@onlyjob

onlyjob Jul 7, 2016

Go native vendoring could be applied to everything but github.com/camlistore/go4/lock due to problems described in #122. Unfortunately go4/lock is a part of large and messy code repository with many dependencies. ql uses small portion of go4 and the latter is unstable to package separately. Moreover they strongly encourage vendoring and for re-distribution it is useful when go4/lock is vendored manually without using native vendoring functionality. It is a practical concern -- I hope #122 provides some insights to why it is necessary...

onlyjob commented Jul 7, 2016

Go native vendoring could be applied to everything but github.com/camlistore/go4/lock due to problems described in #122. Unfortunately go4/lock is a part of large and messy code repository with many dependencies. ql uses small portion of go4 and the latter is unstable to package separately. Moreover they strongly encourage vendoring and for re-distribution it is useful when go4/lock is vendored manually without using native vendoring functionality. It is a practical concern -- I hope #122 provides some insights to why it is necessary...

@calmh

This comment has been minimized.

Show comment
Hide comment
@calmh

calmh Jul 7, 2016

That's a small package, I'd probably just copy it out as github.com/cznic/ql/internal/lock and be done with it.

calmh commented Jul 7, 2016

That's a small package, I'd probably just copy it out as github.com/cznic/ql/internal/lock and be done with it.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 7, 2016

@calmh That's indeed a good idea, but also lldb is vendored that way, I have some idea around that and will come up with a suggestion PR.

s-urbaniak commented Jul 7, 2016

@calmh That's indeed a good idea, but also lldb is vendored that way, I have some idea around that and will come up with a suggestion PR.

@onlyjob

This comment has been minimized.

Show comment
Hide comment
@onlyjob

onlyjob Jul 7, 2016

Sorry, @calmh, what do you mean "copy it out"? And why?
Please keep it as it is if you could. How is copying it out should help? We shouldn't ship those libs separately...

onlyjob commented Jul 7, 2016

Sorry, @calmh, what do you mean "copy it out"? And why?
Please keep it as it is if you could. How is copying it out should help? We shouldn't ship those libs separately...

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jul 7, 2016

@onlyjob "Copying out" means manually copying the source files into this repository, so that it ceases to be a dependency. A good strategy when the dep isn't large; a little copying is better than a little dependency.

peterbourgon commented Jul 7, 2016

@onlyjob "Copying out" means manually copying the source files into this repository, so that it ceases to be a dependency. A good strategy when the dep isn't large; a little copying is better than a little dependency.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 7, 2016

As @peterbourgon found github.com/cznic/ql#Options let's github.com/cznic/ql/vendored/github.com/cznic/exp/lldb#OSFile escape, hence if a consumer pulls in github.com/cznic/ql, this will be of a different type than github.com/cznic/exp/lldb#OSFile.

I therefore suggest not to put lldb in the "vendored" directory, but rather point to it in the readme (, and additionally machine-readable via glide metadata), additionally hide "flock" as an internal package, so consumers are not confused if they vendor it themselves.

I am happy to help out with a PR.

/cc @cznic @onlyjob

s-urbaniak commented Jul 7, 2016

As @peterbourgon found github.com/cznic/ql#Options let's github.com/cznic/ql/vendored/github.com/cznic/exp/lldb#OSFile escape, hence if a consumer pulls in github.com/cznic/ql, this will be of a different type than github.com/cznic/exp/lldb#OSFile.

I therefore suggest not to put lldb in the "vendored" directory, but rather point to it in the readme (, and additionally machine-readable via glide metadata), additionally hide "flock" as an internal package, so consumers are not confused if they vendor it themselves.

I am happy to help out with a PR.

/cc @cznic @onlyjob

@onlyjob

This comment has been minimized.

Show comment
Hide comment
@onlyjob

onlyjob Jul 7, 2016

I think I would understand term "copying in" but "copying out" in regards to "copying the source files into this repository" is confusing... :)

At the moment those deps are "sub-vendored" i.e. copied in, and imported from "vendored" directory.
What's wrong with that? Is there are any known collisions between "vendored" directory and Golang vendoring tools? I do not understand what problem you are trying to solve here...

onlyjob commented Jul 7, 2016

I think I would understand term "copying in" but "copying out" in regards to "copying the source files into this repository" is confusing... :)

At the moment those deps are "sub-vendored" i.e. copied in, and imported from "vendored" directory.
What's wrong with that? Is there are any known collisions between "vendored" directory and Golang vendoring tools? I do not understand what problem you are trying to solve here...

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 7, 2016

@onlyjob The problem is (as explained above) that the vendored github.com/cznic/ql/vendored/github.com/cznic/exp/lldb#OSFile interface escapes the public API of ql, hence consumers won't be able to use github.com/cznic/exp/lldb#OSFile, since they are of different type, and no type coercion is possible.

Also see the https://github.com/peterbourgon/wtf repository for a distilled version of the problem.

s-urbaniak commented Jul 7, 2016

@onlyjob The problem is (as explained above) that the vendored github.com/cznic/ql/vendored/github.com/cznic/exp/lldb#OSFile interface escapes the public API of ql, hence consumers won't be able to use github.com/cznic/exp/lldb#OSFile, since they are of different type, and no type coercion is possible.

Also see the https://github.com/peterbourgon/wtf repository for a distilled version of the problem.

@fabxc

This comment has been minimized.

Show comment
Hide comment
@fabxc

fabxc Jul 7, 2016

One of the types exposed by the API of ql returns a type from the vendored directory. Hence, one has to import the vendored package as a user of ql. This is no sane practice.

Libraries should not vendor and more importantly not expose customly vendored types.

fabxc commented Jul 7, 2016

One of the types exposed by the API of ql returns a type from the vendored directory. Hence, one has to import the vendored package as a user of ql. This is no sane practice.

Libraries should not vendor and more importantly not expose customly vendored types.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jul 7, 2016

Owner

I think I understand all of the problem(s) discussed above. My decision is to not take sides on this and to do whatever all of you guys agree to do (or not to do). I hope there's no hurry, so please discuss and please find a consensus, whatever it will be. Let me explicitly say that it means I don't want to break the #122 changes without @onlyjob feeling comfortable with it, regardless of what I now think about that change.

Meanwhile, it seems to me that adding glide metadata would not break anything #122 brought, so I think this possible partial change may have a consensus already. Is that the case?

Owner

cznic commented Jul 7, 2016

I think I understand all of the problem(s) discussed above. My decision is to not take sides on this and to do whatever all of you guys agree to do (or not to do). I hope there's no hurry, so please discuss and please find a consensus, whatever it will be. Let me explicitly say that it means I don't want to break the #122 changes without @onlyjob feeling comfortable with it, regardless of what I now think about that change.

Meanwhile, it seems to me that adding glide metadata would not break anything #122 brought, so I think this possible partial change may have a consensus already. Is that the case?

@onlyjob

This comment has been minimized.

Show comment
Hide comment
@onlyjob

onlyjob Jul 8, 2016

Problem originates from special treatment of "vendor" directory by Golang, right? Is "vendored" directory affected as well? If so then renaming it to "external" (or something similar) should address the problem.
I had a look at "wtf" repository and I think it is a typical example of broken/incorrect "inside-out" approach that is so typical to Golang. No, it is not binary's responsibility to "vendor" its libraries dependencies. Libraries can have their own independent dependencies that doesn't have to be exposed to application. In this particular case ql uses its own private 3rd party libraries and we should be fine as long as ql (re-) distribute 3rd party code within its source tree without relying on Golang vendoring functionality. I hope that makes sense.

onlyjob commented Jul 8, 2016

Problem originates from special treatment of "vendor" directory by Golang, right? Is "vendored" directory affected as well? If so then renaming it to "external" (or something similar) should address the problem.
I had a look at "wtf" repository and I think it is a typical example of broken/incorrect "inside-out" approach that is so typical to Golang. No, it is not binary's responsibility to "vendor" its libraries dependencies. Libraries can have their own independent dependencies that doesn't have to be exposed to application. In this particular case ql uses its own private 3rd party libraries and we should be fine as long as ql (re-) distribute 3rd party code within its source tree without relying on Golang vendoring functionality. I hope that makes sense.

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak Jul 8, 2016

@onlyjob Hey, thanks for your comments.

Unfortunately in the case of ql your assesment only applies to the vendored lock library, but not to lldb. As I explained above, OSFile leaks, hence it is not a private library used internally. The problem still remains in this case. As for lock, yes, this could be left internally "vendored", as long as no types leak in the ql API.

s-urbaniak commented Jul 8, 2016

@onlyjob Hey, thanks for your comments.

Unfortunately in the case of ql your assesment only applies to the vendored lock library, but not to lldb. As I explained above, OSFile leaks, hence it is not a private library used internally. The problem still remains in this case. As for lock, yes, this could be left internally "vendored", as long as no types leak in the ql API.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jul 8, 2016

Owner

When "vedoring" lldb, I didn't realize the leaking problem. It was an oversight.

@onlyjob , do you see a chance for correcting this mistake while keeping the Debian package up to standards? If necessary, cznic/exp can be killed (it was always marked as experimental/unstable) and its packages can become standalone with a stable status. It would mean taking care of one more package for the maintainer, probably yourself.

Owner

cznic commented Jul 8, 2016

When "vedoring" lldb, I didn't realize the leaking problem. It was an oversight.

@onlyjob , do you see a chance for correcting this mistake while keeping the Debian package up to standards? If necessary, cznic/exp can be killed (it was always marked as experimental/unstable) and its packages can become standalone with a stable status. It would mean taking care of one more package for the maintainer, probably yourself.

@onlyjob

This comment has been minimized.

Show comment
Hide comment
@onlyjob

onlyjob Jul 9, 2016

Due to my illiteracy in Goland I don't understand the problem with sub-vendoring "leaky" libraries but I'll take care of "lldb" packaging if it will be split from "cznic/exp" into its own repository. @cznic, thank you very much for your careful and reasonable approach. :)

onlyjob commented Jul 9, 2016

Due to my illiteracy in Goland I don't understand the problem with sub-vendoring "leaky" libraries but I'll take care of "lldb" packaging if it will be split from "cznic/exp" into its own repository. @cznic, thank you very much for your careful and reasonable approach. :)

@s-urbaniak

This comment has been minimized.

Show comment
Hide comment
@s-urbaniak

s-urbaniak commented Jul 9, 2016

@onlyjob @cznic thanks!

cznic added a commit to cznic/lldb that referenced this issue Jul 11, 2016

cznic added a commit to cznic/kv that referenced this issue Jul 11, 2016

@cznic cznic closed this in a9f03fa Jul 11, 2016

cznic added a commit to cznic/exp that referenced this issue Jul 11, 2016

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