Skip to content

envoy 1.19.0#81490

Closed
codefromthecrypt wants to merge 2 commits intoHomebrew:masterfrom
codefromthecrypt:envoy-1.19.0
Closed

envoy 1.19.0#81490
codefromthecrypt wants to merge 2 commits intoHomebrew:masterfrom
codefromthecrypt:envoy-1.19.0

Conversation

@codefromthecrypt
Copy link
Copy Markdown
Contributor

Signed-off-by: Adrian Cole adrian@tetrate.io

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Jul 19, 2021
@carlocab
Copy link
Copy Markdown
Member

carlocab commented Jul 19, 2021

Please split your commits so that each commit modifies at most one formula. Thanks!

I'm also not certain about adding another versioned Envoy formula. One is fine, but two is probably a bit much, especially given that it works on only two out of five supported platforms.

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

arm isn't expected to work as nothing upstream improved on this see #79312 and related

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

@carlocab sounds good. so I will replace 1.17 with 1.18 that's fine with me.

@carlocab carlocab added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Jul 19, 2021
@carlocab
Copy link
Copy Markdown
Member

Looks like Envoy failed to build with the same error as in the previous PR. #81266

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

cool was waiting for a failed build on a branch with write access. likely Xcode related per some investigations from @mathetake

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

ps any clue on "The homepage URL https://www.envoyproxy.io is not reachable" could it be timeout related?

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

to fix the audit issue is easier, basically Envoy's Netlify is redirecting unless you hit something. Adding index.html will sort it out.

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

the problem is upstream compatibility with xcode 12 envoyproxy/envoy#17393

suppose we can figure out how to revert to an earlier version for these formulas as ^^ won't be in the source history until a later version of envoy

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this isn't needed as it doesn't run envoy's unit tests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this works around the xcode issue until it is fixed upstream

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

thanks especially to @mathetake as he helped form the syntax. I'm not knowledgeable on this part.

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

looks like an impossible combination.. we need to downgrade or delete this line https://github.com/envoyproxy/envoy/blob/main/bazel/envoy_internal.bzl#L69

@carlocab carlocab added the CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. label Jul 20, 2021
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

codefromthecrypt commented Jul 20, 2021

watching paint dry to test locally as the build takes so long, but here's my idea for patching the file

diff --git a/Formula/envoy.rb b/Formula/envoy.rb
index 471b5c5099..38887f2ef8 100644
--- a/Formula/envoy.rb
+++ b/Formula/envoy.rb
@@ -22,6 +22,8 @@ class Envoy < Formula
   depends_on macos: :catalina
 
   def install
+    # Work around xcode 12 incompatibility until envoyproxy/envoy#17393
+    inreplace "bazel/envoy_internal.bzl", '"-Wrange-loop-analysis",', ""
     args = %W[
       -c
       opt

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

at least in the build dir the above replacement worked locally even if the build won't finish for a few hours. I pushed here jic it finishes fine

@carlocab carlocab mentioned this pull request Jul 20, 2021
@codefromthecrypt codefromthecrypt dismissed a stale review via 04d40e7 July 20, 2021 22:10
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

trying both deleting the explicit entry for the linter from the config file and also adding its exclude as an arg. Maybe it is implicitly on by default. If this doesn't work and still no progress upstream...

maybe we temporarily downgrade to the previous version of xcode. this project is extremely complex in terms of flags and attempts cost 3-5hrs each depending on how fast your machine is, and with the build is running it takes it completely over

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

envoyproxy/envoy#17428 is the current approach to solve this

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

ok switched approach to an inline patch

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

I'm not sure the world record on slowest build, but one dimension is fast approaching 8hrs. 🍿

@SMillerDev
Copy link
Copy Markdown
Member

Unfortunately that one is taken by rust/go/python builds. Those often are over 24 hours 😅 .

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

one can dream..

@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

Successful in 481m!

ok this is now passing more than before. There's a patch aka tech debt, but frankly the upstream patch...

  1. isn't merged so would only effect a later version
  2. isn't guaranteed to be back ported

and regardless I'm on the hook at least for a while, so don't mind playing revert duty if/when above change

Copy link
Copy Markdown
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

We can use a patch from the upstream PR instead of carrying it around in the formula, I think.

@codefromthecrypt codefromthecrypt dismissed a stale review via f20d707 July 21, 2021 21:27
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

thanks for the help as usual @carlocab I will re-organize the commits per formula again and 🤞

codefromthecrypt and others added 2 commits July 22, 2021 07:29
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@codefromthecrypt
Copy link
Copy Markdown
Contributor Author

ok finally green!

Copy link
Copy Markdown
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Oops; this got buried under a bunch of other PRs. LGTM. Thanks, @codefromthecrypt!

@carlocab carlocab closed this in 02a017c Jul 23, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2021
@codefromthecrypt codefromthecrypt deleted the envoy-1.19.0 branch October 6, 2021 23:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge-skip `brew pr-automerge` will skip this pull request CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants