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.

Sign up for GitHub

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

Jump to bottom

Remove com_google_protobuf_java #249

Merged
merged 1 commit into from
Apr 26, 2018
Merged

Remove com_google_protobuf_java #249

merged 1 commit into from
Apr 26, 2018

Conversation

davido
Copy link
Contributor

@davido davido commented Jan 23, 2018

Closes #248.

This was deprecated with Bazel 0.8.0, which now uses
@com_google_protobuf instead.

This change will break users that use
closure_repositories(omit_com_google_protobuf_java=True), so I've
added a custom error message to make the resolution clearer.

Test Plan:

$ bazel test javatests/...

and confirm that the warning from #248 is gone.

Inspired-By: Rodrigo Queiro overdrigzed@gmail.com

@bazel-io
Copy link
Member

Can one of the admins verify this patch?

@davido davido mentioned this pull request Jan 23, 2018
Closed
@davido
Copy link
Contributor Author

davido commented Jan 23, 2018 •
edited
Loading

The CI failure is related and caused by too old Bazel version on Travis CI:

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.

The minimum required Bazel version is 0.8.0.

@davido davido force-pushed the remove-protobuf-java branch 2 times, most recently from b23e17e to 98c6377 Compare January 24, 2018 06:45
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this pull request Feb 7, 2018
@davido
Bazel: Update to custom rules_closure to avoid build warning
a0326ec 
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
@jefesaurus
Copy link

Would be nice to have this. Also getting that Bazel nastrygram: WARNING: /code/.cache/bazel/_bazel_glalonde/6fa7a91faa1abdfbb41bc875fa66f0f6/external/com_google_protobuf_java/WORKSPACE:1: Workspace name in /code/.cache/bazel/_bazel_glalonde/6fa7a91faa1abdfbb41bc875fa66f0f6/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

@davido
Copy link
Contributor Author

davido commented Apr 12, 2018

Until this is merged, you could consider to consume rules_closure from my release page, where this fix is included: [1].

[1] https://github.com/davido/rules_closure/releases.

@jart
Copy link
Contributor

jart commented Apr 12, 2018

Could you sync to master?

x1ddos
x1ddos reviewed Apr 23, 2018
View reviewed changes
closure/repositories.bzl
@@ -66,6 +66,8 @@ def closure_repositories(
omit_org_ow2_asm_util=False,
omit_phantomjs=False):
"""Imports dependencies for Closure Rules."""
if omit_com_google_protobuf_java:
fail("omit_com_google_protobuf_java no longer supported and must be not be passed to closure_repositories()")
Copy link

@x1ddos x1ddos Apr 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if the error message also specified either an alternative or referenced a public issue for users to go look for answers such as "why?"

Copy link
Contributor Author

@davido davido Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see @jart's comment on the original issue I raised for this warning: #248 (comment).

I'm not in position to summarize that move in dense couple of words (that would be appropriate for the build tool chain error message), but I would happily add any suggestions from the project maintainers to extend/adjust the current error message, if this would help get this CL merged.

@davido
Remove com_google_protobuf_java
544fdec 
Closes bazelbuild#248.

com_google_protobuf_java was deprecated with Bazel 0.8.0, which now uses
@com_google_protobuf instead.

This change will break users that use
closure_repositories(omit_com_google_protobuf_java=True), so I've
added a custom error message to make the resolution clearer.

Test Plan:

  $ bazel test javatests/...

and confirm that the warning from bazelbuild#248 is gone.

Inspired-By: Rodrigo Queiro <overdrigzed@gmail.com>
@davido
Copy link
Contributor Author

davido commented Apr 24, 2018 •
edited
Loading

@jart

Could you sync to master?

Done.

@jart jart merged commit f04ee86 into bazelbuild:master Apr 26, 2018
@davido davido deleted the remove-protobuf-java branch April 26, 2018 20:36
ptmphuong pushed a commit to ptmphuong/rules_closure that referenced this pull request Dec 9, 2022
@davido @ptmphuong
Remove com_google_protobuf_java (bazelbuild#249)
7c6d5a5 
Closes bazelbuild#248.

com_google_protobuf_java was deprecated with Bazel 0.8.0, which now uses
@com_google_protobuf instead.

This change will break users that use
closure_repositories(omit_com_google_protobuf_java=True), so I've
added a custom error message to make the resolution clearer.

Test Plan:

  $ bazel test javatests/...

and confirm that the warning from bazelbuild#248 is gone.

Inspired-By: Rodrigo Queiro <overdrigzed@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@x1ddos x1ddos x1ddos left review comments

@jart jart jart approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davido @bazel-io @jefesaurus @jart @x1ddos @googlebot