Skip to content

#23533 + move upbdefs.h and upbdefs.c under upbdefs-generated#23835

Closed
jtattermusch wants to merge 23 commits intogrpc:masterfrom
jtattermusch:xds_logging_additions
Closed

#23533 + move upbdefs.h and upbdefs.c under upbdefs-generated#23835
jtattermusch wants to merge 23 commits intogrpc:masterfrom
jtattermusch:xds_logging_additions

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Aug 14, 2020

Based on #23533

Cleaner version of #23634

both upbdefs.h and upbdefs.c files will end up under src/core/ext/upbdefs-generated/

markdroth and others added 15 commits July 17, 2020 14:48
@markdroth
Revert "Merge pull request grpc#23524 from markdroth/xds_logging_revert"
5afa417 
This reverts commit b5b9172, reversing
changes made to 47c6c82.
@markdroth
Merge remote-tracking branch 'upstream/master' into xds_logging
a24306f
@markdroth
Merge remote-tracking branch 'upstream/master' into xds_logging
c66d826
@markdroth
add dep.hpp dependency
09cdfcd
@markdroth
clang-format
ad01616
@markdroth
Merge remote-tracking branch 'upstream/master' into xds_logging
f88449c
@markdroth
clang-format
ab98271
@markdroth
Merge remote-tracking branch 'upstream/master' into xds_logging
478ea19
@jtattermusch
run tools/codegen/core/gen_upb_api.sh
7c49354
@jtattermusch
put .upbdefs.c files in a different directory
7530552
@jtattermusch
also move upbdefs.h files
dd5937c
@jtattermusch
run tools/codegen/core/gen_upb_api.sh
c16056f
@jtattermusch
update BUILD and src/upb/gen_build_yaml.py with new location of .upbd…
b62e4d7 
…efs.c files
@jtattermusch
update BUILD and src/upb/gen_build_yaml.py with new location of .upbd…
928793a 
…efs.h files
@jtattermusch
add src/core/ext/upbdefs-generated include to bazel build
84685b4
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 14, 2020

Likely not all the build systems are passing yet, because of the extra include path (src/core/ext/upbdefs-generated/) - but I'll fix them as needed.

@jtattermusch jtattermusch added lang/c++ release notes: no Indicates if PR should not be in release notes labels Aug 14, 2020
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 14, 2020

CC @stanley-cheung @markdroth once things are fully green here, I'll create a PR against #23573 (or whatever seems to be the easiest way forward).

This was referenced Aug 14, 2020
Closed
Closed
@jtattermusch jtattermusch force-pushed the xds_logging_additions branch from e129a2e to 71d2059 Compare August 19, 2020 08:35
@jtattermusch jtattermusch mentioned this pull request Aug 19, 2020
Closed
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 19, 2020 •
edited
Loading

Most builds are passing, still need to fix iOS builds:
https://source.cloud.google.com/results/invocations/289ec031-978a-42f9-b577-d9ac8730c97b/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-cpp-test-cronet/tests

Wed Aug 19 03:00:50 PDT 2020 - In file included from /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/test/cpp/ios/Pods/Headers/Public/Protobuf-C++/google/protobuf/map_entry_lite.h:42:
Wed Aug 19 03:00:50 PDT 2020 - /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/test/cpp/ios/Pods/Headers/Public/Protobuf-C++/google/protobuf/generated_message_util.h:50:10: fatal error: 'google/protobuf/has_bits.h' file not found
Wed Aug 19 03:00:50 PDT 2020 - #include <google/protobuf/has_bits.h>
Wed Aug 19 03:00:50 PDT 2020 -          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Wed Aug 19 03:00:50 PDT 2020 - 1 error generated.

https://source.cloud.google.com/results/invocations/266284ba-939b-4375-9291-55e299066ff6/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-test-cfstream-tests/tests ( this failure might be "ok" because the same failure is already on #23533).

Also need to fix the check_upb_output.sh script

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 20, 2020

ObjC builds seems fine:

Most builds are passing, still need to fix iOS builds:
https://source.cloud.google.com/results/invocations/289ec031-978a-42f9-b577-d9ac8730c97b/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-cpp-test-cronet/tests

Wed Aug 19 03:00:50 PDT 2020 - In file included from /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/test/cpp/ios/Pods/Headers/Public/Protobuf-C++/google/protobuf/map_entry_lite.h:42:
Wed Aug 19 03:00:50 PDT 2020 - /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/test/cpp/ios/Pods/Headers/Public/Protobuf-C++/google/protobuf/generated_message_util.h:50:10: fatal error: 'google/protobuf/has_bits.h' file not found
Wed Aug 19 03:00:50 PDT 2020 - #include <google/protobuf/has_bits.h>
Wed Aug 19 03:00:50 PDT 2020 -          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Wed Aug 19 03:00:50 PDT 2020 - 1 error generated.

Looks like this is a random flake unrelated to this PR, we've seen in on master in other situations (see b/161998115)

https://source.cloud.google.com/results/invocations/266284ba-939b-4375-9291-55e299066ff6/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-test-cfstream-tests/tests ( this failure might be "ok" because the same failure is already on #23533).

This already happens on #23573

Also need to fix the check_upb_output.sh script

This still needs to be done.

@jtattermusch jtattermusch force-pushed the xds_logging_additions branch from b832dd0 to 4b96f51 Compare August 20, 2020 10:56
@jtattermusch jtattermusch marked this pull request as ready for review August 20, 2020 10:57
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 20, 2020

@markdroth the commits from this PR should be ready to bring into #23533 (I needed to merge upstream/master to resolve conflicts, but the merge commits don't really have any important logic).

I can create a PR against #23533 if you want (but looks like upb generated files in #23533 are out of date).

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 20, 2020

clang tidy complaint (not sure if that's bad news, but seems easily fixable):

Error while processing /local-code/src/core/ext/xds/xds_api.cc.
/local-code/src/core/ext/xds/xds_api.cc:48:10: error: 'envoy/config/cluster/v3/cluster.upbdefs.h' file not found [clang-diagnostic-error]
#include "envoy/config/cluster/v3/cluster.upbdefs.h"
         ^
/local-code/src/core/ext/xds/xds_api.cc:862:52: error: the parameter 'domain_pattern' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
bool DomainMatch(MatchType match_type, std::string domain_pattern,
                                       ~~~         ^
                                       const      &
/local-code/src/core/ext/xds/xds_api.cc:863:30: error: the parameter 'expected_host_name' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
                 std::string expected_host_name) {
                 ~~~         ^
                 const      &
Suppressed 1164 warnings (1164 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings treated as errors
FAILED: src/core/ext/xds/xds_api.cc [ret=2, pid=364, time=2.5sec]

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 21, 2020

superseded by #23904

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

Reviewers

@apolcyn apolcyn Awaiting requested review from apolcyn

@gnossen gnossen Awaiting requested review from gnossen

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

@veblush veblush Awaiting requested review from veblush

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jtattermusch @markdroth