Update rules_go and gazelle #802

drigz · 2018-11-09T13:56:09Z

This requires/allows the following changes:

rerun Gazelle to regenerate the build files
regenerate examples with the newer protoc
fix tests to correctly handle enums inside maps (jsonpb: handle map keys' and values' properties properly golang/protobuf#645)
remove repositories.bzl (replaced by @go_googleapis)

Also, enforce up-to-date build files in the CI check.

WORKSPACE

runtime/marshal_jsonpb_test.go

This requires/allows the following changes: - rerun Gazelle to regenerate the build files - fix marshal_jsonpb_test to assert correct behavior when EnumsAsInts == false - remove `repositories.bzl` (replaced by @go_googleapis) Also, enforce up-to-date build files in the CI check. Note: this version of the commit includes an out-of-date BUILD file to test the CI check.

johanbrandhorst · 2018-11-12T14:02:48Z

What's next to do here? I've lost sight a bit. Do you need anything from us?

drigz · 2018-11-12T14:24:18Z

TBH I'm hoping someone who understands this "enums inside maps" issue will appear and tell me what to do. I don't want to change the behavior of grpc-gateway without understanding the consequences.

Alternatively, we may be able to keep the old behavior by setting EnumsAsInts appropriately for marshalling and unmarshalling. However, I don't have a lot of time to spend on this, as this is "just" a cleanup task on my side.

johanbrandhorst · 2018-11-12T14:29:38Z

If the behavior has been incorrect I think we can safely correct it without worrying about backwards compatibility. Is that simply a matter of copying the fix from the protobuf repo?

drigz · 2018-11-12T14:31:16Z

I wasn't able to easily fix node_test, as changing ints to strings introduced another error. Additionally, I need to regenerate the examples, which I haven't worked out how to do yet.

johanbrandhorst · 2018-11-12T14:34:49Z

Regenerating everything should be covered in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/CONTRIBUTING.md#i-want-to-regenerate-the-files-after-making-changes.

If fixing the tests is a pain, lets raise an issue and solve it in a separate PR. Is it necessarily part of this cleanup? Don't want to waste your time with red tape.

The generated code changed with the updated dependencies.

johanbrandhorst · 2018-11-12T14:55:05Z

Right I see what happened, we updated our dependency of golang/protobuf, and all of a sudden incorrect behaviour that we depended on was fixed, and now our tests fail. It seems we have to fix it in this PR then. I'm not too familiar with this code unfortunately, so I think we just have to do a little digging to get to the bottom of it. Changing the test is correct, so any part of our json marshaller that is doing the wrong thing is what needs to be fixed now.

Is that a correct summary?

johanbrandhorst · 2018-11-12T14:55:48Z

Node tests are failing with:

Expected $.map_value.a = 'ONE' to equal 1.

I wonder if it's just a matter of fixing these tests too?

Enum values inside maps are now supported.

johanbrandhorst · 2018-11-12T14:58:14Z

🤞

johanbrandhorst · 2018-11-12T14:58:59Z

This has become bigger than initially anticipated I think, but lets wait for @achew22 to have a chance to review it as well before merging. Huge thanks for getting the ball rolling on this though @drigz :).

johanbrandhorst · 2018-11-12T14:59:47Z

Node tests are looking good!

codecov-io · 2018-11-12T15:00:07Z

Codecov Report

Merging #802 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #802   +/-   ##
=======================================
  Coverage   53.27%   53.27%           
=======================================
  Files          30       30           
  Lines        3369     3369           
=======================================
  Hits         1795     1795           
  Misses       1399     1399           
  Partials      175      175

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ff87af...f6ea87e. Read the comment docs.

drigz · 2018-11-12T15:03:16Z

Yep, looks like I gave up too quickly. Thanks for the encouragement :)

ivucica · 2018-11-12T15:22:05Z

WORKSPACE


 gazelle_dependencies()

-load("@bazel_gazelle//:def.bzl", "go_repository")
-load("//:repositories.bzl", "repositories")


Not objecting, just trying to learn: Why remove repositories.bzl? Wouldn't it be better to allow third parties to import a repositories.bzl and transitively depend?

The repository defined by repositories.bzl is obsolete, having been replaced by @go_googleapis (created by go_rules_dependencies()). Since repositories.bzl is unable to call go_rules_dependencies() (bazelbuild/bazel#1550) there's nothing left for it to do.

ivucica · 2018-11-12T15:22:56Z

WORKSPACE

@@ -21,12 +29,10 @@ http_archive(
    url = "https://github.com/bazelbuild/buildtools/archive/e90e7cc6ef3e6d08d4ca8a982935c3eed638e058.tar.gz",
 )

-load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")
+load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")

 gazelle_dependencies()


Nob objecting, just trying to learn: Maybe I'm not tracking developments in Bazel world closely enough, but if gazelle_dependencies() is no longer imported, where is this defined? Why does this work?

gazelle_dependencies is still loaded from bazel_gazelle//:deps.bzl, along with go_repository. The only change here is that two load() statements have been consolidated into one.

achew22 · 2018-11-13T15:36:56Z

This has become bigger than initially anticipated I think, but lets wait for @achew22 to have a chance to review it as well before merging. Huge thanks for getting the ball rolling on this though @drigz :).

I think we should merge this as is. It will probably break people but upstream changed on us and we don't have much of an option. If this behavior is required by integrators there is a parameter they can set.

javasgl · 2018-12-12T09:49:36Z

Gopkg.toml

-  # https://github.com/bazelbuild/rules_go/blob/436452edc29a2f1e0edc22d180fbb57c27e6d0af/go/private/repositories.bzl#L75
-  version = "1.1.0"
+  # https://github.com/bazelbuild/rules_go/blob/109c520465fcb418f2c4be967f3744d959ad66d3/go/private/repositories.bzl#L52
+  revision = "aa810b61a9c79d51363740d207bb46cf8e620ed5"


master: Could not introduce github.com/grpc-ecosystem/grpc-gateway@master, as it has a dependency on github.com/golang/protobuf with constraint aa810b61a9c79d51363740d207bb46cf8e620ed5, which has no overlap with existing constraint master from github.com/grpc-ecosystem/go-grpc-middleware@master
@achew22 @drigz

broken!

I'm not a go dep but I don't think this change introduced the problem, as even before the version was constrained and would have conflicted with go-grpc-middleware's constraint. @javasgl Maybe you need to use an override to resolve the difference?

Yeah I've explained the situation in #829

* Update rules_go and gazelle This requires/allows the following changes: - rerun Gazelle to regenerate the build files - fix marshal_jsonpb_test to assert correct behavior when EnumsAsInts == false - remove `repositories.bzl` (replaced by @go_googleapis) Also, enforce up-to-date build files in the CI check. Note: this version of the commit includes an out-of-date BUILD file to test the CI check. * Update Gopkg.toml and Gopkg.lock * Update generated proto code The generated code changed with the updated dependencies. * Fix node_test Enum values inside maps are now supported.

googlebot added the cla: yes label Nov 9, 2018

johanbrandhorst requested a review from achew22 November 9, 2018 13:58

johanbrandhorst assigned achew22 Nov 9, 2018

johanbrandhorst reviewed Nov 9, 2018

View reviewed changes

WORKSPACE Show resolved Hide resolved

johanbrandhorst reviewed Nov 9, 2018

View reviewed changes

runtime/marshal_jsonpb_test.go Show resolved Hide resolved

drigz force-pushed the update-rules-go branch from fa78b24 to 84a56b3 Compare November 9, 2018 15:25

drigz added 2 commits November 9, 2018 16:30

Update Gopkg.toml and Gopkg.lock

cc55347

drigz force-pushed the update-rules-go branch from 84a56b3 to cc55347 Compare November 12, 2018 14:25

Update generated proto code

f7b17b0

The generated code changed with the updated dependencies.

Fix node_test

f6ea87e

Enum values inside maps are now supported.

ivucica reviewed Nov 12, 2018

View reviewed changes

achew22 approved these changes Nov 13, 2018

View reviewed changes

achew22 merged commit a7c0cd0 into grpc-ecosystem:master Nov 13, 2018

drigz mentioned this pull request Nov 19, 2018

Bazel CI breaks frequently #817

Closed

javasgl reviewed Dec 12, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rules_go and gazelle #802

Update rules_go and gazelle #802

drigz commented Nov 9, 2018 •

edited

Loading

johanbrandhorst commented Nov 12, 2018

drigz commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

drigz commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

codecov-io commented Nov 12, 2018

drigz commented Nov 12, 2018

ivucica Nov 12, 2018

drigz Nov 12, 2018

ivucica Nov 12, 2018

drigz Nov 12, 2018

achew22 commented Nov 13, 2018

javasgl Dec 12, 2018 •

edited

Loading

drigz Dec 12, 2018

johanbrandhorst Dec 12, 2018

Update rules_go and gazelle #802

Update rules_go and gazelle #802

Conversation

drigz commented Nov 9, 2018 • edited Loading

johanbrandhorst commented Nov 12, 2018

drigz commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

drigz commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

johanbrandhorst commented Nov 12, 2018

codecov-io commented Nov 12, 2018

Codecov Report

drigz commented Nov 12, 2018

ivucica Nov 12, 2018

Choose a reason for hiding this comment

drigz Nov 12, 2018

Choose a reason for hiding this comment

ivucica Nov 12, 2018

Choose a reason for hiding this comment

drigz Nov 12, 2018

Choose a reason for hiding this comment

achew22 commented Nov 13, 2018

javasgl Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

drigz Dec 12, 2018

Choose a reason for hiding this comment

johanbrandhorst Dec 12, 2018

Choose a reason for hiding this comment

drigz commented Nov 9, 2018 •

edited

Loading

javasgl Dec 12, 2018 •

edited

Loading