[bazel] Add fixes for --incompatible_load_proto_rules_from_bzl by Yannic · Pull Request #20783 · grpc/grpc

Yannic · 2019-10-24T14:57:52Z

Starting soon, Protobuf rules will require explicit load statements
bazelbuild/bazel#8922

@yashykt

linux-foundation-easycla · 2019-10-24T14:57:56Z

The committers are authorized under a signed CLA.

✅ Yannic (f328ac6, bfbc309)

jtattermusch · 2019-10-24T17:08:38Z

@gnossen can you please review?

gnossen · 2019-10-24T17:21:15Z

bazel/grpc_python_deps.bzl

 load("@com_github_grpc_grpc//third_party/py:python_configure.bzl", "python_configure")

 def grpc_python_deps():
-    native.bind(


Does this constitute a breaking change for those who consume us?

I see that we're forced to do this because protobuf already made this breaking change. :/

@nicolasnoble Thoughts on yet another Bazel breaking change? As far as I can tell, the options are:

accept the breaking change and force all of our downstream users to do the same

get protobuf to roll back the change and potentially break users who have already accommodated it

Regardless, it's pretty clear that protobuf needs something like a Bazel distribtest.

Yes, this could break some consumers if they explicitly depend on how six is defined.
I still think it's worth moving away from bind().

@nicolasnoble and I talked offline. I agree that moving away from bind is the right option. But we can't just break backwards compatibility whenever we like. We'd like to batch up several backwards incompatible changes and synchronize them with our addition of official support for Bazel 1.0. We'll be discussing this in a meeting early next week. Until then, we'll need to put this change on hold.

third_party/six.BUILD

WORKSPACE

gnossen · 2019-10-24T17:37:39Z

Build failure from CI:

+ bazel test --spawn_strategy=standalone --genrule_strategy=standalone --test_output=errors //src/python/...
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
Loading:
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
DEBUG: /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:226:13: rbe_msan not using checked in configs; Bazel version 0.29.1 was picked/selected with '["9.0.0"]' compatible configs but none match the 'env = {"ABI_LIBC_VERSION": "glibc_2.19", "ABI_VERSION": "clang", "BAZEL_COMPILER": "clang", "BAZEL_HOST_SYSTEM": "i686-unknown-linux-gnu", "BAZEL_TARGET_CPU": "k8", "BAZEL_TARGET_LIBC": "glibc_2.19", "BAZEL_TARGET_SYSTEM": "x86_64-unknown-linux-gnu", "CC": "clang", "CC_TOOLCHAIN_NAME": "linux_gnu_x86", "BAZEL_LINKOPTS": "-lc++:-lc++abi:-lm"}', 'config_repos = None',and/or 'create_cc_configs = True' passed as attrs
Analyzing: 169 targets (28 packages loaded, 0 targets configured)
Analyzing: 169 targets (69 packages loaded, 783 targets configured)
Analyzing: 169 targets (69 packages loaded, 783 targets configured)
Analyzing: 169 targets (69 packages loaded, 783 targets configured)
ERROR: /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/external/com_github_grpc_grpc/bazel/grpc_python_deps.bzl:8:9: in http_archive rule //external:six: Found reference to a workspace rule in a context where a build rule was expected; probably a reference to a target in that external repository, properly specified as @reponame//path/to/package:target, should have been specified by the requesting rule.
ERROR: Analysis of target '//src/python/grpcio_tests/tests/unit:_abort_test.python3' failed; build aborted: Analysis of target '//external:six' failed; build aborted
INFO: Elapsed time: 24.682s
INFO: 0 processes.

Yannic

Thanks for your review! ptal

third_party/six.BUILD

Yannic · 2019-10-24T17:53:51Z

bazel/grpc_python_deps.bzl

 load("@com_github_grpc_grpc//third_party/py:python_configure.bzl", "python_configure")

 def grpc_python_deps():
-    native.bind(


Yes, this could break some consumers if they explicitly depend on how six is defined.
I still think it's worth moving away from bind().

WORKSPACE

Yannic · 2019-12-16T21:49:32Z

@gnossen Any news on this?

gnossen · 2020-01-06T18:08:54Z

@Yannic Sorry for the long delay. It's clear that this is the only way we're going to be able to update our protobuf dependency, so I'd like to go ahead and fast track this for merge.

gnossen · 2020-01-08T19:15:03Z

@Yannic

ERROR: /root/.cache/bazel/_bazel_root/d2dc70c3d9da3fab488ba0dcbbd35051/external/com_github_grpc_grpc/bazel/grpc_python_deps.bzl:8:9: in http_archive rule //external:six: Found reference to a workspace rule in a context where a build rule was expected; probably a reference to a target in that external repository, properly specified as @reponame//path/to/package:target, should have been specified by the requesting rule.
ERROR: Analysis of target '//src/python/grpcio_tests/tests_aio/benchmark:server' failed; build aborted: Analysis of target '//external:six' failed; build aborted
INFO: Elapsed time: 22.044s
INFO: 0 processes.

There are still some issues with the six changes. I'd suggest find src examples -name 'BUILD*' | xargs grep six to make sure you've got everything. For reference, I also recently tackled this problem in another PR (319686f) and that fully resolved the six build issues. Feel free to cherrypick if it helps.

Yannic · 2020-01-08T20:03:53Z

Thanks for pinging. I'll take a look tomorrow.

Yannic · 2020-01-10T20:00:37Z

This is partly superseded by #21590, so let's wait until that lands as this will likely show the same issues with blacklisted protos.

Yannic · 2020-02-05T19:01:44Z

@gnossen I finally found the time to update this. platl

gnossen · 2020-02-05T19:56:20Z

Awesome! Running CI on it.

Yannic · 2020-02-07T15:21:44Z

Thanks @gnossen! CI failures look unrelated.

gnossen · 2020-02-07T23:46:02Z

Failures:

Artifact Build Linux: quay.io Docker Registry SSL handshake timeout.
Basic Tests C/C++ MacOS
Basic Tests Node MacOS
Basic Tests ObjC iOS
Bazel C/C++ Dbg MacOS

gnossen · 2020-02-07T23:46:53Z

@Yannic Thanks for the contribution!

Yannic requested review from gnossen, jtattermusch, nicolasnoble and veblush as code owners October 24, 2019 14:57

jtattermusch assigned gnossen Oct 24, 2019

jtattermusch added the kokoro:run label Oct 24, 2019

grpc-kokoro removed the kokoro:run label Oct 24, 2019

gnossen reviewed Oct 24, 2019

View reviewed changes

Yannic commented Oct 24, 2019

View reviewed changes

gnossen added area/build infra/Bazel lang/all wrapped languages release notes: yes Indicates if PR needs to be in release notes disposition/on hold labels Oct 24, 2019

gnossen mentioned this pull request Jan 6, 2020

Update third_party/protobuf to v3.11.2 #21590

Merged

gnossen added the kokoro:run label Jan 6, 2020

grpc-kokoro removed the kokoro:run label Jan 6, 2020

[bazel] Add fixes for --incompatible_load_proto_rules_from_bzl

06ecc87

Yannic force-pushed the proto_incompat branch from aedbad6 to 06ecc87 Compare February 5, 2020 18:58

gnossen added the kokoro:run label Feb 5, 2020

grpc-kokoro removed the kokoro:run label Feb 5, 2020

gnossen approved these changes Feb 7, 2020

View reviewed changes

gnossen merged commit a2e4a23 into grpc:master Feb 7, 2020

Yannic deleted the proto_incompat branch February 8, 2020 12:05

lock bot locked as resolved and limited conversation to collaborators May 20, 2020

Conversation

Yannic commented Oct 24, 2019

Uh oh!

linux-foundation-easycla bot commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtattermusch commented Oct 24, 2019

Uh oh!

gnossen Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

gnossen Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yannic Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

gnossen Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gnossen commented Oct 24, 2019

Uh oh!

Yannic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yannic Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yannic commented Dec 16, 2019

Uh oh!

gnossen commented Jan 6, 2020

Uh oh!

gnossen commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yannic commented Jan 8, 2020

Uh oh!

Yannic commented Jan 10, 2020

Uh oh!

Yannic commented Feb 5, 2020

Uh oh!

gnossen commented Feb 5, 2020

Uh oh!

Yannic commented Feb 7, 2020

Uh oh!

gnossen commented Feb 7, 2020

Uh oh!

gnossen commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla bot commented Oct 24, 2019 •

edited

Loading

gnossen Oct 24, 2019 •

edited

Loading

gnossen commented Jan 8, 2020 •

edited

Loading