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

Consider removing bind() #1952

Closed
jart opened this issue Oct 16, 2016 · 58 comments
Closed

Consider removing bind() #1952

jart opened this issue Oct 16, 2016 · 58 comments

Comments

@jart
Copy link
Contributor

@jart jart commented Oct 16, 2016

What is the use case for bind()? I can't think of one. Even in situations where there are multiple ABI-compatible implementations of a library (e.g. OpenSSL, BoringSSL, etc.) this problem could still be solved by using vanilla externals.

Most Bazel projects don't seem to use bind(). The ones that do, it seems to have caused problems.

For example, the protobuf repository, rather than defining a protobuf_repositories() function, simply uses //external:foo for every single target upon which it depends, thereby punting the burden defining bind() rules not only for every single external, but every target within those externals.

As a result, the TensorFlow workspace.bzl file has developed a cargo cult pattern where superfluous bindings will be added, because I don't think people really understand what bind() does.

What is especially suboptimal is that the bind() namespace overlaps with the external repository namespace. We can't name externals like "six" to be "@six" because the protobuf BUILD file asked us for //external:six. So we don't have a choice. We have to name it @six_archive, which hurts readability. It would have been more optimal if the protobuf BUILD file should have just asked for @six//:six.

It would be nice if we could retire bind() and help projects like protobuf migrate to the foo_repositories() model that official Bazel projects use. We could recommend as a best practice the technique that is employed by the Closure Rules repositories.bzl file.

def closure_repositories(
    omit_foo=False,
    omit_bar=False):
  if not omit_foo:
    foo()
  if not omit_bar:
    bar()

def foo():
  native.maven_jar(name = "foo", ...)

def bar():
  native.maven_jar(name = "bar", ...)

This gives dependent Bazel projects the power to schlep in transitive Closure Rules dependencies using either a whitelist or blacklist model. For example, one project that uses Closure Rules has the following in its WORKSPACE file:

http_archive(
    name = "io_bazel_rules_closure",
    sha256 = "7d75688c63ac09a55ca092a76c12f8d1e9ee8e7a890f3be6594a4e7d714f0e8a",
    strip_prefix = "rules_closure-b8841276e73ca677c139802f1168aaad9791dec0",
    url = "http://bazel-mirror.storage.googleapis.com/github.com/bazelbuild/rules_closure/archive/b8841276e73ca677c139802f1168aaad9791dec0.tar.gz",  # 2016-10-02
)

load("@io_bazel_rules_closure//closure:defs.bzl", "closure_repositories")

closure_repositories(
    omit_gson = True,
    omit_guava = True,
    omit_icu4j = True,
    omit_json = True,
    omit_jsr305 = True,
    omit_jsr330_inject = True,
)

Because it directly depends on those transitive dependencies and wants to specify them on its own.

I think this is a much more desirable and flexible pattern than bind().

@philwo
Copy link
Member

@philwo philwo commented Oct 17, 2016

I personally also find bind(...) confusing and am not sure how to correctly use it.

Pinging @damienmg and @lberki for some input here, I think they know more about how this is supposed to work.

@lberki
Copy link
Contributor

@lberki lberki commented Oct 17, 2016

bind was indeed invented for selecting between e.g. different implementations of SSL or e.g. for different versions of GSON/Guava/... in the Closure case. I'm not sure how much use it sees, so I'd not be that trigger-happy with removing it.

@damienmg
Copy link
Contributor

@damienmg damienmg commented Oct 17, 2016

I think all bind uses-cases can be replaced with alias but it does not strike me as high priority. bind() has that weird thing that it creates a //external package that does not really exists, so only for that I would say +1

@jart
Copy link
Contributor Author

@jart jart commented Oct 17, 2016

It might be worth putting a quick change in the documentation saying that bind() is or will likely be deprecated and that alias should be used instead, or simply vanilla external repository names. That should hopefully lessen any refactoring we'll have to do in the future, if it is removed.

@lberki
Copy link
Contributor

@lberki lberki commented Oct 18, 2016

Considering that I'm not really sure we want to do that, I'd rather not deprecate bind() just now.

@jart
Copy link
Contributor Author

@jart jart commented Oct 18, 2016

Then would Bazel at the very least be open to a documentation change
dissuading people from using it?

On Oct 18, 2016 1:45 AM, "lberki" notifications@github.com wrote:

Considering that I'm not really sure we want to do that, I'd rather not
deprecate bind() just now.


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

@robertcrowe-zz
Copy link

@robertcrowe-zz robertcrowe-zz commented Oct 18, 2016

It looks like this could be related to a problem I'm trying to solve. When I try to build my Tensorflow Serving client I'm getting this:

bazel build //FFv1:client
ERROR: /home/rcrowe/.cache/bazel/_bazel_rcrowe/b1867dd6bfd6249a885c91482eccde46/external/org_tensorflow/tensorflow/workspace.bzl:80:3: no such package '@six_archive//': In new_http_archive rule //external:six_archive the 'build_file' attribute does not specify an existing file (/home/rcrowe/serving/six.BUILD does not exist) and referenced by '//external:six'.
ERROR: Analysis of target '//FFv1:client' failed; build aborted.
INFO: Elapsed time: 1.286s

Any idea how I can fix this? I'm on Ubuntu 14.04

@jart
Copy link
Contributor Author

@jart jart commented Oct 18, 2016

@robertcrowe Did you modify the workspace.bzl file? Can you start a separate issue for this and CC me?

@robertcrowe-zz
Copy link

@robertcrowe-zz robertcrowe-zz commented Oct 18, 2016

@jart - Thanks, I created #1963

@kchodorow
Copy link
Contributor

@kchodorow kchodorow commented Oct 19, 2016

+1 to removing bind, updating the docs in the meantime seems like a good idea.

@jart
Copy link
Contributor Author

@jart jart commented Oct 19, 2016

I reviewed the code to rules_web yesterday, which was recently introduced and makes extensive use of bind(). It uses bind() to allow the user to override executable attributes on its Skylark rules without having to repeat them every time the rule is used. I had a discussion with @DrMarcII about this. We both came to the conclusion that it would be better if those attributes were public and the user defined macro wrappers that customize the attribute.

I'm glad we're building consensus around removing bind(). Right now it's the first rule listed in the documentation, but it's actually the last rule we'd want to use. The same is true for git_repository(), which is also listed first, but has the biggest negative impact on performance for the project in question, and all dependent projects. Many users make use of these rules without considering the alternatives, like grabbing the snapshot tarball from GitHub. Whatever we can do to help the user make the correct choices, especially if we make the wrong choices impossible, is going to go a long way to fostering a healthy and lightning fast build ecosystem spanning many GitHub projects that all reference each other.

@damienmg
Copy link
Contributor

@damienmg damienmg commented Oct 20, 2016

The order of the rules are a bit random, I don't think they matter.

Anyway, IMO:

  1. we should advertise bind as deprecated and point user to alias or use
    the repository name instead,
  2. we should starts advertising maven_jar and git_repository as
    deprecated and point the user to http_* rules (for source distribution) and
    the skylark implementations (for those who git / maven support is better).
  3. We should publish a best practice document about the good use of
    external repositories. A blog post maybe?

On Wed, Oct 19, 2016 at 8:36 PM Justine Tunney notifications@github.com
wrote:

I reviewed the code to rules_web https://github.com/bazelbuild/rules_web
yesterday, which was recently introduced and makes extensive use of bind().
It uses bind() to allow the user to override executable attributes on its
Skylark rules
https://github.com/bazelbuild/rules_web/blob/master/web/internal/web_test_config.bzl#L63
without having to repeat them every time the rule is used. I had a
discussion with @DrMarcII https://github.com/DrMarcII about this. We
both came to the conclusion that it would be better if those attributes
were public and the user defined macro wrappers that customize the
attribute.

I'm glad we're building consensus around removing bind(). Right now it's
the first rule listed in the documentation, but it's actually the last rule
we'd want to use. The same is true for git_repository(), which is also
listed first, but has the biggest negative impact on performance for the
project in question, and all dependent projects. Many users make use of
these rules without considering the alternatives, like grabbing the
snapshot tarball from GitHub. Whatever we can do to help the user make the
correct choices, especially if we make the wrong choices impossible, is
going to go a long way to fostering a healthy and lightning fast build
ecosystem spanning many GitHub projects that all reference each other.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1952 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADjHfzgrJ6tRZ8DwEjXfpJr2Yu-g31dhks5q1mMugaJpZM4KYF52
.

@steven-johnson
Copy link
Contributor

@steven-johnson steven-johnson commented Oct 25, 2016

On Thu, Oct 20, 2016 at 12:54 AM, Damien Martin-Guillerez <
notifications@github.com> wrote:

  1. We should publish a best practice document about the good use of
    external repositories.

+1

A blog post maybe?

Sure, but please also mirror it into the official docs somehow; people
arriving later may never see the blog post.

@ittaiz
Copy link
Member

@ittaiz ittaiz commented Nov 1, 2016

@damienmg why are maven_jar and git_repository being deprecated?
additionally the whole "new" prefix is a bit misleading IMHO since in this doc I got the impression that both local_repository and http_archive have a different use-case than the new_local_repository and new_http_archive and not just replacing them.

@damienmg
Copy link
Contributor

@damienmg damienmg commented Nov 2, 2016

They are being replaced with skylark implementation from
@bazel_tools//tools/build_defs/repo:git.bzl and
@bazel_tools//tools/build_defs/repo:maven_rules.bzl

new_* is indeed not the replacement version

On Tue, Nov 1, 2016 at 11:02 AM Ittai Zeidman notifications@github.com
wrote:

@damienmg https://github.com/damienmg why are maven_jar and
git_repository being deprecated?
additionally the whole "new" prefix is a bit misleading IMHO since in this
doc
https://www.bazel.io/versions/master/docs/external.html#depending-on-non-bazel-projects
I got the impression that both local_repository and http_archive have a
different use-case than the new_local_repository and new_http_archive and
not just replacing them.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1952 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADjHf54T56FF5IWlWzFgjgi52ekMGonhks5q5w4ogaJpZM4KYF52
.

@aj-michael
Copy link
Contributor

@aj-michael aj-michael commented Dec 13, 2016

I don't see it mentioned is this thread, so I thought I should mention, the AndroidSdkRepositoryFunction makes use of bind so that android_binary can depend on the android_sdk generated by AndroidSdkRepositoryFunction without needing to know the name of the android_sdk_repository rule.

@kchodorow
Copy link
Contributor

@kchodorow kchodorow commented Dec 13, 2016

Which I think is a mistake, as I think I'm mentioned before. The android_sdk rule still depends on the external name, which is no better that depending on the repository name.

@aj-michael
Copy link
Contributor

@aj-michael aj-michael commented Dec 13, 2016

Sorry, what do you mean by "the android_sdk rule still depends on the external name"? Do you mean that the name of the android_sdk rule is the name of the android_sdk_repository rule? Or that the "bound" name refers to the external name?

@kchodorow
Copy link
Contributor

@kchodorow kchodorow commented Dec 13, 2016

Whoops, I mistyped, I meant the android_binary rule. The android_binary rule depends on something like //external:android_sdk, it would be better for it to depend on @android_sdk//jar or something.

@aj-michael
Copy link
Contributor

@aj-michael aj-michael commented Dec 13, 2016

Hmmm, why would that be better? that would require that every developer name their android_sdk_repository "android_sdk". The current advantage of using //external:android/sdk is that android_binary still works no matter what you name your android_sdk_repository.

I agree with you that android_binary depending on an external bind is not ideal. Maybe this is not the right place to discuss this, but could we remove the name attribute of android_sdk_repository? If we could hardcode the name of android_sdk_repository to something, then we could stop using bind in Android land.

@kchodorow
Copy link
Contributor

@kchodorow kchodorow commented Dec 13, 2016

You are already requiring every developer to declare an android_sdk_repository and bind it to a certain name: this eliminates one of those steps.

Also, you could make it a macro and set the name in the macro, e.g.,

def my_android_sdk():
  native.android_sdk(
    name = 'android_sdk',
    ...
  )
@aj-michael
Copy link
Contributor

@aj-michael aj-michael commented Dec 13, 2016

I don't think we are. The developer does not have to use a bind() in their WORKSPACE. We create the bind under the hood:

.

E.g., the following works:

$ cat WORKSPACE
android_sdk_repository(
    name = "foobar",
    path = "/home/ajmichael/Sdk",
    api_level = 25,
    build_tools_version = "25.0.0",
)
$ cat BUILD
android_binary(
    name = "app",
    manifest = "AndroidManifest.xml",
    custom_package = "com.example"
    srcs = glob(["**.java"]),
)
$ bazel build //:app

I guess if we remove the user-visible bind() function but keep the underlying functionality for native repository rules, android would be fine.

I'm going to open another issue to address your comment about the macro, because I like that idea a lot. 😄

@johnynek
Copy link
Member

@johnynek johnynek commented Jan 12, 2017

I'm -1 on removing or deprecating bind. Suppose you have two external bazel repository dependencies, A, B. Each of which needs dependency foo. One of them expects it at @foo_label_in_a and one expects it in @foo_label_in_b. Then I have two equivalent dependencies that look different to bazel. There is no way (nor even standard convention as far as I can see) on what to name dependencies as a function of their semantic identity.

I think the right way to go here is that A expects a binding to say a_bind_of_foo and B expects b_bind_of_foo then when I depend on both, I set up the bindings appropriately.

I wish the bazel core team had someone whose role was explicitly to advocate for the many external repo use case (hopefully many of them bazel). That is the use case of the vast numbers of people using most existing tools (and migration to monorepo as a precondition to use bazel is not so workable, especially in open source: what would that even mean?_

@ejona86
Copy link

@ejona86 ejona86 commented Jan 14, 2019

I filed a StackOverflow question where bind() seems to be the only real solution. I've not seen any solution other than bind() which fixes transitive dependencies cross-repo for Maven artifacts.

Generating java_library()s in third_party simply doesn't work from within a library because there is no way for an application to change the transitive dependencies. They can change the version of transitive dependencies, but not add/remove dependencies as appropriate for a new version.

mikedanese added a commit to mikedanese/jsonnet that referenced this issue Feb 21, 2019
* explicitly load git_repository rule
* load googletest as a git_repository
* stop using bind (see bazelbuild/bazel#1952)
sparkprime added a commit to sparkprime/jsonnet that referenced this issue May 15, 2019
* explicitly load git_repository rule
* load googletest as a git_repository
* stop using bind (see bazelbuild/bazel#1952)
sparkprime added a commit to google/jsonnet that referenced this issue May 15, 2019
* explicitly load git_repository rule
* load googletest as a git_repository
* stop using bind (see bazelbuild/bazel#1952)
@damienpontifex
Copy link

@damienpontifex damienpontifex commented Nov 22, 2019

I am looking at the exact item mentioned by @jart at the top

ABI-compatible implementations of a library (e.g. OpenSSL, BoringSSL, etc.) this problem could still be solved by using vanilla externals.

Reading through this thread, it's still not evident to me how I solve this problem. Can someone provide guidance on how I could have a http_archive of boringssl, but another dependency is looking for @openssl. How can I just include the original boringssl dependency without needing to also add another http_archive for openssl? Is it an alias or something similar?
Or equally, what have I missed with the statement "using vanilla externals"? And what does that look like in bazel?

@jin
Copy link
Member

@jin jin commented Nov 22, 2019

@damienpontifex Check out repository remapping:

http_archive(
    name = "my_boringssl",
    # ...
)

http_archive(
    name = "dependency_that_uses_openssl",
    repo_mapping = {
        "@openssl": "@my_boringssl",
    },
    # ...
)
cpovirk added a commit to google/dagger that referenced this issue Feb 27, 2020
…_bzl.

...by load()-ing java_library and other rules wherever we use them.

Compare to CL 297412705 for Flogger.

This CL includes updating to a new version of bazel_common to avoid --incompatible_load_java_rules_from_bzl errors in bazel_common. See google/bazel-common#104.

Note that this CL also changes the way we get zlib() (a dependency of protobuf) from bind() to http_archive().
http_archive() seems to be the more recommended pattern:
- https://docs.bazel.build/versions/master/external.html#repository-rules
- bazelbuild/bazel#1952
But my immediate motivation was that bind() wasn't working with the new version of protobuf. (The new version of protobuf is necessary to avoid --incompatible_load_java_rules_from_bzl errors inside protobuf. It comes with the new version of bazel_common.)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=297608393
cpovirk added a commit to google/dagger that referenced this issue Feb 27, 2020
…_bzl.

...by load()-ing java_library and other rules wherever we use them.

Compare to CL 297412705 for Flogger.

This CL includes updating to a new version of bazel_common to avoid --incompatible_load_java_rules_from_bzl errors in bazel_common. See google/bazel-common#104.

Note that this CL also changes the way we get zlib() (a dependency of protobuf) from bind() to http_archive().
http_archive() seems to be the more recommended pattern:
- https://docs.bazel.build/versions/master/external.html#repository-rules
- bazelbuild/bazel#1952
But my immediate motivation was that bind() wasn't working with the new version of protobuf. (The new version of protobuf is necessary to avoid --incompatible_load_java_rules_from_bzl errors inside protobuf. It comes with the new version of bazel_common.)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=297608393
@meisterT meisterT removed this from the 1.0 milestone May 12, 2020
@meisterT
Copy link
Member

@meisterT meisterT commented May 15, 2020

We have no plans to deprecate bind.

@oehme
Copy link

@oehme oehme commented Jul 31, 2020

In that case, the documentation should be updated. It should probably explain some of the potential misuses and better alternatives, along with valid use cases.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.