-
-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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.
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
Envoy 1.20 #86755
Envoy 1.20 #86755
Conversation
Duplicate of #86741 |
@chenrui333 you might notice from the commits here that #86741 is missing some steps including updating the alias and bumping the minor versions. that's why I don't use bump automation yet.. |
96e8644
to
f95d065
Compare
It will take a while for me to mark this ready for review as I need to test locally and envoy takes ages to build. Here's what I am doing: $ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core
$ gh pr checkout 86755
$ brew install --build-from-source envoy@1.19
$ envoy version
$ brew install --build-from-source envoy
$ envoy version |
envoy@1.19 is ok locally $ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core
$ gh pr checkout 86755
$ brew install --build-from-source envoy@1.19
$ brew test envoy@1.19
$ brew audit --strict envoy@1.19
$ brew audit --new envoy@1.19
$ /usr/local/Cellar/envoy\@1.19/1.19.1/bin/envoy --version |
Envoy still doesn't build on M1 due to an unmerged Bazel-related change, so it will continue to fail here. I would also warn that even if merged, typically Envoy doesn't backport build-related stuff to release branches, so it might not affect anything until 1.21 or later envoyproxy/envoy#17438 |
f95d065
to
3b929b2
Compare
Ran the following and it all worked, except a warning about patches in new formula. I looked and found the patch did seem to land in 1.20, so I removed it and am running the below again. If they pass, I will mark ready for review. $ brew install --build-from-source envoy
$ brew test envoy
$ brew audit --strict envoy
$ brew audit --new envoy
$ envoy --version |
ok works locally both! |
another one for you @carlocab 😎 |
Why is ARM no longer supported in this version? |
sorry @SMillerDev I was unclear with my comment. I should have emphasized that arm64 didn't build before. My note was more just re-checking the status of it, and while arm64 support is near, it isn't here, yet. |
@cho-m looks like the linux build broke again? Since you might be familiar, any clues from #82077?
I suspect this would need to be done on both 1.19 and current (1.20)... |
3b929b2
to
e021783
Compare
nm @cho-m I found the part I missed
|
Current failure looks like
|
I asked for help on envoyproxy/envoy#17438 on the failure extracting go here |
the error message seems to match this from bazel
If this is the right bazel code, the command invoked was 'tar -xf go_sdk.tar.gz --strip-components=1' and 'No such file or directory' could possibly have been from tar, if the "go_sdk.tar.gz" didn't actually download. On my ubuntu host's tar, the message is longer, but ends in 'No such file or directory', whereas a binary not found looks like 'not found' or 'command not found'
|
I'm currently of the opinion that tar wasn't found perhaps due to some sandboxing. I can't explain why
Please someone who actually works on envoy please troubleshoot this. Things like this are a good reason why homebrew should be a part of the upstream release and not punted to community who have little experience to figure out these things. See envoyproxy/envoy#17500 |
@carlocab @SMillerDev I also opened a frank discussion as us continually losing weekends over this is less than ideal. Unless envoy starts caring actively about success here I don't think we can afford two operating systems to maintain Homebrew/discussions#2271 |
7363839
to
419fe93
Compare
Summary: Envoy's build uses go to generate files from protobuf sources. Due to bazel norms, it nests download and extraction of a pinned go SDK. Recently, the bazel rules changed to use system tar instead of built-in. So, downloading would work, but if system tar can't be found, the build would fail. The default PATH changed recently when on_mac/on_linux was phased out and replaced with OS.mac/OS.linux. This wasn't tested, but also the substitution went fine on macOS. Linux ended up with a partial failure: The main takeaway is this formula can't use mechanical substitution due to branching logic. All changes need to be built-from-source on change on all supported operating systems. FWIW, I still recommend removing linux support (though maybe not in this PR). It is true this formula will still be patched and complicated even without linux, but it will be less. More importantly, the burden of change is lower, instead of having to test on linux due to issues like this, possibly via docker which is painfully slow, tests are limited to "just macOS". Even if that still takes an hour or more to build locally, it is an improvement and in this case would have saved days. Thanks to @cho-m for spending time to get to the bottom of this. Hopefully, this passes now except for the known issue where arm64 won't yet work on macOS. |
419fe93
to
08e6694
Compare
updated the comments on patches and remaining upstream issues |
Formula/envoy@1.19.rb
Outdated
# GCC added as a test dependency to work around Homebrew issue. Otherwise `brew test` fails. | ||
# CompilerSelectionError: envoy cannot be built with any available compilers. | ||
depends_on "gcc@9" => [:build, :test] | ||
depends_on "python@3.9" => :build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my 2p: this deserves a comment why python@3.9
especially as this is why the PATH is patched for linux below.
Formula/envoy@1.19.rb
Outdated
@@ -0,0 +1,83 @@ | |||
class EnvoyAT119 < Formula |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the version that's failing to build, why don't we leave 1.18 be for now? It's not terribly popular anyway:
==> Analytics
install: 13 (30 days), 73 (90 days), 73 (365 days)
install-on-request: 13 (30 days), 73 (90 days), 73 (365 days)
build-error: 0 (30 days)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to take your time on this one, just I don't fully understand which course to take.
Do you mean leave envoy@1.18 while adding envoy@1.19 and envoy (aliased to @1.20)?
or do you mean leave envoy@1.18, (drop envoy@1.19) while adding envoy (aliased to @1.20)?
Main issue in envoy releases is many are incompatible with each other in some way even if basic usage rarely is impacted. If you meant to drop envoy@1.19 this could interfere with upgrading because that version won't be downloadable when envoy.rb switches from 1.19 to 1.20. Ignore this whole paragraph if that's not what you meant, just elaborating for the sake of timezones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, your opinion might be correct, as if you read carefully the release notes, it seems like 1.19 will be a dead release. That or they suggest skipping it, but will still release patches. In any case, I think you are right on this one..
https://www.envoyproxy.io/docs/envoy/v1.20.0/version_history/current
dns_filter: dns_filter protobuf fields have been renumbered to restore compatibility with Envoy 1.18, breaking compatibility with Envoy 1.19.0 and 1.19.1. The new field numbering allows control planes supporting Envoy 1.18 to gracefully upgrade to dns_resolution_config, provided they skip over Envoy 1.19.0 and 1.19.1. Control planes upgrading from Envoy 1.19.0 and 1.19.1 will need to vendor the corresponding protobuf definitions to ensure that the renumbered fields have the types expected by those releases.
One of the builds failed like this:
I'll restore the bottle block and 🤞 |
08e6694
to
ce9799d
Compare
Signed-off-by: Adrian Cole <adrian@tetrate.io> Co-authored-by: Michael Cho <cho-m@tuta.io>
ce9799d
to
aaf7380
Compare
I removed the dropping 1.18 for 1.19 after thinking through the implications of #86755 (comment). The summary is that if the project team suggest skipping 1.19.0 and 1.19.1, there's no value in retaining it. Implicitly, 1.18.x has higher priority. |
I feel the osx 10.14 fail might be around old certs as it consistently fails I'm surprised arm64 is passing.. I'll try it on my M1 |
"Successful in 19s" should have tipped me off that arm64 didn't build, just skipped gracefully. I tried manually and the known issues discussed and documented about arm64 still exist. @cho-m @carlocab are either of you keen to merge? Envoy had a conference this week and 1.20 was a part of that. It would be nice to have this version available to users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's try it. The weird state of the Linux runner might make @BrewTestBot unhappy though.
Dispatched a bottle for Linux: https://github.com/Homebrew/homebrew-core/actions/runs/1335583065 |
thanks @carlocab. I'll keep an eye out! |
1.19 was dropped by Homebrew/homebrew-core#86755 Signed-off-by: Adrian Cole <adrian@tetrate.io>
1.19 was dropped by Homebrew/homebrew-core#86755 Signed-off-by: Adrian Cole <adrian@tetrate.io>
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?