Remove _xds suffix Bazel rules completely#25980
Conversation
markdroth
left a comment
There was a problem hiding this comment.
This approach looks much cleaner! Just one minor comment.
Reviewed 5 of 5 files at r1.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @gnossen, @jtattermusch, @lidizheng, @markdroth, @nicolasnoble, and @veblush)
test/cpp/end2end/admin_services_end2end_test.cc, line 87 at r1 (raw file):
#endif // GRPC_NO_XDS or DISABLED_XDS_PROTO_IN_CC #if defined(GRPC_NO_XDS) || defined(DISABLED_XDS_PROTO_IN_CC)
Suggest just using #else here.
lidizheng
left a comment
There was a problem hiding this comment.
Thanks for the review! PTALA.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)
test/cpp/end2end/admin_services_end2end_test.cc, line 87 at r1 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest just using
#elsehere.
In the test body, if any of the two flags defined, make sure CSDS is not registered; otherwise, check if CSDS is one of the registered services. This is done by the #else statement below.
markdroth
left a comment
There was a problem hiding this comment.
Looks great!
Reviewed 3 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gnossen, @jtattermusch, @nicolasnoble, and @veblush)
|
Ruby MacOS failed with C++ Opt MacOS: No log; probably didn't run at all. |
As title. Most logic will be in google3, see cl/368529309.
We added "_xds" suffix Bazel rules, because we hope to have a clean separation between specially treated targets and normal targets. However,
grpcpp_csdsis going to be depended by many tests and binaries, existing solution requires people to remember changing the normal rule to "_xds" ones, WHENEVER the target transitively depends ongrpcpp_csds. This is very error-prone.Besides, Bazel rules
grpc_cc_library/grpc_cc_testare exempted from strict headers check: https://source.corp.google.com/piper///depot/google3/java/com/google/devtools/build/lib/view/cpp/LayeringCheckAllowlists.java?q=grpc_cc_library. The new "_xds" Bazel rules ain't, and will trigger following error:So, this PR:
DISABLED_XDS_PROTO_IN_CCasGRPC_NO_XDSin admin servicesadmin_services_end2end_test_xdsBazel rules (internally, existing logics are merged into normal Bazel rules)This change is