Skip to content

Unify boringssl submodules and use non-developer boringssl cmake build#21527

Merged
jtattermusch merged 26 commits intogrpc:masterfrom
jtattermusch:unify_boringssl_deps2
Jan 20, 2020
Merged

Unify boringssl submodules and use non-developer boringssl cmake build#21527
jtattermusch merged 26 commits intogrpc:masterfrom
jtattermusch:unify_boringssl_deps2

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Dec 19, 2019

Main parts of this PR:

  • remove the third_party/boringssl submodule entirely and moving everything towards using boringssl-with-bazel (both submodules actually contain the same boringssl source code, but "boringssl-with-bazel" points to "master-with-bazel" branch, which contains pregenerated code for bazel and CMake builds)
  • instead of boringssl's "developer" build which has some unusual dependencies (such as golang etc, required to perform codegeneration of the assembly code), switch our cmake build to boringssl's newly introduced "external cmake build". That results in less codegeneration being required and fewer dependencies for the users.
  • deduplicate the "boringssl" and "boringssl-with-bazel" submodules that both needed to be regularly updated and that was source of confusion and extra maintenance.

A bit more background on boringssl's "master" vs "master-with-bazel" branches (might be useful when reviewing).
both contain the same code, but "master-with-bazel" has exactly the same files as "master" but all the sources are moved under /src and in the repository root there are some pre-generated assembly files used by the CMake and bazel builds. The "master-with-bazel" branch is updated by boringssl team's bot every time a commit is added to "master".
(also see https://github.com/google/boringssl/blob/master/INCORPORATING.md#bazel)

TODOs:

@jtattermusch jtattermusch force-pushed the unify_boringssl_deps2 branch from 6fa8590 to dae922c Compare January 16, 2020 08:59
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jan 16, 2020

  • ObjC tests mostly fail with
    /bin/bash: line 3: include/openssl/umbrella.h: No such file or directory, but that seems fixable.

  • Besides that, there's a bunch of ubsan failures (seem related to boringssl)

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //test/core/end2end:h2_oauth2_test@bad_ping@poller=epoll1
-----------------------------------------------------------------------------
D0116 09:14:06.824699123      17 test_config.cc:385]         test slowdown factor: sanitizer=5, fixture=1, poller=1, total=5
I0116 09:14:06.825680183      17 ev_epoll1_linux.cc:116]     grpc epoll fd: 3
D0116 09:14:06.825716743      17 ev_posix.cc:174]            Using polling engine: epoll1
D0116 09:14:06.826282179      17 dns_resolver_ares.cc:516]   Using ares dns resolver
external/boringssl/src/crypto/fipsmodule/rsa/rsa_impl.c:371:14: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:93:34: note: nonnull attribute specified here
    #0 0xec2eda in rsa_blinding_get /proc/self/cwd/external/boringssl/src/crypto/fipsmodule/rsa/rsa_impl.c:371:7

https://source.cloud.google.com/results/invocations/bead21ee-4f3a-4e2c-a3b8-fa3a17111027/targets/%2F%2Ftest%2Fcore%2Fend2end:h2_oauth2_test@filter_context@poller%3Depollex/log
https://source.cloud.google.com/results/invocations/bead21ee-4f3a-4e2c-a3b8-fa3a17111027/targets/%2F%2Ftest%2Fcore%2Fend2end:h2_ssl_cred_reload_nosec_test@bad_ping@poller%3Depoll1/log
https://source.cloud.google.com/results/invocations/bead21ee-4f3a-4e2c-a3b8-fa3a17111027/targets/%2F%2Ftest%2Fcore%2Fend2end:h2_ssl_cred_reload_nosec_test@binary_metadata@poller%3Depoll1/log

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jan 17, 2020

@muxi can you please help fixing the grpc/templates/src/objective-c/BoringSSL-GRPC.podspec.template?
Right now ObjC tests mostly fail with
/bin/bash: line 3: include/openssl/umbrella.h: No such file or directory.

The change in this PR is that instead of third_party/boringssl the openssl files are now under third_party/boringssl-with-bazel/src (but the submodule is still third_party/boringssl-with-bazel, so with respect to the repository root, so e.g. include/openssl/aes.h is now under src/include/openssl/aes.h)

@jtattermusch
Copy link
Copy Markdown
Contributor Author

@muxi looks like there's still some errors:

target 'Sample' do
  # Depend on the generated RemoteTestClient library
  pod 'RemoteTest', :path => "../RemoteTestClient"

  # Use the local versions of Protobuf, BoringSSL, and gRPC. You don't need any of the following
  # lines in your application.
  pod '!ProtoCompiler',  :path => "#{GRPC_LOCAL_SRC}/src/objective-c"
  pod '!ProtoCompiler-gRPCPlugin', :path => "#{GRPC_LOCAL_SRC}/src/objective-c"

  pod 'Protobuf', :path => "#{GRPC_LOCAL_SRC}/third_party/protobuf"

  pod 'BoringSSL-GRPC', :podspec => "#{GRPC_LOCAL_SRC}/src/objective-c"

  pod 'gRPC', :path => GRPC_LOCAL_SRC
  pod 'gRPC-Core', :path => GRPC_LOCAL_SRC
  pod 'gRPC-RxLibrary', :path => GRPC_LOCAL_SRC
  pod 'gRPC-ProtoRPC',  :path => GRPC_LOCAL_SRC
end

# This pre_install hook is only needed to use the local version of gRPC-Core. You don't need it in
# your application.
pre_install do |installer|
  # This is the gRPC-Core podspec object, as initialized by its podspec file.
  grpc_core_spec = installer.pod_targets.find{|t| t.name == 'gRPC-Core'}.root_spec

  # Copied from gRPC-Core.podspec, except for the adjusted src_root:
  src_root = "$(PODS_ROOT)/../#{GRPC_LOCAL_SRC}"
  grpc_core_spec.pod_target_xcconfig = {
    'GRPC_SRC_ROOT' => src_root,
    'HEADER_SEARCH_PATHS' => '"$(inherited)" "$(GRPC_SRC_ROOT)/include"',
    'USER_HEADER_SEARCH_PATHS' => '"$(GRPC_SRC_ROOT)"',
    # If we don't set these two settings, `include/grpc/support/time.h` and
    # `src/core/lib/gpr/string.h` shadow the system `<time.h>` and `<string.h>`, breaking the
    # build.
    'USE_HEADERMAP' => 'NO',
    'ALWAYS_SEARCH_USER_PATHS' => 'NO',
  }
end

Error

Errno::ENOENT - No such file or directory @ rb_sysopen - /Volumes/BuildData/tmpfs/src/github/grpc/src/objective-c/examples/Sample/Pods/BoringSSL-GRPC/include/openssl/BoringSSL.modulemap
/Users/kbuilder/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/cocoapods-1.5.3/lib/cocoapods/installer/xcode/pods_project_generator/pod_target_installer.rb:595:in `read'
/Users/kbuilder/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/cocoapods-1.5.3/lib/cocoapods/installer/xcode/pods_project_generator/pod_target_installer.rb:595:in `read'
/Users/kbuilder/.rvm/rubies/ruby-2.5.1/lib/ruby/gems/2.5.0/gems/cocoapods-1.5.3/lib/cocoapods/installer/xcode/pods_project_generator/pod_target_installer.rb:595:in `block in create_module_map'

https://source.cloud.google.com/results/invocations/772a7f41-c3a9-4ed9-91bc-bcc9fe7d2630/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-buildtest-example-sample-frameworks/tests
https://source.cloud.google.com/results/invocations/09107ad4-57e8-4363-bdca-2789f4f41eb7/log

@jtattermusch
Copy link
Copy Markdown
Contributor Author

@jtattermusch jtattermusch force-pushed the unify_boringssl_deps2 branch from 0762555 to cb800db Compare January 18, 2020 08:13
@jtattermusch jtattermusch changed the title Unify boringssl deps2 Unify boringssl submodules and use non-developer boringssl cmake build Jan 18, 2020
@jtattermusch jtattermusch requested review from muxi and veblush January 18, 2020 08:20
@jtattermusch jtattermusch marked this pull request as ready for review January 18, 2020 08:21
@jtattermusch
Copy link
Copy Markdown
Contributor Author

This is now ready to review. There might be little bit of cleanup remaining, but that can be done incrementally as a followup (this PR is already complicated enough).

@jtattermusch
Copy link
Copy Markdown
Contributor Author

FYI @gnossen @nicolasnoble I will also need owner's approval.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

Known failures:
#21713
#21534
#21674
#21627
#13379

Looks like all the failures are unrelated, so this should be safe to merge.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

If there are any more comments, I can address them as a followup.

@jtattermusch jtattermusch merged commit eba60d8 into grpc:master Jan 20, 2020
jtattermusch added a commit that referenced this pull request Jan 27, 2020
Reintroduce #21527 (boringssl submodule unification)
hcaseyal added a commit that referenced this pull request Jan 28, 2020
Revert "Reintroduce #21527 (boringssl submodule unification)"
jtattermusch added a commit to jtattermusch/grpc that referenced this pull request Jan 30, 2020
jtattermusch added a commit that referenced this pull request Jan 31, 2020
Reintroduce #21527 (boringssl submodule unification - take two)
@lock lock bot locked as resolved and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lang/c++ release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants