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

gazelle vendor support #114

Closed
hochhaus opened this issue Sep 22, 2016 · 20 comments
Closed

gazelle vendor support #114

hochhaus opened this issue Sep 22, 2016 · 20 comments
Labels

Comments

@hochhaus
Copy link
Contributor

Is vendoring automatically supported when using gazelle/new_go_repository()?

As a specific example, when using new_go_repository() with a repository that uses vendoring (eg: https://github.com/rsc/letsencrypt) I see that gazelle creates BUILD files for the vendored repositories.

For example, adding the following to my WORKSPACE:

new_go_repository(
    name = "io_rsc_letsencrypt",
    commit = "a18c646c3d0772313b7b53c78847bb3eb6463f0b",
    importpath = "rsc.io/letsencrypt",
)

generates the following BUILD files:

ahochhaus@ahochhaus-pc:~/ll$ find -L bazel-* -name BUILD | grep letsen
bazel-ll/external/io_rsc_letsencrypt/BUILD
bazel-ll/external/io_rsc_letsencrypt/vendor/github.com/xenolf/lego/acme/BUILD

However when I build, the vendored repository (com_github_xenolf_lego) can't be found.

@pcj
Copy link
Member

pcj commented Sep 22, 2016

(chiming in)

There would have to be a WORKSPACE file written at $(bazel info output_base)/external/com_github_xenolf_lego for this to work. Does that file exist? Otherwise I'd think one would have to refer to it as @io_rsc_letsencrypt//vendor/github.com/xenolf/lego/acme:go_default_library.

@pmbethe09
Copy link
Member

So running gazelle creates deps by using a consistent conversion string
from Go import-path to a workspace-name.
That is true both for manual local gazelle, and gazelle underneath a
new_go_repository.

However in both cases that assumes you have all of the right things in your
WORKSPACE file.

Presumably what we need is either a stand-alone "workzelle", or something
running as part of local invocation of 'gazelle -workspace=fix ...'
This would collect all of the remote-repo deps as referenced by gazelle,
and add them to your WORKSPACE, as well as look at the transitive
dependencies.
Note: this is because bazel require a flattening of all transitive
dependencies into your local WORKSPACE file -- it doesn't read WORKSPACE
files read from any remote repos.

I have started working on such a thing, but want to touch base with @yugui
to plan

On Wed, Sep 21, 2016 at 11:08 PM, Paul Cody Johnston <
notifications@github.com> wrote:

(chiming in)

There would have to be a WORKSPACE file written at $(bazel info
output_base)/external/com_github_xenolf_lego for this to work. Does that
file exist? Otherwise I'd think it one would have to refer to it as
@io_rsc_letsencrypt//vendor/github.com/xenolf/lego/acme:go_default_library
.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#114 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALF-0lNsU8V6Fd9R4bUmRCjnW5mTfv2Nks5qsfFAgaJpZM4KDdMI
.

@yugui
Copy link
Member

yugui commented Sep 23, 2016

I have started working on such a thing, but want to touch base with @yugui to plan

Both of workzelle and -workspace=fix sound good for me. But probably workzelle is simpler.

Also, I just want to share another point in my mind with you -- it might be related to this topic. Some users (e.g. my team) �uses both of new_go_repository rule and vendor/ directory. So they would need a way to customize the mapping from the importpath to label in gazelle.

@hochhaus
Copy link
Contributor Author

hochhaus commented Sep 23, 2016

Thanks for the work on gazelle and all the great feedback!

As best I can tell, none of these proposals match the semantics of the go tool. My initial bug report was not clear about what I am looking for.

The problem is that multiple repositories can all depend on different versions of com_github_xenolf_lego (as that package could be vendored in two or more different versions).

Instead of the proposed behavior, it seems better to me that the vendored copy of com_github_xenolf_lego in io_rsc_letsencrypt should actually be named something like @io_rsc_letsencrypt_vendored_com_github_xenolf_lego. Also the build files generated for io_rsc_letsencrypt should automatically depend on that vendored package.

However, at that point, why can't the dependencies in bazel-ll/external/io_rsc_letsencrypt/BUILD automatically reference bazel-ll/external/io_rsc_letsencrypt/vendor/github.com/xenolf/lego/acme/BUILD hence eliminating the need for an extra WORKSPACE entry for the vendored package all together.

Does this behavior seem reasonable?

@pmbethe09
Copy link
Member

On Fri, Sep 23, 2016 at 2:27 AM, Andy Hochhaus notifications@github.com
wrote:

Thanks for the work on gazelle and all the great feedback!

As best I can tell, none of these proposals match the semantics of the go
tool. My initial bug report was not clear about what I am looking for.

The problem is that multiple repositories can all depend on different
versions of com_github_xenolf_lego (as that package could be vendored at
different versions in two or more libraries).

Instead of the proposed behavior, it seems better to me that the vendored
copy of com_github_xenolf_lego in io_rsc_letsencrypt actually be named
something like @io_rsc_letsencrypt_vendored_com_github_xenolf_lego. Also
the build files generated for io_rsc_letsencrypt should automatically
depend on the vendored package.

So here by vendored, you mean checked into a vendored directory, rather
than pinned to a particular version?

For the remote-vendored diamond dependency problem, bazel wants you to
choose the winner by flattening your workspace.

However, at that point, why can't the dependencies in
bazel-ll/external/io_rsc_letsencrypt/BUILD automatically reference
bazel-ll/external/io_rsc_letsencrypt/vendor/github.com/
xenolf/lego/acme/BUILD hence eliminating the need for an extra WORKSPACE
entry for the vendored package all together.

Some people want that, others prefer a flattened WORKSPACE file that
clearly identifies all direct and indirect dependencies, so that there are
no surprises.

Does this behavior seem reasonable?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#114 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALF-0t8QTOp28r6_1FN3T0pYNrYik9O_ks5qs3FKgaJpZM4KDdMI
.

@hochhaus
Copy link
Contributor Author

hochhaus commented Sep 23, 2016

Thanks.

So here by vendored, you mean checked into a vendored directory, rather than pinned to a particular version?

Yes, for example rsc/letsencrypt/vendor/github.com/xenolf/lego.

For the remote-vendored diamond dependency problem, bazel wants you to
choose the winner by flattening your workspace.

Unfortunately, you can't always "pick a single winner" as the API could have changed between library versions. Since GOVENDOREXPERIMENT became the default in Go 1.6 the standard go build tools assume that multiple different versions of the same library can be compiled into a single binary. I think that if rules_go does not follow a similar convention it will be harder to get general interoperability with the rest of the open source go ecosystem.

In the case of io_rsc_letsencrypt, it really does depend on an outdated API of com_github_xenolf_lego (specifically TLSSNI01ChallengeCertDomain()). Any other use of com_github_xenolf_lego which uses the new API (TLSSNI01ChallengeCert()) in my project (or a library used by my project) will mean that no single version of com_github_xenolf_lego can be entered into the WORKSPACE file.

Some people want that, others prefer a flattened WORKSPACE file that
clearly identifies all direct and indirect dependencies, so that there are
no surprises.

I certainly see value in both models. For example, a downside of using the vendor/ directory model that I propose is that it is less predictable and leads to binary bloat. However, a downside of forcing a single version of each dep to be entered into the WORKSPACE file is that it is less interoperable with the rest of the Go ecosystem and may require code changes or manually written BUILD files.

At a minimum could we make this behavior configurable via an option?

@pcj
Copy link
Member

pcj commented Sep 23, 2016

I agree, the whole point of vendoring is that you can maintain several different versions of the same library in the same WORKSPACE, so it does not seem right to hoist a vendored name into the global at-namespace.

I'm not in love with the _vendored part of @io_rsc_letsencrypt_vendored_com_github_xenolf_lego (name is long enough, isn't it already implied?) but I completely agree with the design.

@hochhaus
Copy link
Contributor Author

I'm not in love with the _vendored part of @io_rsc_letsencrypt_vendored_com_github_xenolf_lego (name is long enough, isn't it already implied?) but I completely agree with the design.

Good point. I'm happy with any name. I was just trying to propose the concept with a concrete example.

@pmbethe09
Copy link
Member

I re-read this and realized that what is really needed for consistency with the Go tool, is gazelle generating dependencies to things that are vendored, rather than to external repos.
So in this case rather than a dep on "@com_github_xenolf_lego//acme:go_default_library" it should be as @pcj wrote above, to "io_rsc_letsencrypt//vendor/github.com/xenolf/lego/acme:go_default_library"

Probably:

  • Add -ignore_vendored flag to gazelle (where false will become default)
    ** if true, assumes all deps are remote
    ** if false reads a //vendor directory
  • Add ignore_vendored attribute to new_go_repository, which passes through to the gazelle invocations
    ** allows users to make WORKSPACE decision to avoid vendoring, but the default of False means "do what 'go tool' would have done"

@mikedanese
Copy link
Contributor

To workaround no support for vendor/ in gazelle, the kubernetes project is using https://github.com/mikedanese/gazel

@pmbethe09
Copy link
Member

pmbethe09 commented Dec 21, 2016 via email

@mikedanese
Copy link
Contributor

mikedanese commented Dec 21, 2016

It manages most of the BUILD files in kubernetes. It produces a single very big BUILD file for vendor https://github.com/kubernetes/kubernetes/blob/master/vendor/BUILD. We don't use go_library remote repository rules yet and may never. As far as I could tell, gazelle requires this for libraries outside of your go_prefix. It also allows for partially managed BUILD files. With gazelle it's all or none. There may be other differences as well.

@pcj
Copy link
Member

pcj commented Dec 21, 2016

@mikedanese, can you add a README to your repository that shows how to use it?

@mikedanese
Copy link
Contributor

@pcj added a README

@pcj
Copy link
Member

pcj commented Dec 21, 2016

@mikedanese Nit: will render better as README.md

@twpayne
Copy link
Contributor

twpayne commented Jan 10, 2017

#227 contains a partial fix, I think.

@kerinin
Copy link
Contributor

kerinin commented Jan 10, 2017

I think what's missing after #227 is the ability to generate BUILD files in vendor with the correct go prefix

@mikedanese
Copy link
Contributor

Preferably a single BUILD file in the root of vendor/

@MarkusTeufelberger
Copy link

This would be pattern 2 in #16 (comment), right?

As far as i understand it, gazelle only implements patterns 1 and 3 so far, while the gazel tool used in Kubernetes implements pattern 2. It would be nice if gazelle also supported that pattern.

@jayconrod
Copy link
Contributor

Close old Gazelle issues. If Gazelle isn't doing something you want, please open a new issue at https://github.com/bazelbuild/bazel-gazelle.

This should be working now. Gazelle will resolve dependencies against existing libraries in vendor in either external or vendored mode.

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

No branches or pull requests

9 participants