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.

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: removes go dependency #84071

Closed

Conversation

codefromthecrypt
Copy link
Contributor

According to @lizan #83413 (comment)

Envoy build process pulls Go 1.15.5 by Bazel (https://github.com/envoyproxy/envoy/blob/main/bazel/dependency_imports.bzl#L14), so it isn't relevant to system Go. M1 support should be ignored until Envoy supports that.

  • 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>?

According to @lizan:
> Envoy build process pulls Go 1.15.5 by Bazel (https://github.com/envoyproxy/envoy/blob/main/bazel/dependency_imports.bzl#L14), so it isn't relevant to system Go. M1 support should be ignored until Envoy supports that.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@BrewTestBot BrewTestBot added the no ARM bottle Formula has no ARM bottle label Aug 27, 2021
@carlocab carlocab added the CI-no-bottles Merge without publishing bottles label Aug 27, 2021
@cho-m cho-m added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Aug 27, 2021
@SMillerDev
Copy link
Member

I think we're going the wrong direction here. We do not want software to pull in their own software to build with. At best that violates https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae and at worst it's https://docs.brew.sh/Acceptable-Formulae#we-dont-like-install-scripts-that-download-unversioned-things

@codefromthecrypt
Copy link
Contributor Author

@SMillerDev so this change is trying to make it more honest than before as even if we depended on system go, as far as I know it was never used. Maybe you can mention impact on this issue as I'm not sure if any Envoy maintainers are reading this one envoyproxy/envoy#17886

@carlocab
Copy link
Member

This was already pulling it its own version of Go I think. The only thing this PR does is makes sure we don't have to spend an extra 8~ hours building envoy in Go PRs, which is good for our CI throughput.

@cho-m
Copy link
Member

cho-m commented Aug 28, 2021

envoy is another Bazel-built formula, so it shares some aspects with other Bazel formulae that don't exactly align with Homebrew's preferences. Specifically, due to Bazel's ideology of reproducible builds, it will vendor various dependences and it will also evade our superenv/shims.

So, unless the upstream specifically provides a way around this, we probably have no easy way to provide our own system libraries, build tools, etc.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Sep 19, 2021
Copy link
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.

In favour of merging this -- this doesn't change anything about how envoy is built, and saves us a lot of time in CI.

@carlocab carlocab removed do not merge stale No recent activity labels Sep 20, 2021
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@codefromthecrypt codefromthecrypt deleted the envoy-nogo branch October 6, 2021 23:48
@codefromthecrypt
Copy link
Contributor Author

envoy's internal management of go SDK during build caused another problem, though I highly doubt they will stop doing this #86755 (comment)

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-bottles Merge without publishing bottles no ARM bottle Formula has no ARM bottle outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants