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

build: revert boringssl patch #2651

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Nov 1, 2022

Reverting #470 which is a patch to boringssl
https://boringssl-review.googlesource.com/c/boringssl/+/37804 indicates that this patch was added to avoid a linker error, so given CI passes I assume it can be reverted.

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

Part of envoyproxy/envoy#23758
Fixes #471

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk force-pushed the boring branch 2 times, most recently from 0563701 to c892a9d Compare November 1, 2022 20:06
@alyssawilk alyssawilk changed the title boring build: revert boringssl patch Nov 2, 2022
@alyssawilk alyssawilk marked this pull request as ready for review November 2, 2022 14:07
@keith
Copy link
Member

keith commented Nov 2, 2022

So the linker failures for this were downstream, so I don't think our tests here ever really validated it was an issue / fixed. I verified with my repro case https://github.com/keith/BoringSSLWeakRepro that it does still repro with HEAD BoringSSL. But the issue was only around when you used this with bitcode, which was deprecated starting with Xcode 14. So we might be able to merge this and just ignore this moving forward. I feel like it should be reproducible if you're using LTO, but my repro case doesn't work with just testing that quickly now.

@alyssawilk
Copy link
Contributor Author

Ok in that case let's try landing and seeing if it sticks. If not we can roll back and revisit - I had an email drafted to boringssl folks to ask they bump priority but I wanted to repro first and this didn't :-P

@alyssawilk alyssawilk merged commit dbdb100 into envoyproxy:main Nov 2, 2022
@keith
Copy link
Member

keith commented Nov 2, 2022

I just rebased and commented again on my upstream patch, we'll see if it goes anywhere.

jpsim added a commit that referenced this pull request Nov 4, 2022
…builder-function

* origin/main:
  remove the use of deprecated flag (#2658)
  Cronvoy: Map EM Errors to Cronet API Errors II (#1594) (#2633)
  build: revert boring patch (#2651)
  Bump Lyft Support Rotation (#2654)
  fix one issue blocking bumping Envoy (#2649)
  ci: remove Snow from Lyft EM rotation (#2650)
  ci: increasing timeouts (#2653)
  python: Apply Envoy python-yapf formatting (#2648)
  repo: Shellcheck cleanups (#2646)
  repo: Switch `pip_install` -> `pip_parse` (#2647)
  Use safe_malloc instead of new when creating new_envoy_map_entry (#2632)
  python: Pin requirement hashes (#2643)
  Disable flaky TestConfig.StringAccessors (#2642)
  ci: migrate from set-output to GITHUB_OUTPUT (#2625)
  Add a comment to addPlatformFilter (#2634)
  Allow Cronvoy to build with proguard. (#2635)
  Update Envoy (#2630)
  Add support for Platform and Native filters to C++ EngineBuilder (#2626)
  Register getaddrinfo in extension_registry (#2627)
  dns: stop using cares DNS resolver (#2618)

Signed-off-by: JP Simard <jp@jpsim.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.

bazel: remove BoringSSL workaround
2 participants