Upgrade bazelbuild rules_go to latest release: 0.13.0

[zanes2016]

There have been some new features and significant improvements since release 0.11.0 that should be picked up.

[mattklein123]

@zanes2016 thank you. Please check CI and DCO.

[zanes2016]

Thanks @mattklein123 , see the pending PR bufbuild/protoc-gen-validate#86 for the dependency fix.

[zanes2016]

Once bufbuild/protoc-gen-validate#87 is merged, then the CI should pass

[zanes2016]

Issue

Repeated Failures: proto package names should not be mixed in the same directory

Any idea what needs to be done about this failure?

Is it an issue with the Bazel api_proto_library_internal rule not handling deps correctly?

Outputs

BUILD

load("//bazel:api_build_system.bzl", "api_proto_library_internal")

licenses(["notice"])  # Apache 2

api_proto_library_internal(
    name = "mongo_proxy",
    srcs = ["mongo_proxy.proto"],
    deps = ["//envoy/config/filter/fault/v2:fault"],
)

stderr

...
2018/07/31 18:19:30 protoc-gen-go: error:[gatherer][file:envoy/config/filter/network/mongo_proxy/v2/mongo_proxy.proto]proto package names should not be mixed in the same directory (envoy.config.filter.fault.v2, envoy.config.filter.network.mongo_proxy.v2)
--validate_out: protoc-gen-validate: Plugin failed with status code 1.
...

File

$ ls api/envoy/config/filter/network/mongo_proxy/v2/
BUILD			mongo_proxy.proto
$
$ cat api/envoy/config/filter/network/mongo_proxy/v2/mongo_proxy.proto
syntax = "proto3";

package envoy.config.filter.network.mongo_proxy.v2;
option go_package = "v2";

import "envoy/config/filter/fault/v2/fault.proto";

import "validate/validate.proto";

// [#protodoc-title: Mongo proxy]
// MongoDB :ref:`configuration overview <config_network_filters_mongo_proxy>`.

message MongoProxy {
  // The human readable prefix to use when emitting :ref:`statistics
  // <config_network_filters_mongo_proxy_stats>`.
  string stat_prefix = 1 [(validate.rules).string.min_bytes = 1];

  // The optional path to use for writing Mongo access logs. If not access log
  // path is specified no access logs will be written. Note that access log is
  // also gated :ref:`runtime <config_network_filters_mongo_proxy_runtime>`.
  string access_log = 2;

  // Inject a fixed delay before proxying a Mongo operation. Delays are
  // applied to the following MongoDB operations: Query, Insert, GetMore,
  // and KillCursors. Once an active delay is in progress, all incoming
  // data up until the timer event fires will be a part of the delay.
  envoy.config.filter.fault.v2.FaultDelay delay = 3;
}

[moderation]

bufbuild/protoc-gen-validate#87 has merged and I thought I'd try this PR. With my local compilation I get the following errors:

ERROR: ~/.cache/bazel/_bazel_username/3002a12f12a06255067bdca8d32b0440/external/envoy_api/envoy/service/discovery/v2/BUILD:5:1: ProtoCompile external/envoy_api/envoy/service/discovery/v2/ads.pb.validate.h failed (Exit 1)

2018/07/31 14:24:40 protoc-gen-go: error:[gatherer][file:envoy/service/discovery/v2/ads.proto]proto package names should not be mixed in the same directory (envoy.api.v2, envoy.service.discovery.v2)
--validate_out: protoc-gen-validate: Plugin failed with status code 1.

These are different errors than those that @zanes2016 posted above I suspect as I'm compiling out a lot of the extensions including Mongo.

[zanes2016]

@moderation It looks like you're seeing the same type of error: proto package names should not be mixed in the same directory (see my updated comment).

I suspect that the Bazel rule dependencies from load("//bazel:api_build_system.bzl", "api_proto_library_internal") do not play nice with the protoc-gen-validate from https://github.com/lyft/protoc-gen-validate.git.

It seems to complain by whatever source is in the deps field of api_proto_library_internal. For example,

api_proto_library_internal(
    name = "mongo_proxy",
    srcs = ["mongo_proxy.proto"],
    deps = ["//envoy/config/filter/fault/v2:fault"],
)

@mattklein123 Do you have any ideas?

[mattklein123]

@zanes2016 I have no idea. @jmillikin-stripe @htuch might know.

[jmillikin-stripe]

Where is that error "proto package names should not be mixed in the same directory" coming from? There is no such rule in protobuf, and it's very common for a single proto/ directory to contain protos from many packages. Is this something PGV is enforcing? Can it be told to not enforce that strange invariant?

[akonradi]

The "proto package names should not be mixed in the same directory" error is coming from protoc-gen-star, which PVG builds on: https://github.com/lyft/protoc-gen-star/blob/f3b83fca61817e55058744eb1a3c8e2e95a0dada/gatherer.go#L114

[clnperez]

If I test this on ppc64le, I get ERROR: error loading package '': Extension file not found. Unable to load file '@com_lyft_protoc_gen_validate//bazel:go_proto_library.bzl': file doesn't exist or isn't a file

I see that bufbuild/protoc-gen-validate#87 is merged. Does something need to be bumped here?

[moderation]

@clnperez If you include this commit - 2431eda that should unblock you. However you will likely hit the errors that are commented on above.

[clnperez]

@moderation -- Indeed! I thought I had all those commits, but that link got me to the same place you're at. Thanks!

[zanes2016]

@jmillikin-stripe How do you recommend we proceed?

Should a PR be created to add an optional flag/config to PGV that skips the above check? Do you know where PGV is actually imported/triggered?

[jmillikin-stripe]

Should a PR be created to add an optional flag/config to PGV that skips the above check? Do you know where PGV is actually imported/triggered?

I don't think PGV should be doing that check at all. Why does the validation code need to care about file layout?

Not very familiar with Envoy's custom protobuf setup, but as I understand it the validation stuff is plumbed into envoy_build_system.bzl. Probably someone from Lyft (@mattklein123 ?) would have more context on their protobuf validator plugin.

[mattklein123]

I don't know anything about this. I would open an issue in PGV and discuss. @rodaine can help you.

[rodaine]

So for context, protoc-gen-validate (PGV) is written on the protoc-gen-star (PG*) framework that currently leans heavily on protoc-gen-go (PGG). This check stems from existing logic in PGG that does not allow inconsistent package names between the files being run as PGG is much stricter about directory structure (as is Go, see this issue on the subject).

All that said, there's work happening to remove the PGG dependency altogether from the PG* core, so this issue will be resolved at that time. You should expect PGV updated within the week.

[moderation]

bufbuild/protoc-gen-validate#92 has merged. Updating the sha and using the changes to WORKSPACE outlined at https://github.com/envoyproxy/envoy/pull/3949/files generates a bunch of errors like:

ERROR: ~/.cache/bazel/_bazel_moderation/3002a12f12a06255067bdca8d32b0440/external/envoy_api/envoy/config/accesslog/v2/BUILD:5:1: Traceback (most recent call last):
        File "~/.cache/bazel/_bazel_moderation/3002a12f12a06255067bdca8d32b0440/external/envoy_api/envoy/config/accesslog/v2/BUILD", line 5
                api_proto_library_internal(name = "als", srcs = ["als.proto"], ..."])
        File "~/.cache/bazel/_bazel_moderation/3002a12f12a06255067bdca8d32b0440/external/envoy_api/bazel/api_build_system.bzl", line 85, in api_proto_library_internal
                api_proto_library(visibility = visibility, **kwargs)
        File "~/.cache/bazel/_bazel_moderation/3002a12f12a06255067bdca8d32b0440/external/envoy_api/bazel/api_build_system.bzl", line 123, in api_proto_library
                pgv_cc_proto_library(name = _Suffix(name, _CC_SUFFIX), <4 more arguments>)
        File "~/.cache/bazel/_bazel_moderation/3002a12f12a06255067bdca8d32b0440/external/com_lyft_protoc_gen_validate/bazel/pgv_proto_library.bzl", line 40, in pgv_cc_proto_library
                native.cc_library(name = name, hdrs = [((":" + name)...")], <4 more arguments>)
duplicate keyword 'srcs' in call to native.cc_library

I suspect further changes are required in WORKSPACE but I haven't worked that out yet. cc @rodaine

[akonradi]

@moderation The PGV proto generation rule signatures have changed, which is causing the duplicate 'srcs' keyword error. You'll need to update the rule here to point the pgv_cc_proto_library call at proto_library targets like the one created a few lines above:

envoy/api/bazel/api_build_system.bzl

Line 125 in 0cf947f

srcs = srcs,

Try changing srcs = srcs to deps = [name] and 'deps = ...tocc_deps=...`.

[sesmith177]

@moderation the current version of PGV doesn't work with Envoy -- it will fail with:

2018/08/09 10:40:50 protoc-gen-go: error:[gatherer][file:envoy/config/trace/v2/trace.proto]proto package names should not be mixed in the same directory (envoy.api.v2, envoy.config.trace.v2)                                                                                                   
--validate_out: protoc-gen-validate: Plugin failed with status code 1

After that is fixed (lyft/protoc-gen-star#28, I think), and after bufbuild/protoc-gen-validate#94 is merged, we plan to PR the bump to PGV into Envoy (we've already worked out most of the changes required)

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[clnperez]

Can this be revisited?

[moderation]

@clnperez now that my dependency PR is in I'd like to look at this this week. I believe the critical blocker in PGV has been resolved so this in theory this should be possible.

[zanes2016]

@moderation Did you get a chance to take a look at this?

[moderation]

@zanes2016 I have not had a chance to look at this. Traveling for work all this week so will be tough. May be possible to do some testing later today.

[moderation]

I tried to update to the latest commit sha for PGV and tried my previously working diff at #3786 (comment) however there have been enough changes that these modifications now longer work. I'll ping @akonradi in Slack.

#3786 is not merged either but not sure if this is required /cc @sesmith177

[sesmith177]

#3786 shouldn't be required. I suspect bufbuild/protoc-gen-validate#89 is likely to still be an issue since it doesn't look like PGV has been updated with the latest protoc-gen-star yet

[akonradi]

Chiming in here: the diff below fixes the invocation of pgv_cc_proto_library which recently changed to take proto_library targets instead of .proto files. This results in a new issue, though, which is that the generated Go code for protos (not validation code, pure protobuf code) is invalid.

diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl
index 497d82c5c..45776ea89 100644
--- a/api/bazel/api_build_system.bzl
+++ b/api/bazel/api_build_system.bzl
@@ -116,15 +116,10 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
         visibility = visibility,
     )

-    # Under the hood, this is just an extension of the Protobuf library's
-    # bespoke cc_proto_library. It doesn't consume proto_library as a proto
-    # provider. Hopefully one day we can move to a model where this target and
-    # the proto_library above are aligned.
     pgv_cc_proto_library(
         name = _Suffix(name, _CC_SUFFIX),
-        srcs = srcs,
-        deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps],
-        external_deps = [
+        deps = [name],
+        cc_deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps] + [
             "@com_google_protobuf//:cc_wkt_protos",
             "@googleapis//:http_api_protos",
             "@googleapis//:rpc_status_protos",
diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl
index 9ce66300a..adeedadee 100644
--- a/api/bazel/repositories.bzl
+++ b/api/bazel/repositories.bzl
@@ -3,7 +3,7 @@ GOGOPROTO_SHA = "1adfc126b41513cc696b209667c8656ea7aac67c"  # v1.0.0
 PROMETHEUS_SHA = "99fa1f4be8e564e8a6b613da7fa6f46c9edafc6c"  # Nov 17, 2017
 OPENCENSUS_SHA = "ab82e5fdec8267dc2a726544b10af97675970847"  # May 23, 2018

-PGV_GIT_SHA = "e60e7f91da46a87d9067679064571c6954343c3c"  # July 31, 2018
+PGV_GIT_SHA = "64fcb82c878efbfe3240e4f164490eefaf6b3c4b"  # Sept 11, 2018

 load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")

@@ -33,7 +33,10 @@ filegroup(
 proto_library(
     name = "api_httpbody_protos_proto",
     srcs = [":api_httpbody_protos_src"],
-    deps = ["@com_google_protobuf//:descriptor_proto"],
+    deps = [
+       "@com_google_protobuf//:any_proto",
+       "@com_google_protobuf//:descriptor_proto",
+    ],
     visibility = ["//visibility:public"],
 )

[rodaine]

#3786 shouldn't be required. I suspect bufbuild/protoc-gen-validate#89 is likely to still be an issue since it doesn't look like PGV has been updated with the latest protoc-gen-star yet

Working on this currently; I have it building with just the Go toolchain and trying to get the BUILD files appropriately updated. Right now, I'm blocked with gazelle generating erroneous targets, and I've been unsuccessful with a fix for them. Once I get a free second this week, I'll push the code up to hopefully get help triaging what I'm doing wrong.

[akonradi]

@rodaine Happy to help with BUILD files. I haven't used gazelle but it sounds like it was designed for code that follows the Go package directory layout, which PVG doesn't.

[clnperez]

Is there anyone else who could take a crack at this? It's currently the only thing left before we can have simple builds for envoy on other architectures. I hate to ask other people for things in an OSS community, but, this one's just out of my wheel house. :)

[moderation]

@clnperez this is already complete. In master this commit is circa 0.15 - https://github.com/envoyproxy/envoy/blob/master/bazel/repository_locations.bzl#L169-L174. I'm actually running rules_go 0.16 without issue. This fixed compilation for arm64 - see bazel-contrib/rules_go@be231c1#diff-7b3b235551404141ea1cd86713732d44. Not sure about ppc.

     io_bazel_rules_go = dict(
-        sha256 = "d1ad521fbd0997df53161e29df0964468157fc9c6ee16265db37cc6daaf334ef",
-        strip_prefix = "rules_go-3d966375ff7971d43b863f785f495c7dcd6923da",
-        # 2018-10-02
-        urls = ["https://github.com/bazelbuild/rules_go/archive/3d966375ff7971d43b863f785f495c7dcd6923da.tar.gz"],
+        sha256 = "d0e51353639cd6ea43e614d80c29d3f7f318e88dbbc28c4d8c07b129b46e20c0",
+        strip_prefix = "rules_go-0.16.0",
+        urls = ["https://github.com/bazelbuild/rules_go/archive/0.16.0.tar.gz"],
     ),

[jmillikin-stripe]

I've sent a PR to take the short hop from the current 0.15-ish version to 0.16: #4836

[clnperez]

oh hey! i had no idea. i was watching this pr. thanks @moderation and @jmillikin-stripe