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

Envoy failed on Bazel CI with "unused variable 'factory_name'" #13079

Closed
meteorcloudy opened this issue Sep 14, 2020 · 21 comments · Fixed by #13203
Closed

Envoy failed on Bazel CI with "unused variable 'factory_name'" #13079

meteorcloudy opened this issue Sep 14, 2020 · 21 comments · Fixed by #13203
Labels
area/build

Comments

@meteorcloudy
Copy link
Contributor

https://buildkite.com/bazel/envoy/builds/1295#b92e2abd-a008-49b7-a929-2a5dde5a0152

bazel-out/k8-fastbuild/bin/include/envoy/registry/_virtual_includes/registry/envoy/registry/registry.h:348:44: error: unused variable 'factory_name' [-Werror=unused-variable]
     for (const auto& [factory_name, factory] : factories()) {
                                            ^
cc1plus: all warnings being treated as errors
@meteorcloudy meteorcloudy added bug triage Issue requires triage labels Sep 14, 2020
@meteorcloudy
Copy link
Contributor Author

/cc @lizan @htuch Can you please take a look, should be an easy fix?

@ratulb
Copy link

ratulb commented Sep 15, 2020

bazel-out/k8-opt/bin/include/envoy/registry/_virtual_includes/registry/envoy/registry/registry.h:348:44: error: unused variable 'factory_name' [-Werror=unused-variable]
for (const auto& [factory_name, factory] : factories()) {

@meteorcloudy
Copy link
Contributor Author

@ratulb Did you encounter the same build failure?

@RuijingGuo
Copy link

I created fix in #13125

@ratulb
Copy link

ratulb commented Sep 16, 2020 via email

@htuch
Copy link
Member

htuch commented Sep 16, 2020

@guoruijing @ratulb @meteorcloudy which compiler version is this? Has this just switched to on-by-default?

I think we should fix this structurally in Envoy so we don't have this come up again, probably by adding to envoy_copts in bazel/envoy_internal.bzl`, but maybe first by understanding why this is an issue for Bazel CI and not us.

@htuch htuch mentioned this issue Sep 16, 2020
Closed
@asraa
Copy link
Contributor

asraa commented Sep 16, 2020

@meteorcloudy Can you reproduce if you use gcc version 9?

@mattklein123 mattklein123 added area/build and removed bug triage Issue requires triage labels Sep 16, 2020
@RuijingGuo
Copy link

I can reproduce the issue with gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

@meteorcloudy
Copy link
Contributor Author

I believe we also use gcc 7.5 on Bazel CI. Somehow this unused variable is treated as an error on Bazel CI, but probably as a warning on circle CI?

@asraa
Copy link
Contributor

asraa commented Sep 17, 2020

I think GCC 9 is used in Envoy's CI https://github.com/envoyproxy/envoy-build-tools/blob/e7ea4e81bbd5028abb9d3a2f2c0afe063d9b62c0/build_container/build_container_ubuntu.sh#L12, is it possible that this is why?

@meteorcloudy
Copy link
Contributor Author

Sorry, I have no idea.

@ratulb
Copy link

ratulb commented Sep 18, 2020 via email

@RuijingGuo
Copy link

there are lots of locations using unused variables. I can change like followings or we just upgrade compiler version?

  • for (const auto& [factory_name, factory] : factories()) {
  • for (const auto& t: factories()) {
  •  const auto& factory = std::get<1>(t);

@asraa
Copy link
Contributor

asraa commented Sep 21, 2020 •
edited
Loading

We support and use GCC 9 in CI. But these docs https://github.com/envoyproxy/envoy/blob/4706ee01abe191eef73c64d86c1835baf566323c/bazel/README.md#supported-say compiler-versions currently has GCC 7+ as supported. There was some refactor after C++17 was enabled to use structured binding, which should be find since structured bindings appear in GCC 7.

However, I did find a GCC bug about this: https://gcc.gnu.org/bugzilla/show_bug.cgi?format=multiple&id=81767
that was fixed in https://gcc.gnu.org/git/?p=gcc.git&a=commit;h=6fc9f7aa731e895585c47d740509b5cd1591e797
I wonder if your compiler has the fix? It is in version 8.1, but not 7.5 from my github search.

I think I would suggest upgrading compiler version.
@envoyproxy/maintainers

@meteorcloudy
Copy link
Contributor Author

I don't think we can easily upgrade the gcc version on Bazel CI, other projects may still depend on the current version.
For fixing envoy on Bazel CI purpose, maybe you can disable this warning in the config script (.bazelci/presubmit.yml) or in the bazelrc file?
/cc @philwo

@meteorcloudy
Copy link
Contributor Author

FYI, according to https://buildkite.com/bazel/envoy/builds?branch=master&page=7,
this breakage was introduced between (da58cfc and defea7e ]

@asraa
Copy link
Contributor

asraa commented Sep 21, 2020

Yep, I'm assuming they were coming from these PRs
#12791
#12359
#12468

@mattklein123
Copy link
Member

IMO we should either downgrade Envoy GCC to match Bazel CI, or just do this:

For fixing envoy on Bazel CI purpose, maybe you can disable this warning in the config script (.bazelci/presubmit.yml) or in the bazelrc file?

I think ^ is probably simpler?

@ratulb
Copy link

ratulb commented Sep 21, 2020 via email

@lizan
Copy link
Member

lizan commented Sep 21, 2020

Perhaps we can simply remove GCC Bazel CI jobs, we do test RBE and Coverage (via local installed clang-10), IMO they are enough to catch issues from Bazel.

@lizan lizan mentioned this issue Sep 21, 2020
Merged
@philwo
Copy link

philwo commented Sep 21, 2020 •
edited
Loading

Bazel CI by design only installs tools and toolchains from packages supplied by a given distribution version and we do not upgrade these beyond what the distribution offers in their own repositories. This is so Bazel and other projects can verify that they are compatible with e.g. Ubuntu 18.04 LTS out of the box.

Thus, from Bazel CI's perspective this is working as intended: It alerted you to the fact that Envoy can no longer be built on Ubuntu 18.04 LTS out of the box. 😊

Whether this is fine or not, I can't decide - every project has their own guidelines regarding with which distributions they want to stay compatible. If you don't need to be compatible with Ubuntu 18.04 LTS, I would recommend to stop testing on that distro. Unfortunately Ubuntu 20.04 LTS is not yet available on Bazel CI, but I'll add it shortly: bazelbuild/continuous-integration#1035

@RuijingGuo RuijingGuo mentioned this issue Sep 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/build
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bazelci: remove gcc job envoyproxy/envoy
8 participants
@philwo @lizan @ratulb @meteorcloudy @asraa @mattklein123 @htuch @RuijingGuo