Update third_party/boringssl-with-bazel #31938
Conversation
jtattermusch
added
release notes: yes
grpc-checks
bot
added
per-call-memory/neutral
per-channel-memory/neutral
bloat/none
labels
|
Oh, this is probably from https://boringssl-review.googlesource.com/c/boringssl/+/51307. We cleaned that up because it was confusing for Arm Macs to use crypto_ios_aarch64 when the assembly was actually targeting all Apple platforms. (Conversely, it was confusing for the iOS simulator to use crypto_mac_x86_64.) It's probably not so useful when it's a whole year of changes (too many to scroll through), but we try to call out potentially interesting changes with "Update-Note" lines in the CL description. Many of our update scripts dump something like I think you need to update the
Hmm, not sure what's going on here. #31869 (comment) mentioned something about matching header file names, but that's an odd error to get in this context. Unless perhaps it's doing an implicit prototype (without warning??) and then some later step notices the mismatch. If it is the matching header names, that's a bit odd. Is it possible BoringSSL's |
Looking at the flags in the build log, a few things stand out to me: (I sorted them to get all the It seems there's some weird thing going on with the BoringSSL-GRPC "pod"? That's this file, right? I don't quite understand what's going on here, but it looks like the podspec has to play a lot of strange games with headers as part of some Apple-specific modules system? Is it possible one of these is resulting in BoringSSL's headers getting added directly to the include path? Perhaps someone familiar with podspecs can help fix this? PS: After https://boringssl-review.googlesource.com/c/boringssl/+/50849, you don't need to wrap the arm_arch.h include here anymore: |
@sampajano can you help here? |
@jtattermusch Thanks for looping me in. I took a quick look at the issue but i don't really have any previous familiarity or expertise with Podspec files and the header conflict. @HannahShiSFB - Since you've been working with the boring SSL upgrade and has seen the header conflict, i wonder if you have any extra insight on what's happening here? Please lemme know if you need me to take a look together with you to dig into it. Thanks! |
|
Is there someone on gRPC who does understand those files? You all evidently set them up at some point. :-) Right now, all indications are that gRPC have somehow misconfigured the build and it only happened to be noticed now. If this is some fundamental constraint in cocoapods, we can try to figure something out, but someone needs to understand it before it can be declared a fundamental constraint. |
|
My understanding is to make boringssl be able to coexist with openssl (#16358, #16660, etc.), follow things were added in BoringSSL-GRPC.podspec
I tried disabled all the above 4 but the error remains, the build only succeeded with removing "openssl/time.h" file and the includes. test code in #31989 |
|
@jtattermusch Fun. I've no idea what any of that means. I gather this part of Bazel doesn't actually work yet? @sifmelcara I don't see an error about poly_rq_mul.S anywhere in your comment. What's that error? The one folks are running into here seem to be something else. |
|
The poly_rq_mul.S looks like this (there are like 6000 of these error) Click to expandThe way I'm testing this PR is |
|
That sounds like something has gone very badly wrong in gRPC's build. That's an x86_64 assembly file, which shouldn't be used when building for Arm targets. Perhaps more fallout from Bazel's "platforms" machinery not working right? gRPC folks, this is the second of your buildsystems that has run into mysterious issues so far. Who on your team is familiar with your build? |
|
One thing that might help in future: https://boringssl.googlesource.com/boringssl/+/cdccbe121fa5dc8c6df7102eaedb62c99eec8273 tried to wrap every asm file in ifdefs corresponding to the platform. The hope was that we can reduce the amount of platform conditions in the build and just rely on the preprocessor to drop the files that aren't used. That would probably have avoided the poly_rq_mul.S issue. But I haven't gotten that to work on the build side yet, so for now we still depend on detecting the platform in the build. |
ObjC examples tests have passed in #31869, could you check the difference first? I'm currently working on the android problem and will help to look into it later, if needed. |
|
Ok, so I think I've been able to fix the android binder example build. Basically, since we use I think I was able to figure out the right platform_mappings file to fix the build, but the problem with boringssl switching to bazel
I think eventually grpc will need to switch to |
jtattermusch
force-pushed
the
update_boringssl_202212202125
branch
from
a812c43
to
905871e
Compare
|
@HannahShiSFB looks like I have a solution for the android binder example (or a temporary workaround), but I'd still need help with the ObjC Examples. |
@jtattermusch Hey Jan, since Hannah has provided her boring SSL update PR (#31869) where the test is working, could you try to find out what are the discrepancies on why it's passing there but failing here? It might give you a clue on how to improve the script here? (If i understand correctly the only difference between yours and her PR is that she did the upgrade manually and you did it via a script?) Thanks! |
https://boringssl-review.googlesource.com/c/boringssl/+/55927, though it sounds like that doesn't address the broader |
This seems to work more reliably across platforms. Actually, both seem to work on Apple platforms, so we probably didn't need the conditions, while Android seems to only accept -pthread, based on [1]. This also matches Abseil [2], Envoy [3], and a random example in Bazel documentation [4]. Though, as a counter-example, POSIX seems to prefer -lpthread, not -pthread, per [5]. [1] grpc/grpc#31938 (comment) [2] https://github.com/abseil/abseil-cpp/blob/625a18016d6208c6c0419697cb6caa3f23ce31bc/absl/base/BUILD.bazel#L185 [3] https://github.com/envoyproxy/envoy/blob/main/bazel/envoy_binary.bzl#L72 [4] https://bazel.build/tutorials/cpp-use-cases#include-external-libraries [5] gflags/gflags#176 (comment) Change-Id: I15005bcf4e4b7ebe91a835b8262c5c6434715c3b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55927 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
@HannahShiSFB 's PR doesn't have ObjC tests failing since it doesn't upgrade to the latest boringssl version, but to a commit from Nov 2022 (see #31869 (comment)), just before boringssl has added the time.h related change. Also that PR doesn't have the 'USE_HEADERMAP' => 'NO' change in it (since it doesn't need it, there are no time.h related boringssl changes that would need such fix). As mentioned, the USE_HEADERMAP setting seems to solve the issue for some ObjC test jobs but not for ObjC examples. Since Nov 2022 is not that long ago, perhaps one solution would be to solve the boringssl upgrade in two steps:
|
It's great that you got the android binder example done! Unfortunately, I don't have a quick answer for the ObjC Examples failure, will need extra time to look into it. (we need to fix this eventually, but maybe not right now? one step at a time... ) |
|
Since the new error doesn't seem related to time.h, my guess is that disabling headermaps didn't so much fail to solve a second time.h problem but uncovered or introduced (guess we'll figure out which when we understand this mess!) a problem elsewhere. From what I can tell, headermaps seem to bypass the standard include path machinery. In the case of time.h, it bypassed it in a way that caused problems. But it's plausible that something else was relying on it and didn't have an equivalent setup with the standard mechanism. So my guess is, by setting that, we've exposed this and need to tidy that up. (We can probably confirm this guess by seeing what USE_HEADERMAPS does in a pre-time.h BoringSSL.) |
|
While this PR is important and we do want to upgrade boringssl ASAP (definitely for the next release cycle 1.53.x), I'm not going to block the 1.52.x release on it (so removing the release blocker label). |
|
'USE_HEADERMAP' => 'NO' does fix the time.h issue. I couldn't reproduce the same error of not find 'asn1t.h' but werid Projects in But I get error building samples in I however don't know why they are different form the test projects and how to fix them. |
This seems to work more reliably across platforms. Actually, both seem to work on Apple platforms, so we probably didn't need the conditions, while Android seems to only accept -pthread, based on [1]. This also matches Abseil [2], Envoy [3], and a random example in Bazel documentation [4]. Though, as a counter-example, POSIX seems to prefer -lpthread, not -pthread, per [5]. [1] grpc/grpc#31938 (comment) [2] https://github.com/abseil/abseil-cpp/blob/625a18016d6208c6c0419697cb6caa3f23ce31bc/absl/base/BUILD.bazel#L185 [3] https://github.com/envoyproxy/envoy/blob/main/bazel/envoy_binary.bzl#L72 [4] https://bazel.build/tutorials/cpp-use-cases#include-external-libraries [5] gflags/gflags#176 (comment) Change-Id: I15005bcf4e4b7ebe91a835b8262c5c6434715c3b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55927 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 412b20b0f699938a370a97cc11815b1eb1e0fcb2)
This seems to work more reliably across platforms. Actually, both seem to work on Apple platforms, so we probably didn't need the conditions, while Android seems to only accept -pthread, based on [1]. This also matches Abseil [2], Envoy [3], and a random example in Bazel documentation [4]. Though, as a counter-example, POSIX seems to prefer -lpthread, not -pthread, per [5]. [1] grpc/grpc#31938 (comment) [2] https://github.com/abseil/abseil-cpp/blob/625a18016d6208c6c0419697cb6caa3f23ce31bc/absl/base/BUILD.bazel#L185 [3] https://github.com/envoyproxy/envoy/blob/main/bazel/envoy_binary.bzl#L72 [4] https://bazel.build/tutorials/cpp-use-cases#include-external-libraries [5] gflags/gflags#176 (comment) Change-Id: I15005bcf4e4b7ebe91a835b8262c5c6434715c3b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55927 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 412b20b0f699938a370a97cc11815b1eb1e0fcb2)
This seems to work more reliably across platforms. Actually, both seem to work on Apple platforms, so we probably didn't need the conditions, while Android seems to only accept -pthread, based on [1]. This also matches Abseil [2], Envoy [3], and a random example in Bazel documentation [4]. Though, as a counter-example, POSIX seems to prefer -lpthread, not -pthread, per [5]. [1] grpc/grpc#31938 (comment) [2] https://github.com/abseil/abseil-cpp/blob/625a18016d6208c6c0419697cb6caa3f23ce31bc/absl/base/BUILD.bazel#L185 [3] https://github.com/envoyproxy/envoy/blob/main/bazel/envoy_binary.bzl#L72 [4] https://bazel.build/tutorials/cpp-use-cases#include-external-libraries [5] gflags/gflags#176 (comment) Change-Id: I15005bcf4e4b7ebe91a835b8262c5c6434715c3b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55927 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 412b20b0f699938a370a97cc11815b1eb1e0fcb2)
Now that all assembly files are conditionalized, we no longer need to detect platforms at the build level. This is convenient because detecting platforms in Bazel is a bit of a mess. In particular, this reduces how much we depend on @platforms being correct. gRPC's build appears to still be using some legacy modes which seem cause it to, on cross-compiles, report the host's platforms rather than the target. See grpc/grpc#31938 gRPC should eventually fix this, but it is apparently challenging due to complexities in migrating from Bazel's legacy system the new "platforms" mechanism. Instead, try to sidestep this problem by not relying on the build to do this. Now, we primarily rely on os:windows being accurate, and cross-compiling to/from Windows is uncommon. We do also need os:linux to be accurate when Linux is the target OS, but if Linux is the host and gRPC mislabels the target as os:linux, this is fine as long as the target is not FreeBSD, Apple, or another platform that cares about _XOPEN_SOURCE. (In particular, Android is ambivalent.) I've also renamed a few things based on what they were actually selecting. posix_copts was really copts for toolchains with GCC-style flags. Unfortunately, it's not clear how to condition on the compiler, rather than the platform in Bazel, so we'll do the wrong thing on non-MSVC Windows toolchains, but that was true before. Bug: 542 Change-Id: I7330d8961145ae5714d4cad01259044230d96bcd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56465 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
|
Closing in favor of #32492 |
The docs change is extracted from grpc#31869 and grpc#31938. The actual upgrade of boringssl is in progress, but in the meantime we can at least make sure the instructions are up-to-date. I'll also update the internal counterpart (cl/501499368) Co-authored-by: Hannah Shi <hannahshisfb@gmail.com>
Change was created by the release automation script. See go/grpc-release
Alternative to #31869
TODO: figure out the breakages caused by the update (python, objc, BinderTransport etc).