Skip to content

Conversation

@jonsimantov
Copy link
Contributor

@jonsimantov jonsimantov commented Dec 8, 2020

This PR gives you the option to switch all desktop platforms over to using boringssl rather than openssl, which does not like being embedded in our SDKs. If you are building a binary SDK, you may want to enable this option. If you are linking directly to our SDK from source via cmake, you probably don't need it (but you should ensure that OpenSSL is installed on your system.

It will be enabled if you turn on FIREBASE_USE_BORINGSSL at cmake configure time, e.g.:
cmake [source directory] -DFIREBASE_USE_BORINGSSL=ON

With this option enabled, it will build boringssl during the configure step by shelling out to another instance of cmake. Much work is done in build_external_dependencies to ensure that this (and future) sub-builds correctly match the main project's selection of:

  • CPU architecture
  • build type (debug/release)
  • Windows MSVC runtime
  • Linux legacy/c++11 ABI

A few notes on switching to boringSSL:

  • We manually set all of the OPENSSL_* flags that CMake's find_package(OpenSSL) sets, so that any subprojects using openssl will get the include files and libraries of boringssl instead.
  • We have updated to the latest libcurl which supports boringssl properly.
  • OpenSSL has been removed from the vcpkg files.
  • Golang has been added as a prerequisite as it's required for boringssl's build. Github runners already have it installed, but I've added it to the prereqs script anyway.

Additional changes in this PR:

  • The Linux build has been switched to use the legacy ABI rather than the C++11 ABI. An upcoming PR will enable both ABIs.
  • Binutils on Mac is restored to the latest version, as openssl was the cause of the library corruption we were seeing.
  • Some missing Firestore core headers are now added to the final C++ SDK package.
  • Integration test github action now builds and runs both openssl and boringssl versions.

You'll also see a few workaround at the end of the download_external_sources function, this is to work around a few issues in subprojects that expect openssl but are now getting boringssl. There are PRs to fix these in the upstream sources, but until we update to their latest version these will need to be there.

@google-cla google-cla bot added the cla: yes label Dec 8, 2020
…ust the ones they are embedded in. Also pass along build options to the cmake build for boringssl. Also set curl to only build http/https support and not other things like SMTP, FTP, etc.
@jonsimantov
Copy link
Contributor Author

Seems to be causing a couple issues that I will look into before this moves forward.

This is now fixed, you can link the SDK from source either with openssl (default) or specifying FIREBASE_USING_BORINGSSL.

PTAL

@jonsimantov jonsimantov changed the title Switch all dependencies to use boringssl instead of openssl. Allow all dependencies (curl, websockets, grpc) to use boringssl instead of openssl when making packaged C++ SDK. Dec 14, 2020
@vimanyu
Copy link
Contributor

vimanyu commented Dec 14, 2020

Now that there is option to also use openssl, do we want to re-add openssl to vcpkg's list of packages?
Or we could assume users are trying to use a system level openssl package?

vcpkg will only make sense if we don't want to use a system specific package (vcpkg packages are localized in a directory and only used for that project) and if we care about the versions of openssl library.

@jonsimantov
Copy link
Contributor Author

I think we should keep it out of vcpkg - again, I do think it might make sense to move off of vcpkg and build all dependencies in the binary directory in the future. Also putting it into vcpkg causes cmake to sometimes think it wants to use that library.

@vimanyu
Copy link
Contributor

vimanyu commented Dec 14, 2020

Cool, sounds good. I agree, it would be awesome to have a self contained build in the future and avoid dealing with external packages.
Just to summarize, if users build our SDK from source by themselves and if they do not set FIREBASE_USE_BORINGSSL=ON (default value is OFF), they will have to ensure OpenSSL availability on their system but if they use the build_desktop.py script, then they dont have to deal with openssl as the command sets this option before running cmake.
Approving the PR.

vimanyu
vimanyu previously approved these changes Dec 14, 2020
Copy link
Contributor

@vimanyu vimanyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

a-maurice
a-maurice previously approved these changes Dec 14, 2020
@jonsimantov jonsimantov dismissed stale reviews from a-maurice and vimanyu via b8a1b2b December 14, 2020 22:19
@jonsimantov jonsimantov merged commit 1e2dd0e into dev Dec 15, 2020
@jonsimantov jonsimantov deleted the feature/js-switch-to-boringssl branch December 15, 2020 07:04
@firebase firebase locked and limited conversation to collaborators Jan 14, 2021
@DellaBitta DellaBitta restored the feature/js-switch-to-boringssl branch January 20, 2021 15:03
@DellaBitta DellaBitta deleted the feature/js-switch-to-boringssl branch January 20, 2021 15:57
@DellaBitta DellaBitta restored the feature/js-switch-to-boringssl branch January 20, 2021 15:57
@DellaBitta DellaBitta deleted the feature/js-switch-to-boringssl branch January 20, 2021 15:57
@jonsimantov jonsimantov restored the feature/js-switch-to-boringssl branch January 21, 2021 07:44
@jonsimantov jonsimantov deleted the feature/js-switch-to-boringssl branch February 8, 2021 04:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3 participants