-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Make BoringSSL symbols private to gRPC in Obj-C so there is no conflict when linking with OpenSSL. #16358
Conversation
|
How is this shadow file being generated? How do we make sure it's up to date when we update the dependency? |
|
Yep I have script that needs to be polished then uploaded. Current commits are just to verify if tests run ok. |
|
|
1 similar comment
|
|
|
|
|
1 similar comment
|
|
|
|
|
|
|
|
|
|
|
BUILD
Outdated
@@ -1543,6 +1543,7 @@ grpc_cc_library( | |||
"grpc_base", | |||
"grpc_transport_chttp2_alpn", | |||
"tsi", | |||
"shadow_boringssl", |
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.
Any particular reason to pick this name "shadow_boringssl"? How about a more intuitive name like "boringssl_grpc" since you are using BoringSSL-Grpc in the podsec.
BUILD
Outdated
grpc_cc_library( | ||
name = "shadow_boringssl", | ||
hdrs = [ | ||
"src/core/tsi/boringssl_shadow.h", |
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.
Use consistent naming. Rename boringssl_shadow.h to shadow_boringssl.h. boringssl_grpc.h sounds more intuitive.
|
|
|
|
|
|
|
|
|
@@ -0,0 +1,32 @@ | |||
#!/bin/bash |
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.
nit: check_boringssl_commit.sh is not a great name. You are checking if the "shadow boringssl symbol list is up to date", the fact that you are looking whether the commit sha in a file is the same is implementation detail. IMHO, seeing "commit" in the script name doesn't tell me anything useful
I suggest renaming to check_boringssl_symbol_list.sh
, check_boringssl_shadow_symbol_list.sh
or sth in that sense.
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.
check_shadow_boringssl_symbol_list.sh
is perhaps the best name, but leaving up to you.
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.
One last comment about the check_boringssl_commit.sh
and after that I'm happy with the change.
LGTM and thanks!
4e90cc6
to
b24b212
Compare
|
|
|
|
|
|
|
The change is to add prefix to BoringSSL symbols in gRPC Objective-C to avoid conflicting with OpenSSL for apps that depend on both SSL libraries (see detailed discussions in #6111). This PR makes gRPC to prefix its own BoringSSL symbols with
GRPC_SHADOW_
to avoid conflict with OpenSSL.