Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use clang 12 #18220

Merged
merged 12 commits into from
Sep 28, 2021
Merged

use clang 12 #18220

merged 12 commits into from
Sep 28, 2021

Conversation

lizan
Copy link
Member

@lizan lizan commented Sep 22, 2021

Signed-off-by: Lizan Zhou lizan@tetrate.io

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18220 was opened by lizan.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18220 was opened by lizan.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 22, 2021
@lizan
use clang 12
8ad8a2d 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@moderation
Copy link
Contributor

Can we bump the Clang version at https://github.com/envoyproxy/envoy/blob/main/bazel/repository_locations.bzl#L902-L916 for consistency?

@lizan
format
1d67d21 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18220 was synchronize by lizan.

see: more, trace.

@moderation
Copy link
Contributor

It would be preferable to not introduce a patch. Since we control envoy-build-tools can this not be fixed upstream?

@lizan
Copy link
Member Author

lizan commented Sep 24, 2021

@moderation this is just a draft to test how this goes in CI, will see what will be the path forward.

@lizan
fix
6beae65 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
fix
adbd679 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
fix
b097594 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
Merge remote-tracking branch 'upstream/main' into clang_12_upgrade
9a12e92
@yanavlasov yanavlasov self-assigned this Sep 27, 2021
@yanavlasov
Copy link
Contributor

Coverage dip maybe due to tooling change? We can adjust the levels for now. @alyssawilk

@lizan
Copy link
Member Author

lizan commented Sep 27, 2021

Yes I think coverage dip is due to they change how lines get counted. I will dig it later.

@alyssawilk
Copy link
Contributor

If we need to bump we need to bump. The problem is that it looks like it's now considering comments to be code. It'd be lovely to fix if we can

see 115 for
https://storage.googleapis.com/envoy-pr/cb08fb4/coverage/source/common/api/posix/os_sys_calls_impl.cc.gcov.html
vs
https://storage.googleapis.com/envoy-postsubmit/main/coverage/source/common/api/posix/os_sys_calls_impl.cc.gcov.html

@lizan
Copy link
Member Author

lizan commented Sep 27, 2021

If we need to bump we need to bump. The problem is that it looks like it's now considering comments to be code. It'd be lovely to fix if we can

see 115 for
https://storage.googleapis.com/envoy-pr/cb08fb4/coverage/source/common/api/posix/os_sys_calls_impl.cc.gcov.html
vs
https://storage.googleapis.com/envoy-postsubmit/main/coverage/source/common/api/posix/os_sys_calls_impl.cc.gcov.html

This is an improvement? The main branch shows comments to be code and PR doesn't count that.

@lizan
-g instead of -g2
867de15 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
relatch coverage numbers
c8a8344 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
fix
edf7536 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@alyssawilk
Copy link
Contributor

yep, was reading it backwards.
As far as I can tell, old coverage considered comments code, new coverage does not, and the ratio of covered comments to non-covered comments makes a difference in aggregate :-P
I'm fine bumping the threshhold but if we can please only bump it for the project as a whole (but keep the bar for each directory at the 96.6) I think it'll be easier to gain that ground back.

@lizan
fix
0b9fcdc 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
Merge branch 'main' of https://github.com/envoyproxy/envoy into clang…
27d6a46 
…_12_upgrade
@lizan
update compiler-rt
f0c5938 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@yanavlasov
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18220 (comment) was created by @yanavlasov.

see: more, trace.

@lizan lizan marked this pull request as ready for review September 28, 2021 17:38
@lizan
Copy link
Member Author

lizan commented Sep 28, 2021

@htuch @moderation this is ready to go, about patches:

rules_cc patch posted to bazelbuild/rules_cc#115
envoy-build-tools patch, while we control the repo, though this is patching generated toolchain config which is coming from bazel itself and it is not easy to override how it is generated. If we move this to envoy-build tools it will be a patch -p1 in the generator script and isn't any better.

@yanavlasov
Copy link
Contributor

@htuch @moderation this is ready to go, about patches:

rules_cc patch posted to bazelbuild/rules_cc#115 envoy-build-tools patch, while we control the repo, though this is patching generated toolchain config which is coming from bazel itself and it is not easy to override how it is generated. If we move this to envoy-build tools it will be a patch -p1 in the generator script and isn't any better.

I can work with the RBE dev to make this aspect of toolchain generator configurable. Once this is done we can update toolchain generator. I see this patch as temporary.

@wrowe
Copy link
Contributor

wrowe commented Sep 28, 2021

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 28, 2021
@lizan lizan merged commit d60f86c into envoyproxy:main Sep 28, 2021
@lizan lizan deleted the clang_12_upgrade branch September 28, 2021 19:05
@yanavlasov yanavlasov mentioned this pull request Sep 28, 2021
Closed
lfpino added a commit to lfpino/envoy-mobile that referenced this pull request Sep 29, 2021
@lfpino
Upgrade envoyproxy/envoy-build-ubuntu docker image to fix the failure…
ebb993d 
…s caused by envoyproxy/envoy#18220 updating the run_bazel_coverage_script.sh

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>
@lfpino lfpino mentioned this pull request Sep 29, 2021
Merged
@alyssawilk
Copy link
Contributor

hey, did we envoy-dev announce this change? we should warn folks to install clang 12 no? Lizan if this makes sense can you do that?

@phlax phlax mentioned this pull request Sep 29, 2021
Closed
@wrowe
Copy link
Contributor

wrowe commented Sep 29, 2021

AIUI, the build tools are bumped to 12. The flags we've changed should keep clang-11 users largely compatible, there's nothing revolutionary in this change, so they should still be able to build locally with clang-11 except very specific platforms.

mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 29, 2021
@mpuncel
Merge branch 'main' into mpuncel/hc-after-sds-init-manager
66373d1 
* main: (114 commits)
  kafka: add header support to mesh-filter (envoyproxy#18248)
  rbac: add support for upstream ip policy. (envoyproxy#17645)
  SIPProxy BUGFIX UT failure for fastbuild/debug (envoyproxy#18303)
  quic: updating goaway code (envoyproxy#18291)
  various tiny fixes (envoyproxy#18287)
  dns cache: remove assert at this layer (envoyproxy#18301)
  [ext_authz]: ext_authz filter unit test that use real threading (envoyproxy#17742)
  signal action: fully disable sigaltstack on Apple (envoyproxy#18299)
  Add missing dependencies (envoyproxy#18297)
  ext_proc: Pass stream_info to gRPC streams (envoyproxy#18190)
  use clang 12 (envoyproxy#18220)
  Update PR template to include the "Fixes commit" message when reverting or fixing bad commits (envoyproxy#18298)
  [test] Fixing integration test to cleanup cleanly (envoyproxy#18293)
  test: moving grpc bridge tests out of core directory (envoyproxy#18227)
  runtime: disable deprecated extensions names by default (envoyproxy#18239)
  quiche: updating deps (envoyproxy#18272)
  sip_proxy: SIP protocol support in envoy (envoyproxy#18039)
  http: add core retry policy to route retry policy conversion utility (envoyproxy#17803)
  build: updating stale visibility (envoyproxy#18278)
  alternate_protocols_cache: Impose a max size limit on the alternate protocols cache (envoyproxy#18258)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
sergiitk
sergiitk reviewed Jan 26, 2022
View reviewed changes
api/envoy/config/filter/http/jwt_authn/v2alpha/config.proto
Comment on lines +393 to +394
map<string, JwtRequirement>
requires = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this change was accidental. FORCE_PROTO_FORMAT=yes ./tools/proto_format/proto_format.sh check complains about this.
(same applies to corresponding v3/config.proto)

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

@sergiitk sergiitk sergiitk left review comments

@yanavlasov yanavlasov yanavlasov approved these changes

Assignees

@yanavlasov yanavlasov

@htuch htuch

Labels
api v2-freeze
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lizan @moderation @yanavlasov @alyssawilk @wrowe @sergiitk @htuch