Skip to content

Update envoy SHA to 6/18#3387

Merged
istio-testing merged 11 commits into
istio:masterfrom
bianpengyuan:update-envoy-sha-0618
Jun 29, 2021
Merged

Update envoy SHA to 6/18#3387
istio-testing merged 11 commits into
istio:masterfrom
bianpengyuan:update-envoy-sha-0618

Conversation

@bianpengyuan
Copy link
Copy Markdown
Contributor

@bianpengyuan bianpengyuan commented Jun 25, 2021

No description provided.

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 25, 2021
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jun 25, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2021
@google-cla google-cla Bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jun 25, 2021
@bianpengyuan bianpengyuan marked this pull request as ready for review June 26, 2021 00:00
@bianpengyuan bianpengyuan requested review from a team June 26, 2021 00:00
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 26, 2021
@bianpengyuan
Copy link
Copy Markdown
Contributor Author

bianpengyuan commented Jun 26, 2021

@PiotrSikora @lambdai Any idea about why centos has range-loop-analysis check enabled? Should we turn it off? https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_proxy/3387/release-centos-test_proxy/1408575965637906432

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Jun 26, 2021 •
edited
Loading

Add a "build --cxxopt -Wno-range-loop-analysis" around https://github.com/istio/proxy/blob/master/.bazelrc#L51-L54 ?

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Jun 26, 2021

Add a "build --cxxopt -Wno-range-loop-analysis" around https://github.com/istio/proxy/blob/master/.bazelrc#L51-L54 ?

I want to make it clear the -Wno-range-loop-analysis is following "--cxxopt" and NOT in action_env

@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Jun 26, 2021

I wonder if the issue is outdated compiler and a false positive warning? It's not happening on Ubuntu nor upstream.

lambdai
lambdai reviewed Jun 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

Thanks! A few questions though

Comment thread extensions/common/util.h
Comment thread extensions/common/util.h Outdated
Comment thread extensions/common/wasm/json_util.h Outdated
Comment thread extensions/metadata_exchange/plugin.cc
@bianpengyuan bianpengyuan force-pushed the update-envoy-sha-0618 branch 2 times, most recently from e4058c1 to 76e939d Compare June 27, 2021 23:28
@bianpengyuan bianpengyuan force-pushed the update-envoy-sha-0618 branch from 76e939d to e958e8a Compare June 27, 2021 23:29
@bianpengyuan
Copy link
Copy Markdown
Contributor Author

bianpengyuan commented Jun 28, 2021 •
edited
Loading

Add a "build --cxxopt -Wno-range-loop-analysis" around https://github.com/istio/proxy/blob/master/.bazelrc#L51-L54 ?

I want to make it clear the -Wno-range-loop-analysis is following "--cxxopt" and NOT in action_env

@lambdai does not seem to work..

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Jun 28, 2021 

-Wno-range-loop-analysis -D_GLIBCXX_USE_CXX11_ABI=1'...
-Wrange-loop-analysis

Looks like conflict args are provided and the order matters.
Need to eliminate the latter one or place our override behind

bianpengyuan added a commit to istio/tools that referenced this pull request Jun 28, 2021
@bianpengyuan
Update LLVM/clang to 10.0 for centos
6bd96d4 
Try to see if compiler update could unblock envoy sha update istio/proxy#3387
@bianpengyuan bianpengyuan mentioned this pull request Jun 28, 2021
Merged
istio-testing pushed a commit to istio/tools that referenced this pull request Jun 28, 2021
@bianpengyuan
Update LLVM/clang to 10.0 for centos (#1589)
7972426 
Try to see if compiler update could unblock envoy sha update istio/proxy#3387
kyessenov
kyessenov reviewed Jun 28, 2021
View reviewed changes
Comment thread extensions/stackdriver/common/utils.cc
location->value()->string_view(), "/instances/", name);
return absl::StrCat(
"//compute.googleapis.com/projects/",
::Wasm::Common::toAbslStringView(project->value()->string_view()),
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov Jun 28, 2021

Choose a reason for hiding this comment

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

nit: doesn't absl StrCat handle std::string_view natively?

Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Jun 28, 2021

Choose a reason for hiding this comment

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

It doesn't - it supports absl::string_view, which by default is equivalent to std::string_view in C++17, but Envoy switched to non-standard null-terminated variants in envoyproxy/envoy#16029, so std::string_view is no longer supported.

Copy link
Copy Markdown
Contributor

@lambdai lambdai Jun 28, 2021

Choose a reason for hiding this comment

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

@PiotrSikora Could you clarify wrt null?
is it null-terminated or nullptr constructed?

Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Jun 28, 2021

Choose a reason for hiding this comment

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

Good catch - nullptr-constructed.

kyessenov
kyessenov approved these changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

I think Wasm needs to switch to absl string_view to make this cleaner.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 29, 2021
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 29, 2021
@istio-testing istio-testing merged commit 3a9cc09 into istio:master Jun 29, 2021
Shuanglu pushed a commit to Shuanglu/istio-tools that referenced this pull request Jun 30, 2022
@bianpengyuan
Update LLVM/clang to 10.0 for centos (istio#1589)
cef81a7 
Try to see if compiler update could unblock envoy sha update istio/proxy#3387
Shuanglu pushed a commit to Shuanglu/istio-tools that referenced this pull request Jul 6, 2022
@bianpengyuan
Update LLVM/clang to 10.0 for centos (istio#1589)
77f9e48 
Try to see if compiler update could unblock envoy sha update istio/proxy#3387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@PiotrSikora PiotrSikora PiotrSikora left review comments

@lambdai lambdai lambdai left review comments

@kyessenov kyessenov kyessenov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bianpengyuan @istio-testing @lambdai @PiotrSikora @kyessenov