Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Update protobuf rules for examples #203

Merged
merged 3 commits into from
Nov 20, 2018
Merged

Conversation

pcj
Copy link
Member

@pcj pcj commented Oct 24, 2018

  • Remove pubref/rules_protobuf
  • Add stackb/rules_proto
  • Update rules_go to 0.16.0

@pcj
Copy link
Member Author

pcj commented Oct 24, 2018

I've tested the individual server/client example implementations but did not run a full e2e test suite (hoping buildkite will do that for me). If passes, this PR will supercede #195 as this one updates rules_go to the desired version

cc @katre

@nlopezgi
Copy link
Contributor

thanks a lot for putting this together @pcj !
looks like //examples/hellohttp:deployment_test is failing, could you double check running it locally?
also kicked off buildkite tests that will allow us to get the full error log

@pcj
Copy link
Member Author

pcj commented Oct 24, 2018

/retest

All tests passing on my ubuntu 17 laptop

$ bazel test //...
INFO: Build completed successfully, 84 total actions

@pcj
Copy link
Member Author

pcj commented Oct 24, 2018

/retest

@katre
Copy link
Member

katre commented Oct 24, 2018

Thanks very much, @pcj !

Copy link
Contributor

@erain erain left a comment

Choose a reason for hiding this comment

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

Also please check the e2e tests since it is failing right now with the error msg:

  bazel-bin/examples/hellogrpc/cc/server/staging.describe
INFO: Elapsed time: 0.365s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
Error from server (NotFound): deployments.apps "hello-grpc-staging" not found

So I suspect it has something to do with the rename of build targets.

@nlopezgi
Copy link
Contributor

/retest
@erain I'm not sure the failure is related to the line
Error from server (NotFound): deployments.apps "hello-grpc-staging" not found
From what I could tell, this line is also present in passing runs:
https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/bazelbuild_rules_k8s/200/pull-rules-k8s-e2e/301/?log#log+
https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/bazelbuild_rules_k8s/193/pull-rules-k8s-e2e/299/?log#log

I dug a bit more in the log and the relevant error message is:
./examples/hellogrpc/e2e-test.sh: line 47: ./bazel-bin/examples/hellogrpc/go/client/client: No such file or directory
It looks like the new /examples/hellogrpc/go/client:client target is not producing the expected ./bazel-bin/examples/hellogrpc/go/client/client output, but instead its producing as output ./bazel-bin/examples/hellogrpc/go/client/linux_amd64_stripped/client (probably due to changes in rules_go).
The issue is the script expects the target to have that name here (https://github.com/bazelbuild/rules_k8s/blob/master/examples/hellogrpc/e2e-test.sh#L47).

Note that for the cc, java and py hellogrpc samples the output is indeed the expected ./bazel-bin/examples/hellogrpc/${LANGUAGE}/client/client so changing that line for:
OUTPUT=$(./bazel-bin/examples/hellogrpc/${LANGUAGE}/client/linux_amd64_stripped/client $(get_lb_ip))
does not work as it breaks execution with cc, java and py (but succeeds for go)

Should be feasible to have some hack in the script to add the linux_amd64_stripped only if language is go.

@pcj
Copy link
Member Author

pcj commented Oct 24, 2018

Thanks @nlopezgi for looking into this; that makes sense. The workaround is similar to bazel-contrib/rules_go#1239 (comment) that I'll address.

@nlopezgi
Copy link
Contributor

thanks @pcj , the workaround proposed there should work fine here. I'll kick off buildkite once you update your commit and do then a full review (also please address the comments made by @erain). Thanks again.

@nlopezgi
Copy link
Contributor

this is still failing,
looks like the normalized rule is producing
bazel-genfiles/examples/hellogrpc/go/client/client
but the script is looking for
./bazel-bin/examples/hellogrpc/go/client/client

Previous protobuf rules used by example gRPC server/client
implementations did not use proto_library.  This change updates
to these rules (compatible with newer rules_go)

Upgrade to rules_go 0.16.1.  Previously, the e2e_test.sh assumed
the name of the generated go_binary matched the rule name, but
with changes introduced for cross-compilation, that assumption no
longer held true.  A genrule has been introduced to fix that.
@pcj
Copy link
Member Author

pcj commented Oct 24, 2018

You are too fast and too right sir! Added output_to_bindir. In resolving @erain comment I took the liberty of updating to rules_go 0.16.1. PR squashed to a single commit and force pushed.

@nlopezgi
Copy link
Contributor

hi @pcj ,
we've fixed some of the flakyness issues with our e2e tests if you want to try to update this branch so we can get it in. Let me know if you dont think you'll have the time, if so we'll try to find someone else that can pick up and finish it.
Thanks!

@nlopezgi
Copy link
Contributor

Hi @pcj ,
Im going to try to fix this PR and get it merged as its blocking #227

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlopezgi, pcj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nlopezgi nlopezgi merged commit c839dce into bazelbuild:master Nov 20, 2018
@nlopezgi
Copy link
Contributor

@pjc, thanks again for putting this pr together

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

Successfully merging this pull request may close these issues.

5 participants