Skip to content

Remove _xds suffix Bazel rules completely#25980

Merged
lidizheng merged 5 commits intogrpc:masterfrom
lidizheng:remove-xds-suffix
Apr 15, 2021
Merged

Remove _xds suffix Bazel rules completely#25980
lidizheng merged 5 commits intogrpc:masterfrom
lidizheng:remove-xds-suffix

Conversation

@lidizheng
Copy link
Copy Markdown
Contributor

@lidizheng lidizheng commented Apr 14, 2021

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_csds is 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 on grpcpp_csds. This is very error-prone.

Besides, Bazel rules grpc_cc_library/grpc_cc_test are 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:

ERROR: /google/src/cloud/lidiz/github-sync-20210414144431/google3/third_party/grpc/BUILD:2647:20: in cc_library rule //third_party/grpc:grpcpp_admin: C++ rules should use 'strict' hdrs_check (not 'loose' or 'warn').The hdrs_check='strict' ensures that all #included files must be explicitly declared somewhere in the hdrs or srcs of the rules providing the libraries or the rules containing the including source. Please remove 'hdrs_check' attribute from this rule, as well as the 'default_hdrs_check' attribute from the package, or set them to 'strict'. If you think your use case qualifies for an exception, please see go/blaze-strict-layering-exception

So, this PR:

  • Treat DISABLED_XDS_PROTO_IN_CC as GRPC_NO_XDS in admin services
  • Simplified the preprocessing logic in admin_services_end2end_test
  • Remove all _xds Bazel rules (internally, existing logics are merged into normal Bazel rules)

This change is Reviewable

@lidizheng lidizheng added lang/c++ release notes: no Indicates if PR should not be in release notes labels Apr 14, 2021
@lidizheng lidizheng marked this pull request as ready for review April 15, 2021 00:17
@lidizheng lidizheng requested a review from markdroth April 15, 2021 00:17
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

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 #else here.

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.

Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

@lidizheng
Copy link
Copy Markdown
Contributor Author

Ruby MacOS failed with

++ gpg --keyserver hkp://86.105.108.212 --recv-keys 409B6B1796C275462A1703113804BB82D39DC0E3 7D2BAF1CF37B13E2069D6956105BD0E739499BDB
gpg: directory '/Users/kbuilder/.gnupg' created
gpg: keybox '/Users/kbuilder/.gnupg/pubring.kbx' created
gpg: keyserver receive failed: No data

C++ Opt MacOS: No log; probably didn't run at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lang/c++ release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants