Skip to content

use envoy_google_grpc_external_deps for google_grpc_context_lib#8380

Merged
htuch merged 3 commits intoenvoyproxy:masterfrom
qiwzhang:target_gce
Sep 26, 2019
Merged

use envoy_google_grpc_external_deps for google_grpc_context_lib#8380
htuch merged 3 commits intoenvoyproxy:masterfrom
qiwzhang:target_gce

Conversation

@qiwzhang
Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang commented Sep 26, 2019

Signed-off-by: Wayne Zhang qiwzhang@google.com

Directly use "external_deps" in google_grpc_context_lib causing
source/exe:envoy_main_common_lib not able to support target:gce

Description:
Risk Level: None
Testing: None
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Sep 26, 2019

@jmarantz what do you think?

jmarantz
jmarantz previously approved these changes Sep 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz 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 doing this; I'm not exactly sure why we needed this as in OSS it builds without it, but this makes sense.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 26, 2019

Please merge master to fix CI.

/wait

@qiwzhang
use envoy_google_grpc_external_deps for google_grpc_context_lib
b3fc37a 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
fix format
7323256 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
wrap grpc code with ENVOY_GOOGLE_GRPC
3ecaaca 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Sep 26, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 26, 2019

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8380 (comment) was created by @qiwzhang.

see: more, trace.

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Sep 26, 2019

It looks like the "bazel.release" test failed for this test:

//test/extensions/access_loggers/grpc:http_config_test

In my local,. it passed "bazel test //test/extensions/access_loggers/grpc:http_config_test "

But I could not run "bazel.releae" in my local with docker. I got this error:

./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.release'
No remote cache bucket is set, skipping setup remote cache.
ENVOY_SRCDIR=/source
fatal: Not a git repository (or any parent up to mount point /build)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Sep 26, 2019

@jmarantz @htuch, @lizan could you re-test it to see if this is a flaky test?

@htuch htuch merged commit 6d77245 into envoyproxy:master Sep 26, 2019
@qiwzhang qiwzhang deleted the target_gce branch September 26, 2019 21:41
mattklein123
mattklein123 reviewed Sep 26, 2019
View reviewed changes
source/common/grpc/google_grpc_context.cc
#include "common/common/macros.h"
#include "common/common/thread.h"

#ifdef ENVOY_GOOGLE_GRPC
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 26, 2019

Choose a reason for hiding this comment

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

Sorry why is this needed again? I thought we are not including this class unless Google gRPC is enabled? @jmarantz @lizan ?

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Sep 26, 2019

Choose a reason for hiding this comment

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

I did not understand why this was needed either. When I approved this PR this change had not been made yet.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 26, 2019

Choose a reason for hiding this comment

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

@qiwzhang can you do a follow up to revert this part? Thank you.

Copy link
Copy Markdown
Contributor Author

@qiwzhang qiwzhang Sep 26, 2019

Choose a reason for hiding this comment

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

Sorry, #8407 will revert this change

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Sep 26, 2019

Also @qiwzhang please don't force-push or squash commits. They drop history and comments made during review.

qiwzhang added a commit to qiwzhang/envoy that referenced this pull request Sep 26, 2019
@qiwzhang
Revert "use envoy_google_grpc_external_deps for google_grpc_context_l…
bb3efb8 
…ib (envoyproxy#8380)"

This reverts commit 6d77245.
@qiwzhang qiwzhang mentioned this pull request Sep 26, 2019
Closed
qiwzhang added a commit to qiwzhang/envoy that referenced this pull request Sep 26, 2019
@qiwzhang
Revert "use envoy_google_grpc_external_deps for google_grpc_context_l…
6f09080 
…ib (envoyproxy#8380)"

This reverts commit 6d77245.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang mentioned this pull request Sep 26, 2019
Merged
htuch pushed a commit that referenced this pull request Sep 27, 2019
@qiwzhang @htuch
Revert "use envoy_google_grpc_external_deps for google_grpc_context_l…
555f88b 
…ib (#8380)" (#8407)

This reverts commit 6d77245.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@qiwzhang
use envoy_google_grpc_external_deps for google_grpc_context_lib (envo…
a752bec 
…yproxy#8380)

Directly use "external_deps" in google_grpc_context_lib causing a problem during Google import rewriting.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@qiwzhang
Revert "use envoy_google_grpc_external_deps for google_grpc_context_l…
77a6430 
…ib (envoyproxy#8380)" (envoyproxy#8407)

This reverts commit 6d77245.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmarantz jmarantz jmarantz left review comments

@mattklein123 mattklein123 mattklein123 left review comments

+1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmarantz jmarantz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@qiwzhang @mattklein123 @jmarantz @htuch