Skip to content

bazel: disable -Wrange-loop-analysis on macOS#17428

Merged
snowp merged 1 commit intoenvoyproxy:mainfrom
keith:ks/bazel-disable-wrange-loop-analysis-on-macos
Jul 21, 2021
Merged

bazel: disable -Wrange-loop-analysis on macOS#17428
snowp merged 1 commit intoenvoyproxy:mainfrom
keith:ks/bazel-disable-wrange-loop-analysis-on-macos

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Jul 20, 2021

The version of clang that ships with Xcode 12 has false positives with
this warning that might be fixed by https://reviews.llvm.org/D73007

In the meantime we can disable it entirely as discussed on
#17393

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

The version of clang that ships with Xcode 12 has false positives with
this warning that might be fixed by https://reviews.llvm.org/D73007

In the meantime we can disable it entirely as discussed on
envoyproxy#17393

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@codefromthecrypt
Copy link
Copy Markdown
Contributor

cool if this works I will change Homebrew/homebrew-core#81490 to use a patch as this change looks a little hard to do as an inline edit

@codefromthecrypt
Copy link
Copy Markdown
Contributor

also if this works, please pick into the 1.18 branch as home-brew will drop 1.17 for 1.18 once 1.19 works 🙇

@keith
Copy link
Copy Markdown
Member Author

keith commented Jul 20, 2021

I think your --cxxopt there is a fine workaround in the meantime actually, easier than a patch

@codefromthecrypt
Copy link
Copy Markdown
Contributor

codefromthecrypt commented Jul 20, 2021

on its own the arg didn't work heh. then I tried removing the line which also didn't work. the latest commit both removes the line from config and adds the arg 🤷

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 self-assigned this Jul 20, 2021
@keith
Copy link
Copy Markdown
Member Author

keith commented Jul 20, 2021

Ah got it, @codefromthecrypt if you could test this with that patch before we land this that would be great just to verify it covers your case entirely

@codefromthecrypt
Copy link
Copy Markdown
Contributor

I will give it a try, sure. by patch, I meant a commit to cherry-pick, but I can probably find a way to embed a patch. Regardless, it will take 5 hours to find out if it works or not after applying it.

@codefromthecrypt
Copy link
Copy Markdown
Contributor

ok the patch is here. again it will likely be +5hrs to figure out if it works Homebrew/homebrew-core#81490

@codefromthecrypt
Copy link
Copy Markdown
Contributor

this works, though there are serious build performance issues that should be addressed at some point

@keith
Copy link
Copy Markdown
Member Author

keith commented Jul 21, 2021

this works, though there are serious build performance issues that should be addressed at some point

Great to hear! You mean on the homebrew side?

@keith
Copy link
Copy Markdown
Member Author

keith commented Jul 21, 2021

@mattklein123 i think this is ready to go then!

@snowp snowp merged commit eb157b7 into envoyproxy:main Jul 21, 2021
@codefromthecrypt
Copy link
Copy Markdown
Contributor

@keith I mean the build takes 5-8 hrs on a GHA runner. probably there is something that can be optimized. possibly less annoying if there was a way to know if lint would fail sooner also

@mattklein123
Copy link
Copy Markdown
Member

@keith I mean the build takes 5-8 hrs on a GHA runner. probably there is something that can be optimized

Unfortunately probably not. Building everything just takes a really long time on slow machines. There are 2 options:

  1. Build less stuff (we actually do this in macOS CI by stripping out a lot of extensions and tests).
  2. Move to a remote build farm for the macOS build. We are currently in trials to do this with EngFlow for the Envoy Mobile build and if successful I would like to do this for the main macOS builds also.

@codefromthecrypt
Copy link
Copy Markdown
Contributor

thanks for the context, @mattklein123. I'm hearing any optimizations like memory or how I/O works in the build steps are explored at this point. It is down to doing less or having something faster do it. got it.

homebrew core recipes require building from source, so it actually wouldn't benefit from another option which is to share existing builds (via tarballs #16830), so that less folks need to actually build it.

@codefromthecrypt
Copy link
Copy Markdown
Contributor

for context also it isn't so much about slow machines either. ex I have the latest and greatest MacBook and the formula takes over an hour and angrily buzzing. It is just that slower/shared/virt hosts typically used in CI are far worse and less reliable. Part of this is what you are used to.. ex if >1hr is considered slow

EOF as ack my comments are wandering this issue

@codefromthecrypt
Copy link
Copy Markdown
Contributor

#17442

@keith keith deleted the ks/bazel-disable-wrange-loop-analysis-on-macos branch July 22, 2021 00:21
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
The version of clang that ships with Xcode 12 has false positives with
this warning that might be fixed by https://reviews.llvm.org/D73007

In the meantime we can disable it entirely as discussed on
envoyproxy#17393

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants