build: Bump rules_go and protobuf to pick up Windows fixes by sesmith177 · Pull Request #4556 · envoyproxy/envoy

sesmith177 · 2018-09-28T16:22:41Z

Description:

Bump rules_go to 1.15.3 (latest). This picks up some fixes so that
rules_go works on Windows. The api go tests don't appear to build with
go1.11, so pin to go1.10.4
Bump google/protobuf to fa252ec2, which picks up a fix for handling
Windows path separators

Risk Level:
Low
Testing:
Ran bazel build //source/exe:envoy-static && bazel test //test/... and bazel test @envoy_api//test/... @envoy_api//tools/...
Docs Changes:
n/a
Release Notes:
n/a

- Bump rules_go to 1.15.3 (latest). This picks up some fixes so that rules_go works on Windows. The api go tests don't appear to build with go1.11, so pin to go1.10.4 - Bump google/protobuf to fa252ec2, which picks up a fix for handling Windows path separators Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-09-28T21:07:38Z

It looks like this commit bazel-contrib/rules_go@3a4db22 breaks bazel build @envoy_api//envoy/.... Unfortunately, we need rules_go 0.14.0 or later for it to have actual Windows support. Will continue looking at this

This works around protoc-gen-go generating invalid go code Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-09-28T21:54:05Z

It looks like the issue is with how newer versions of protoc-gen-go handle a proto file importing string.proto (e.g.

envoy/api/envoy/type/matcher/value.proto

Line 7 in 9297104

import "envoy/type/matcher/string.proto";

).

Specifically, it will generate a value.pb.go file with an invalid import statement:

import string "github.com/envoyproxy/data-plane-api/api/string"

which will fail to compile.

I've pushed a commit that works around this by renaming string.proto to strings.proto, but I'm concerned this may be considered a breaking change as the filename in import / include statements would have to change. If so, we'll have to find another way to work around this

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch · 2018-10-02T15:34:39Z

@sesmith177 would you like to review/merge as is (to get this off the books) and then do a late fixup when the strings proto issue is resolved? Or just wait until then?

sesmith177 · 2018-10-02T17:52:26Z

@htuch not sure I follow -- are you suggesting we review/merge with the renaming of string.proto to strings.proto? Or just with the rules_go and google/protobuf bumps?

htuch · 2018-10-02T21:36:06Z

@sesmith177 why does protoc-gen-go now do this? This seems broken.

Technically I think the string(s) rename is safe as we don't change the package namespace; see https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md for discusion of when it is (un)safe to rename.

sesmith177 · 2018-10-02T21:59:29Z

It looks like the change in behavior occurred when moving from protoc-gen-go v1.0.0 -> v1.1.0; we have not determined what caused the change.

I agree that this behavior seems like a bug

htuch · 2018-10-02T22:01:20Z

@sesmith177 thanks - if it's a reasonable bug to fix in protoc-gen-go, then let's do it there. Otherwise, we can probably merge this PR without breaking too much.

sesmith177 · 2018-10-03T18:43:30Z

@htuch we submitted a PR to golang/protobuf: golang/protobuf#722

One thing to note is that it is difficult to bump this dependency in Envoy, as it does not have any BUILD files -- they are generated by rules_go when it brings in golang/protobuf. So if golang/protobuf#722 gets merged, we'll probably have to copy over some BUILD files from rules_go as well

This reverts commit 1491add. Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

This reverts commit ecf1bf9. Signed-off-by: Sam Smith <sesmith177@gmail.com>

- it was generating bad go code from the api protos Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

htuch · 2018-10-03T21:57:39Z

@sesmith177 can we also bump rules_go?

sesmith177 · 2018-10-03T22:11:29Z

sure, we'll bump it to 0.15.4

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

htuch · 2018-10-04T23:00:02Z

@sesmith177 I guess I was asking "Is there an ability to fix this upstream in rules_go so we don't need the giant patch?"

- this lets us access the patch files for com_github_golang_protobuf directly Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-10-05T15:35:08Z

@htuch it looks like if we bump rules_go forward past bazel-contrib/rules_go@fcfb5f5, we can reference their patch files directly without needing to carry our own

htuch

Awesome, this is getting smaller all the time :)

htuch · 2018-10-05T19:14:38Z

ci/WORKSPACE.filter.example

 load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_toolchains")
 go_rules_dependencies()
-go_register_toolchains()
+go_register_toolchains(go_version = "1.10.4")


Do we need to modify WORKSPACE here, is there any way to hide this under a macro in envoy_build_system.bzl? The reason I ask this is that any change here can be chaotic to consumer of Envoy who have their own custom WORKSPACEs. Ideally we make version details as opaque as possible.

htuch · 2018-10-05T19:14:56Z

bazel/repositories.bzl

+        _repository_impl(
+            name = "com_github_golang_protobuf",
+            patches = [
+                "@io_bazel_rules_go//third_party:com_github_golang_protobuf-gazelle.patch",


Woohoo! Can you add a comment on why these are needed and when we can get rid of the patches?

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch · 2018-10-05T20:00:28Z

@sesmith177 rad, ready to merge when CI completes.

rgs1 · 2018-10-10T00:43:20Z

@sesmith177 @htuch hmm, i am getting these errors after this change (on Linux):

17:38:18 ERROR: /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/envoy/bazel/repositories.bzl:55:13: //external:com_github_golang_protobuf: no such attribute 'patch_args' in 'git_repository' rule
17:38:18 ERROR: error loading package '': Encountered error while reading extension file 'bazel/repositories.bzl': no such package '@envoy_api//bazel': error loading package 'external': Could not load //external package
17:38:18 ERROR: error loading package '': Encountered error while reading extension file 'bazel/repositories.bzl': no such package '@envoy_api//bazel': error loading package 'external': Could not load //external package

It looks patch_args is available in newer bazel releases:

~/src/bazel (master) > git show 348225e4d25                                                                                                                                      
commit 348225e4d25b9259489c1ed66eb7eca7612cddcc
Author: Klaus Aehlig <aehlig@google.com>
Date:   Wed Jun 13 05:08:16 2018 -0700

    Skylark repositories: support additional arguments for the patch tool

    Instead of hard-coding "-p0", allow the arguments for that patch tool
    to be overridden. In particular, this supports the use case of patches
    generated with `git format-patch` which are to be read as `-p1`.

~/src/bazel (master) > git tag --contains 348225e4d25b9259489c1ed66eb7eca7612cddcc
0.16.0
0.16.1
0.17.1
0.17.2

whereas we the minimum bazel version for envoy is 0.15.0, no?

htuch · 2018-10-10T21:26:15Z

@rgs1 the minimum version for Envoy is the last point release, we move fast. You're going to have to bump.

rgs1 · 2018-10-10T21:28:55Z

@htuch ah fair. yeah — moved to 0.17.2 and life is good again. this:

https://github.com/envoyproxy/envoy/blob/master/ci/build_container/build_container_common.sh#L3

made me think 0.15.0 was being used... which i just realized refers to buildifier. Sorry for the confusion.

…y#4556) - Bump rules_go to 1.15.3 (latest). This picks up some fixes so that rules_go works on Windows. The api go tests don't appear to build with go1.11, so pin to go1.10.4 - Bump google/protobuf to fa252ec2, which picks up a fix for handling Windows path separators Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

moderation · 2018-10-11T13:13:59Z

@rgs1 that version number if only for buildifier. buildifier 0.17.2 has just been released and it looks like they are going to mirror Bazel releases. Will generate a PR soon to bump.

sesmith177 mentioned this pull request Sep 28, 2018

build: bazel changes to build on Windows #3786

Closed

Rename string.proto -> strings.proto

ecf1bf9

This works around protoc-gen-go generating invalid go code Signed-off-by: Sam Smith <sesmith177@gmail.com>

Fix docs

1491add

Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123 assigned htuch Sep 30, 2018

sesmith177 mentioned this pull request Oct 1, 2018

NT support #129

Closed

sesmith177 mentioned this pull request Oct 3, 2018

protoc-gen-go: predeclared identifiers in cleanPackageName golang/protobuf#722

Merged

sesmith177 and others added 3 commits October 3, 2018 16:57

Revert "Fix docs"

d2c3c84

This reverts commit 1491add. Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Revert "Rename string.proto -> strings.proto"

56f3ec2

This reverts commit ecf1bf9. Signed-off-by: Sam Smith <sesmith177@gmail.com>

Bump golang/protobuf to pick up fix

e30cbe7

- it was generating bad go code from the api protos Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Bump rules_go to 0.15.4

81ed492

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Bump rules_go to 3d966375

70eb158

- this lets us access the patch files for com_github_golang_protobuf directly Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch suggested changes Oct 5, 2018

View reviewed changes

Hide go version in repositories.bzl

0b2717e

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch approved these changes Oct 5, 2018

View reviewed changes

mattklein123 merged commit 1071264 into envoyproxy:master Oct 5, 2018

sesmith177 deleted the bump-deps-for-windows branch November 28, 2018 20:56

Conversation

sesmith177 commented Sep 28, 2018

Uh oh!

sesmith177 commented Sep 28, 2018

Uh oh!

sesmith177 commented Sep 28, 2018

Uh oh!

htuch commented Oct 2, 2018

Uh oh!

sesmith177 commented Oct 2, 2018

Uh oh!

htuch commented Oct 2, 2018

Uh oh!

sesmith177 commented Oct 2, 2018

Uh oh!

htuch commented Oct 2, 2018

Uh oh!

sesmith177 commented Oct 3, 2018

Uh oh!

htuch commented Oct 3, 2018

Uh oh!

sesmith177 commented Oct 3, 2018

Uh oh!

htuch commented Oct 4, 2018

Uh oh!

sesmith177 commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

htuch commented Oct 5, 2018

Uh oh!

rgs1 commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Oct 10, 2018

Uh oh!

rgs1 commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moderation commented Oct 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sesmith177 commented Oct 5, 2018 •

edited

Loading

rgs1 commented Oct 10, 2018 •

edited

Loading

rgs1 commented Oct 10, 2018 •

edited

Loading