bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
22.99k stars 4.03k forks source link

Consider removing bind() #1952

Closed jart closed 4 years ago

jart commented 7 years ago

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().

liujisi commented 6 years ago

I was pointed here when reviewing the PR https://github.com/google/protobuf/pull/4204

Looks like the repo based dependency will attach the dependency to a specific version and deploy that dependency edge together with the library. How do we solve the diamond dependency problem when two dependency edges on the same library don't agree on the versions?

For instance, both libfoo and libbar depend on different versions of libprotobuf, then if an application depend on both libfoo and libbar, it can no longer compile due to the symbol conflicts/ ODR violation. The bind() approach gives the end user control which library/version should be depended on. Solving the problem by checking if a library is loaded or not in protobuf_dependencies() , IMO, is no better than bind(). The actual version will be determined by the load order. This brings side-effects when refactoring BUILD rules, which should be avoided. If the best practice is for users to always load dependency libraries manually first, then it's essentially the same as bind(), while bind() being more explicit to prevent errors.

I feel like the dependency problem here is much harder than in google. bind() is actually one way to enforce the one version rule in opensource.

Also, there needs to be a migration path. Even if we are moving away from bind, we cannot just cut a release that breaks everyone's build. Especially this case, as it requires an atomic change on the client code to adopt the new library.

jart commented 6 years ago

Note: I noticed not too long ago a use-case for bind(). gRPC uses it for this "unsecure" target that lets you hook in your own C++ authentication function. That's a good reason to use bind() IMHO although I feel like there's a better way Bazel could solve that particular problem.

Gentle remember that dependencies are exceedingly difficult, but the work is worth doing. Motivational reading: https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5 Anyone who looks to bind() for easy answers is likely to only dig that hole deeper.

ittaiz commented 6 years ago

@jart I think the last link is somewhat irrelevant since most Bazel users I've talked with use some kind of tool to generate the transitive closure and given a large graph (as often happens on the jvm) it becomes extremely difficult to know exactly what goes in. Best case people skim the list to make sure no conflicts. On Sat, 27 Jan 2018 at 3:26 Justine Tunney notifications@github.com wrote:

Note: I noticed not too long ago a use-case for bind(). gRPC uses it for this "unsecure" target that lets you hook in your own C++ authentication function. That's a good reason to use bind() IMHO although I feel like there's a better way Bazel could solve that particular problem.

Gentle remember that dependencies are exceedingly difficult, but the work is worth doing. Motivational reading: https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5 Anyone who looks to bind() for easy answers is likely to only dig that hole deeper.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel/issues/1952#issuecomment-360948264, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIF002jTPdHkpbIwPW8XnHe93kWkgrks5tOns5gaJpZM4KYF52 .

ejona86 commented 5 years ago

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.

damienpontifex commented 4 years ago

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 commented 4 years ago

@damienpontifex Check out repository remapping:

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

http_archive(
    name = "dependency_that_uses_openssl",
    repo_mapping = {
        "@openssl": "@my_boringssl",
    },
    # ...
)
meisterT commented 4 years ago

We have no plans to deprecate bind.

oehme commented 4 years ago

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.

warriorstar-orion commented 3 years ago

Hello,

As of today (2021-02-07) the master build encyclopedia still suggests that usage of bind() is not recommended, with a link to this bug. If it is indeed the case, as in https://github.com/bazelbuild/bazel/issues/1952#issuecomment-629113959, that there are no plans to deprecate bind(), and—as commented upon above by several—it exists to solve problems that cannot be solved in an alternative manner, then this warning should be removed. While I see much conversation above about it in regards specifically to Maven and Java, this is not my use case, and reading the documentation, and then following the link to this bug, gives me no more insight as to whether or not bind() is suitable for my use case.

As can be seen above, many repositories link to this bug as an explanation as to why bind() should not be used, such as https://github.com/liucijus/rules_scala/pull/1 which mentions removing use of the "problematic bind()". The warning on the documentation, and this bug, has created a feedback loop where projects will simply be asked to remove the use of bind(), by pointing to the warning in the documentation (e.g. https://github.com/grpc/grpc/issues/13902), even though, as https://github.com/bazelbuild/bazel/issues/1952#issuecomment-629113959 states, there are no plans to deprecate it, and the warning was added in response to this bug.

If it's not recommended, there should be a clear and specific justification in the documentation directly, or a specific deprecation plan. If the usage of bind() is not problematic, this should also be made clear.

meisterT commented 3 years ago

@Wyverald how does bind fit into the overhaul of external deps?

Wyverald commented 3 years ago

Overall, I don't think bind() can exist in the new world (for anyone lacking context: the external deps overhaul). This is admittedly a long thread, and after reading through some of the comments, I think most (if not all) usages of bind() can be replaced with repo_mapping and/or alias.

mering commented 8 months ago

The Bzlmod documentation states

The bind rule in WORKSPACE is deprecated and not supported in Bzlmod.

If it is really deprecated then the documentation should state that and there should be a deprecation/removal timeline communicated.

lberki commented 8 months ago

The page you linked to says "Warning: use of bind() is not recommended." . I thought that qualified as a deprecation warning, doesn't it?