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

Allow CMake to build Firestore with OpenSSL #1597

Merged
merged 4 commits into from
Jul 27, 2018
Merged

Allow CMake to build Firestore with OpenSSL #1597

merged 4 commits into from
Jul 27, 2018

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jul 27, 2018

Also allow the build to succeed with no SSL.


if(CXX_CLANG OR CXX_GNU)
if((OPENSSL_VERSION VERSION_EQUAL "1.1.0") OR
(OPENSSL_VERSION VERSION_GREATER "1.1.0"))
Copy link
Member

Choose a reason for hiding this comment

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

VERSION_GREATER_EQUAL ?

(Or are you avoiding this due to a cmake version requirement issue? In which case, carry on.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VERSION_GREATER_EQUAL was added in CMake 3.7, so yes it's a version requirement thing.


enable_language(C)
enable_language(CXX)

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this commit:

  1. "match boringssl build prep"
    This doesn't look related to boringssl?
  2. CMP0025
    No objection to this change, but I don't understand why it's necessary to make openssl work.
  3. cl.exe
    This seems potentially necessary, as anything protected with if(CXX_GNU) will now not be run. Concretely, you won't set the -Wno-deprecated-declarations cflag. (Side note: Does cl.exe not do deprecation warnings? Or maybe grpc doesn't generate __declspec(deprecated) if compiling with msvc? In either case, the flag to disable the warnings will certainly be different.)

....

Update: Ah, you meant https://github.com/google/boringssl/blob/master/CMakeLists.txt.

So, IIUC, this commit is unrelated to openssl; is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, this PR is a pile of fixes that I needed at one point and I think are worth keeping. I've been rebasing them further and further into my queue because they haven't been essential.

Re (1) and (2): It's all related to getting SSL support for gRPC to work with add_subdirectory and on Windows. It's become a bit unstuck in time though because I was trying to pare down the commits required to convert us to the-add_subdirectory-style build.

Initially on Windows I couldn't get BoringSSL to build at all so I added support for building with an outboard OpenSSL.

Then on macOS I could not get BoringSSL to build with add_subdirectory at the version of grpc we're using because of the conflict on the gtest_main target. I had thought I needed to update to 11850d5f, but this broke the build because clang didn't recognize -Wno-tautological-constant-compare.

At that point I found ec55dc15 and realized that both platforms were suffering from the same problem: that we were failing to get CMake into a state that BoringSSL expected. The problem here is that you have to set policies that affect the compiler before doing any compiler setup, but as the controlling project we were preempting BoringSSL's code to do this.

In the interest of minimizing the add_subdirectory changes I upgraded BoringSSL to the then-present. However, this is actually past the last formal release branch so it's an unsatisfying solution.

These changes have been sitting in my queue and I think they're worthwhile because they expand the window of compatible BoringSSL commits. Outboard OpenSSL is worthwhile because people are complaining about this in the CocoaPods build--we can actually solve the problem in CMake.

Re (3): Most unfortunately, cl does not accept gcc-compatible flags, so passing -Wno-deprecated-declarations fails the build on Windows. The equivalent there is /wd4996. Microsoft has implemented Clang/C2 which is the clang-frontend with the microsoft backend, but this isn't mature enough to be a requirement at VS 2017 and It's not available in VS 2015. (Also, it looks like they'll be dropping it anyway--see Andrew Pardoe's comment.)

If you'd prefer I can rename this PR "SSL interoperability" :-).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explaination; this is making more sense now.

The equivalent there is /wd4996

Yeah, I figured there must be some equivalent. I'm surprised you didn't need to add it, but maybe our cl.exe cflags don't exit on warnings yet? Not something I'm concerned about; I'm sure our cl.exe cflags are woefully incomplete at this stage.

@wilhuff wilhuff merged commit 256fc3c into master Jul 27, 2018
@wilhuff wilhuff deleted the wilhuff/openssl branch July 27, 2018 16:23
@firebase firebase locked and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants