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

Recommended protobuf integration triggers bazel warning: external/com_google_protobuf_java/WORKSPACE (@com_google_protobuf) does not match #4495

Closed
davido opened this issue Jan 21, 2018 · 10 comments
Assignees

Comments

@davido
Copy link
Contributor

davido commented Jan 21, 2018

//CC @ejona86 @cgrushko as author of Protobuf integration recommendation and maintainer of https://github.com/cgrushko/proto_library.

In bazelbuild/rules_closure#231, bazelbuild/rules_closure#232 rules_closure recently moved from depending on protobuf release binaries to depending on the protobuf directly.

Gerrit Code Review updated recently to rules_closure@Head and now Gerrit build started to issue this warning:

WARNING: /private/var/tmp/_bazel_xxx/4e4bb427cd475349ab3944ff29048468/external/com_google_protobuf_java/WORKSPACE:1: Workspace name in /private/var/tmp/_bazel_xxx/4e4bb427cd475349ab3944ff29048468/external/com_google_protobuf_java/WORKSPACE (@com_google_protobuf)
does not match the name given in the repository's definition (@com_google_protobuf_java);
this will cause a build error in future versions

To reproduce, clone gerrit code review and issue bazel build gerrit.

I reported this issue in rules_closure repository, but I was recommended to ask for feedback here.

The alternative way to repro is to clone rules_closure and run the tests there:

davido@wizball:~/projects/rules_closure (master=)$ bazel test javatests/...
WARNING: /home/davido/.cache/bazel/_bazel_davido/2a0702ea37a9bc7e0f689212ecb3673c/external/com_google_protobuf_java/WORKSPACE:1: Workspace name in /home/davido/.cache/bazel/_bazel_davido/2a0702ea37a9bc7e0f689212ecb3673c/external/com_google_protobuf_java/WORKSPACE (@com_google_protobuf) does not match the name given in the repository's definition (@com_google_protobuf_java); this will cause a build error in future versions

The Protobuf integration recommendation is telling us to fetch the same repository multiple times under different names:

# proto_library rules implicitly depend on @com_google_protobuf//:protoc,
# which is the proto-compiler.
# This statement defines the @com_google_protobuf repo.
http_archive(
    name = "com_google_protobuf",
    urls = ["https://github.com/google/protobuf/archive/b4b0e304be5a68de3d0ee1af9b286f958750f5e4.zip"],
)

# cc_proto_library rules implicitly depend on @com_google_protobuf_cc//:cc_toolchain,
# which is the C++ proto runtime (base classes and common utilities).
http_archive(
    name = "com_google_protobuf_cc",
    urls = ["https://github.com/google/protobuf/archive/b4b0e304be5a68de3d0ee1af9b286f958750f5e4.zip"],
)

# java_proto_library rules implicitly depend on @com_google_protobuf_java//:java_toolchain,
# which is the Java proto runtime (base classes and common utilities).
http_archive(
    name = "com_google_protobuf_java",
    urls = ["https://github.com/google/protobuf/archive/b4b0e304be5a68de3d0ee1af9b286f958750f5e4.zip"],
)

What is the recommended way to avoid the warning? Or should Bazel accept such mismatch:
external/com_google_protobuf_java/WORKSPACE (@com_google_protobuf) does not match without warning, not to mention "cause a build error in future versions"?

@cgrushko
Copy link
Contributor

cgrushko commented Jan 22, 2018 via email

@davido
Copy link
Contributor Author

davido commented Jan 22, 2018

Seems like a bug in the gh.com/google/protobuf repo?

That's indeed the question here, whether or not is this a bug or a feature? In case it's a feature, then Bazel should accept the mismatch and remove the warning. Can you (or someone else) shed some light on what is the right next step here?

It can be also seen, that Bazel's own build tool chain uses a similar language-specific-protobuf-rules-duplication idiom under different names:

  • com_google_protobuf
  • com_google_protobuf_cc
  • com_google_protobuf_java

bazel/WORKSPACE

Lines 75 to 91 in bea6712

new_local_repository(
name = "com_google_protobuf",
build_file = "./third_party/protobuf/3.4.0/BUILD",
path = "./third_party/protobuf/3.4.0/",
)
new_local_repository(
name = "com_google_protobuf_cc",
build_file = "./third_party/protobuf/3.4.0/BUILD",
path = "./third_party/protobuf/3.4.0/",
)
new_local_repository(
name = "com_google_protobuf_java",
build_file = "./third_party/protobuf/3.4.0/com_google_protobuf_java.BUILD",
path = "./third_party/protobuf/3.4.0/",
)

@drigz
Copy link
Contributor

drigz commented Jan 22, 2018

The warning will be resolved in grpc-java v1.10.0, which will include grpc/grpc-java#3881. The warning is caused by grpc-java, not the recommendations (although these are indeed out-of-date - maybe worth another issue?)

@ejona86
Copy link

ejona86 commented Jan 22, 2018

The warning will be common for any project using the previous set up with @com_google_protobuf_java and similar. They should migrate to using @com_google_protobuf. This will be handled by each project individually. File a bug against the repo if it hasn't yet updated. In this case, bazelbuild/rules_closure.

@drigz, the warning will not be resolved by the grpc update, since @davido is having trouble with rules_closure because it still uses the old style.

@ejona86
Copy link

ejona86 commented Jan 22, 2018

cgrushko/proto_library#5 can be referenced to show projects how to update.

@drigz
Copy link
Contributor

drigz commented Jan 22, 2018

Ah, thanks for the correction, @ejona86.

@cgrushko
Copy link
Contributor

@ejona86 thanks for taking a look.
@davido, does that answer your question?

@davido
Copy link
Contributor Author

davido commented Jan 22, 2018

Thanks, it almost clarifies it. Looking into grpc/grpc-java#3881 and comparing it with the current situation in rules_closure@Head, we have actually a complication there with com_google_protobuf_js rules duplication:

https://github.com/bazelbuild/rules_closure/blob/08039ba8ca59f64248bb3b6ae016460fe9c9914f/closure/repositories.bzl#L643-L653:

def com_google_protobuf_js():
  native.new_http_archive(
      name = "com_google_protobuf_js",
      urls = [
          "https://mirror.bazel.build/github.com/google/protobuf/archive/v3.5.0.tar.gz",
          "https://github.com/google/protobuf/archive/v3.5.0.tar.gz",
      ],
      sha256 = "0cc6607e2daa675101e9b7398a436f09167dffb8ca0489b0307ff7260498c13c",
      strip_prefix = "protobuf-3.5.0/js",
      build_file = str(Label("//closure/protobuf:protobuf_js.BUILD")),
  )

As it uses its own build_file and different strip_prefix ("protobuf-3.5.0/js").

I'm not sure how to replace this according to the new style?

@davido
Copy link
Contributor Author

davido commented Jan 23, 2018

On second thought, I will let the fix for consumption of com_google_protobuf_js to rules_closure maintainers. I fixed the warning in gerrit build with this PR: [1] and here I forked rules_clsoure in gerrit: [2] to use this custom release: [3] until my PR is merged. Thanks again for the help.

[1] bazelbuild/rules_closure#249
[2] https://gerrit-review.googlesource.com/#/c/gerrit/+/153870
[3] https://github.com/davido/rules_closure/releases/tag/0.8.0

@davido davido closed this as completed Jan 23, 2018
@davido
Copy link
Contributor Author

davido commented Jan 23, 2018

FWIW: both CIs are now red with my CLs:

with the same message:

ERROR: /home/travis/build/bazelbuild/rules_closure/java/io/bazel/rules/closure/BUILD:60:1: every rule of type proto_library implicitly depends upon the target '@com_google_protobuf_java//:java_toolchain', but this target could not be found because of: no such package '@com_google_protobuf_java//': The repository could not be resolved.
ERROR: Analysis of target '//javatests/com/google/javascript/jscomp:JsCompilerTest' failed; build aborted.
INFO: Elapsed time: 8.570s
ERROR: Couldn't start the build. Unable to run tests.

That's because on both environments Bazel is tool old:

I guess Bazel must be at least 0.8.0 to support that, yet, rules_closure project removed recently Bazel version check, ...

lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Feb 7, 2018
Rules closure protobuf rules consumption is outdated and triggers
workspace name mismatch warning. See discussion in: [1],[2].

Fork rules_closure with the fix until the PR [3] is merged upstream.

[1] bazelbuild/rules_closure#248
[2] bazelbuild/bazel#4495
[3] bazelbuild/rules_closure#249

Bug: Issue 8182
Change-Id: Ifb59f90ac5a7604922b841377e75598369e9f70c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants