api: generate c++ grpc stubs #3857

bplotnick · 2018-07-14T00:57:32Z

Signed-off-by: Ben Plotnick [email protected]

Description: Generate c++ grpc stubs
This passes use_grpc_plugin to pgv_cc_proto_library, which enables c++ grpc stubs to be generated. There are some necessary binds that need to be done unfortunately. This would be easier if we could do load("@com_github_grpc_grpc//bazel:grpc_deps", "grpc_deps"), but you can't load within a macro. I'm not sure if there's a better way to do this.

Note that bazel targets depping the _cc targets will also have to add these binds. Generally, they can call grpc_deps() as I mentioned before. But perhaps this should go in api_dependencies instead so that users can use that?

Risk Level: Low
Testing: We've been using this internally for a little while now and it works.

Signed-off-by: Ben Plotnick <[email protected]>

mattklein123 · 2018-07-14T20:37:07Z

@bplotnick can you merge master? There were flake fixes that went in.

Signed-off-by: Ben Plotnick <[email protected]>

bplotnick · 2018-07-15T04:09:46Z

So i forgot to run the api tests (doh) and i think this is going to need a bit more work. The problem is the one I alluded to in the description. Now @envoy_api directly depends on com_github_grpc_grpc. The idea I had of moving grpc into api_dependencies is not going to quite work because of the way we dep libssl and cares.

htuch · 2018-07-15T20:42:29Z

@bplotnick why can't you do the load in https://github.com/envoyproxy/envoy/blob/master/api/bazel/repositories.bzl (at the top) and then make use of the grpc_deps in api_dependencies()?

bplotnick · 2018-07-16T01:59:53Z

@htuch, that seems to make the bazel.api tests pass, but i'm confused as to how this works. The com_github_grpc_grpc external dep shouldn't actually be present in the @envoy_api workspace, since it is added in the @envoy workspace. What would happen for direct users of data-plane-api? Would they need to manually add the com_github_grpc_grpc external dep into their workspace?

The reason I didn't go down this route is that i thought I would've had to add the external dep in the same macro as the load. When i tried doing this, I ran into the same problem detailed here: bazelbuild/bazel#1550

My current approach that i'm trying (about to commit) is to move the _com_github_grpc_grpc() macro into the api and then do conditional loading for cares and libssl like is done in grpc_deps, but not use that function directly because (as mentioned above) i can't.

Btw, I'm still pretty new to Bazel, so please feel free to correct me if my terminology or understanding is wrong. Also, i'm happy to discuss this on Slack if you prefer that 😄

Signed-off-by: Ben Plotnick <[email protected]>

bplotnick · 2018-07-19T07:36:12Z

@htuch I had another go at this and managed to get it working with the strategy I laid out before (moving the grpc dependency stuff to api_dependencies).

I pulled cares out of prebuilt and now build it in bazel using the BUILD file that grpc provides. It was failing for -Wold-style-cast, so i put a bunch of pragmas in place where we use the preprocessor macros. It's pretty ugly (and made uglier by check_format dedenting the pragmas) and i'd like to do this a different way, but i'm not sure how yet.

I also bumped grpc to 0.13.1 to pick up a workaround for bazel build failures on mac for grpc (grpc/grpc#13856). It required me to add -DGRPC_BAZEL_BUILD copt.

Lemme know what you think.

htuch · 2018-07-23T01:03:29Z

test/common/network/dns_impl_test.cc

-        // Only expecting a single question.
+// Only expecting a single question.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wold-style-cast"


Yeah, this seems pretty ugly. Can we fix the gRPC c-ares BUILD to do something similar to what we did in https://github.com/envoyproxy/envoy/blob/master/ci/prebuilt/BUILD#L9? This handles the includes as system includes IIRC, which avoids Wold-style-cast.

I wasn't sure how to get these as -isystem. I thought that what the grpc cares BUILD does should have done the trick, but apparently not.

htuch · 2018-07-23T01:05:35Z

api/bazel/repositories.bzl

+
+    _com_github_grpc_grpc()
+
+def _com_github_grpc_grpc():


So, what you've done is push a bunch of deps down into api/bazel, since we can then have a single API related setup Skylark function. This, to me, seems totally reasonable. the downside is we lose some of the nice machinery for managing repository locations that @jmillikin-stripe setup there. Ideally we also port that back into api/bazel, but that might be too much unrelated work for this PR. @jmillikin-stripe do you have any opinions as Envoy Bazel owner?

jmillikin-stripe · 2018-07-23T17:54:39Z

the downside is we lose some of the nice machinery for managing repository locations that @jmillikin-stripe setup there.

Can't we just move or copy the repository_locations stuff into api/bazel/? It's not a lot of code and isn't especially complex. As long as the Envoy folks want to keep the external dependency URLs declarative, I think it makes sense for the API build to match.

bplotnick · 2018-07-24T18:07:03Z

@jmillikin-stripe I don't want to expand the scope of this PR so i'll tackle copying the build logic to api/bazel in separate PR.

Do you happen to know a better solution for the -Wold-style-cast issues?

jmillikin-stripe · 2018-07-24T18:25:36Z

Does Clang stop warning about old-style casts if you use #include <ares.h> and #include <ares_dns.h>?

Signed-off-by: Ben Plotnick <[email protected]>

bplotnick · 2018-07-25T19:26:33Z

@jmillikin-stripe that seemed to do the trick, thanks!

htuch · 2018-07-25T21:50:36Z

Yeah, that works. It's a bit ugly; we have inconsistent treatment of headers in include style. Can you add a TODO to fix this? Also, please at least add a TODO for porting the repository_locations.bzl infrastructure to bazel/api. Thanks.

stale · 2018-08-01T22:29:39Z

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!

Signed-off-by: Ben Plotnick <[email protected]>

bplotnick · 2018-08-08T17:50:09Z

@htuch I finally got back to this PR. I added one TODO for porting the repository_locations.bzl logic, and I went ahead and made the ares includes consistently be system includes.

htuch

Great, looks like there is merge conflicts and need to reconcile with #4047.

htuch · 2018-08-08T20:48:51Z

test/common/network/dns_impl_test.cc

@@ -1,3 +1,5 @@
+#include <ares.h>


Still not a big fan of this; it's inconsistent with the way we treat all other external dependencies, and the user might easily believe its coming from the system vs. the gRPC external dependency. Is there no way we can improve this (@jmillikin-stripe)? If not, we should put comments at each of these include sites to explain what's going on and the nature of this exception..

Do you think it'd make sense to use #include <> style for all external dependencies? The presence of "local"-style includes of external deps is fairly strange for C++ code, and IMO is an unwanted abstraction leak from the build system into the code being built.

htuch · 2018-08-08T20:51:04Z

bazel/target_recipes.bzl

@@ -2,7 +2,6 @@
 # target in //ci/prebuilt/BUILD to the underlying build recipe in
 # ci/build_container/build_recipes.
 TARGET_RECIPES = {
-    "ares": "cares",


Should we also remove https://github.com/envoyproxy/envoy/blob/master/ci/build_container/build_recipes/cares.sh?

htuch · 2018-08-08T20:52:09Z

api/bazel/repositories.bzl

+        actual = "@com_github_cares_cares//:ares",
+    )
+
+    if "com_github_cares_cares" not in native.existing_rules():


What do you think about the @jmillikin-stripe ask regarding setting up the repository_locations.bzl infrastructure in api/bazel as well as in bazel?

htuch · 2018-08-08T20:53:00Z

api/bazel/repositories.bzl

@@ -1,5 +1,6 @@
 GOOGLEAPIS_SHA = "d642131a6e6582fc226caf9893cb7fe7885b3411"  # May 23, 2018
 GOGOPROTO_SHA = "1adfc126b41513cc696b209667c8656ea7aac67c"  # v1.0.0
+GRPC_SHA = "befc7220cadb963755de86763a04ab6f9dc14200"  # v1.13.1


@lizan recently bumped to 1.14. This also adds nanopb as a dep, see #4047.

stale · 2018-08-15T21:35:40Z

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 · 2018-08-22T21:49:18Z

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!

generate c++ grpc stubs

70c5c53

Signed-off-by: Ben Plotnick <[email protected]>

mattklein123 assigned htuch and jmillikin-stripe Jul 14, 2018

Merge branch 'master' into generate-cpp-grpc

dcd1222

Signed-off-by: Ben Plotnick <[email protected]>

bplotnick and others added 5 commits July 18, 2018 19:18

move grpc build into api

bdc3fb2

Signed-off-by: Ben Plotnick <[email protected]>

Merge branch 'master' into generate-cpp-grpc

c033780

fix format

226222f

Signed-off-by: Ben Plotnick <[email protected]>

move -DGRPC_BAZEL_BUILD out into bazel.rc

28a13d5

Signed-off-by: Ben Plotnick <[email protected]>

remove cares build recipe

3e51393

Signed-off-by: Ben Plotnick <[email protected]>

htuch reviewed Jul 23, 2018

View reviewed changes

htuch requested a review from jmillikin-stripe July 23, 2018 01:05

bplotnick added 2 commits July 25, 2018 11:23

use system included ares

ce7f6dd

Signed-off-by: Ben Plotnick <[email protected]>

Merge branch 'master' into generate-cpp-grpc

025e50f

Signed-off-by: Ben Plotnick <[email protected]>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 1, 2018

bplotnick added 2 commits August 7, 2018 22:13

Merge branch 'master' into generate-cpp-grpc

d581977

Signed-off-by: Ben Plotnick <[email protected]>

add TODO and use system includes for ares

1ae4006

Signed-off-by: Ben Plotnick <[email protected]>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2018

htuch suggested changes Aug 8, 2018

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 15, 2018

stale bot closed this Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: generate c++ grpc stubs #3857

api: generate c++ grpc stubs #3857

bplotnick commented Jul 14, 2018

mattklein123 commented Jul 14, 2018

bplotnick commented Jul 15, 2018

htuch commented Jul 15, 2018 •

edited

Loading

bplotnick commented Jul 16, 2018

bplotnick commented Jul 19, 2018

htuch Jul 23, 2018

bplotnick Jul 23, 2018

htuch Jul 23, 2018

jmillikin-stripe commented Jul 23, 2018

bplotnick commented Jul 24, 2018

jmillikin-stripe commented Jul 24, 2018

bplotnick commented Jul 25, 2018

htuch commented Jul 25, 2018

stale bot commented Aug 1, 2018

bplotnick commented Aug 8, 2018

htuch left a comment

htuch Aug 8, 2018

jmillikin-stripe Aug 8, 2018

htuch Aug 8, 2018

htuch Aug 8, 2018

htuch Aug 8, 2018

stale bot commented Aug 15, 2018

stale bot commented Aug 22, 2018

api: generate c++ grpc stubs #3857

api: generate c++ grpc stubs #3857

Conversation

bplotnick commented Jul 14, 2018

mattklein123 commented Jul 14, 2018

bplotnick commented Jul 15, 2018

htuch commented Jul 15, 2018 • edited Loading

bplotnick commented Jul 16, 2018

bplotnick commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin-stripe commented Jul 23, 2018

bplotnick commented Jul 24, 2018

jmillikin-stripe commented Jul 24, 2018

bplotnick commented Jul 25, 2018

htuch commented Jul 25, 2018

stale bot commented Aug 1, 2018

bplotnick commented Aug 8, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 15, 2018

stale bot commented Aug 22, 2018

htuch commented Jul 15, 2018 •

edited

Loading