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

Bazel: Add dependency to error_prone_annotations #5811

Conversation

davido
Copy link
Contributor

@davido davido commented Mar 3, 2019

Recently dependency to error_prone_annotations was added to the code,
but only Maven build tool chain was updated.

Closes #5795.

Recently dependency to error_prone_annotations was added to the code,
but only Maven build tool chain was updated.

Closes protocolbuffers#5795.
@TeBoring
Copy link
Contributor

TeBoring commented Mar 4, 2019

Can we add a test to avoid regression?

@davido
Copy link
Contributor Author

davido commented Mar 5, 2019

Well, it doesn't build, so that running this is failing:

  $ bazel build :protobuf_java :protobuf_java_util 

@drigz
Copy link

drigz commented Mar 5, 2019

@davido Thanks for sending this, it should unblock our upgrade to 3.7.0. Do you have any idea why this didn't break the build on Bazel CI? Latest build is green: https://buildkite.com/bazel/protobuf/builds/2318#800b3b9c-a999-4a56-957b-c51e25881591

@davido
Copy link
Contributor Author

davido commented Mar 5, 2019

Do you have any idea why this didn't break the build on Bazel CI? Latest build is green: [...]

That's because the CI is currently saying this:

  $ bazel test --build_tests_only [...] //:all
  [...]
  //:py_descriptor_database_test                                           PASSED in 1.0s
//:py_descriptor_pool_test                                               PASSED in 3.3s
//:py_descriptor_test                                                    PASSED in 1.3s
//:py_generator_test                                                     PASSED in 1.3s
//:py_json_format_test                                                   PASSED in 1.0s
//:py_message_factory_test                                               PASSED in 1.3s
//:py_message_test                                                       PASSED in 1.4s
//:py_proto_builder_test                                                 PASSED in 1.0s
//:py_reflection_test                                                    PASSED in 2.1s
//:py_service_reflection_test                                            PASSED in 1.0s
//:py_symbol_database_test                                               PASSED in 1.2s
//:py_text_encoding_test                                                 PASSED in 1.0s
//:py_text_format_test                                                   PASSED in 2.0s
//:py_unknown_fields_test                                                PASSED in 1.4s
//:py_wire_format_test                                                   PASSED in 0.5s

As you can see, there are currently only python tests available, so that all breakages in Java tool chain is not discovered.

I guess that's why @TeBoring suggested to add some Java tests to avoid future regressions in this area.

@drigz
Copy link

drigz commented Mar 5, 2019

Thanks, I hadn't noticed --build_tests_only. It seems like bazel build //:all doesn't work because :protobuf_objc can only be built on Darwin. Maybe adding an :all_ci_targets filegroup (as suggested by Lukacs here) and running bazel build :all_ci_targets in CI would be easier than adding tests to the Bazel build?

That said, I'd prefer this fix get in quickly, and we fix the CI in a followup change.

bind(
name = "error_prone_annotations",
actual = "@error_prone_annotations_maven//jar",
)
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

The other java dependencies (guava etc) aren't defined there, so this PR doesn't change anything in that respect.

@werkt
Copy link

werkt commented Mar 8, 2019

I'd like to also petition for this to end up in protobuf_deps.bzl, as indicated in change comments, and to use the bazel-deps fully qualified repository name (com_google_errorprone_error_prone_annotations) that it generates.

@acozzette
Copy link
Member

Let me go ahead and merge this now to fix the build breakage. I don't fully understand the proper way to set up our protobuf_deps.bzl file but if anyone is interested in sending a follow-up pull request then that would be great.

@acozzette acozzette merged commit 35c9a5f into protocolbuffers:master Mar 8, 2019
@ejona86
Copy link
Contributor

ejona86 commented Mar 8, 2019

Is this going to be backported to 3.7.x branch?

@davido
Copy link
Contributor Author

davido commented Mar 8, 2019

@ejona86

Is this going to be backported to 3.7.x branch?

Done in: #5862.

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

Successfully merging this pull request may close these issues.

9 participants