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

Delete Firestore public C++ API #2050

Merged
merged 9 commits into from
Nov 7, 2018
8 changes: 6 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,17 @@ add_subdirectory(
set(BUILD_TESTING ${old_build_testing} CACHE BOOL "Restore BUILD_TESTING" FORCE)


# Firebase C++
find_package(firebase_cpp_sdk REQUIRED)


# gRPC
find_package(OpenSSL)
find_package(OpenSSL QUIET)
if(OPENSSL_FOUND)
set(gRPC_SSL_PROVIDER package CACHE STRING "Use external OpenSSL")
endif()

find_package(ZLIB)
find_package(ZLIB QUIET)
if(ZLIB_FOUND)
set(gRPC_ZLIB_PROVIDER package CACHE STRING "Use external ZLIB")
endif()
Expand Down
1 change: 1 addition & 0 deletions Firestore/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@
add_subdirectory(Example/Benchmarks)
add_subdirectory(Protos)
add_subdirectory(core)
add_subdirectory(cpp)
add_subdirectory(fuzzing)
3 changes: 0 additions & 3 deletions Firestore/core/include/firebase/firestore/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
# (see https://stackoverflow.com/questions/27039019/ and open issue on CMake
# issue tracker: https://gitlab.kitware.com/cmake/cmake/issues/15234)
add_custom_target(firebase_firestore_types_ide SOURCES
document_reference.h
event_listener.h
firestore.h
firestore_errors.h
geo_point.h
timestamp.h
Expand Down
17 changes: 17 additions & 0 deletions Firestore/cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

add_subdirectory(include/firebase/firestore)

add_subdirectory(test)
40 changes: 40 additions & 0 deletions Firestore/cpp/include/firebase/firestore/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Workaround to make the headers show up in IDEs
# (see https://stackoverflow.com/questions/27039019/ and open issue on CMake
# issue tracker: https://gitlab.kitware.com/cmake/cmake/issues/15234)
add_custom_target(
firebase_firestore_ide
SOURCES
collection_reference.h
document_change.h
document_reference.h
document_snapshot.h
event_listener.h
field_path.h
field_value.h
firestore.h
listener_registration.h
map_field_value.h
metadata_changes.h
query.h
query_snapshot.h
set_options.h
settings.h
snapshot_metadata.h
source.h
transaction.h
write_batch.h
)
21 changes: 21 additions & 0 deletions Firestore/cpp/test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

cc_test(
firebase_firestore_cpp_test
SOURCES
firebase_app_test.cc
DEPENDS
firebase_app
)
30 changes: 30 additions & 0 deletions Firestore/cpp/test/firebase_app_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2018 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <memory>

#include "firebase/app.h"
#include "gtest/gtest.h"

using firebase::App;
using firebase::AppOptions;

TEST(FirebaseAppTest, TestCreate) {
AppOptions options;
std::unique_ptr<App> app{App::Create(options)};
ASSERT_NE(nullptr, app.get());
EXPECT_STREQ(firebase::kDefaultAppName, app->name());
}
64 changes: 64 additions & 0 deletions cmake/Findfirebase_cpp_sdk.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

include(FindPackageHandleStandardArgs)

find_path(
FIREBASE_CPP_SDK_DIR include/firebase/future.h
HINTS
$ENV{FIREBASE_CPP_SDK_DIR}
${FIREBASE_BINARY_DIR}/external/src/firebase_cpp_sdk
)

find_path(
FIREBASE_CPP_INCLUDE_DIR firebase/future.h
PATHS
${FIREBASE_CPP_SDK_DIR}/include
)

if(APPLE)
if("${CMAKE_OSX_SYSROOT}" MATCHES "iphoneos")
# iOS
set(FIREBASE_CPP_LIB_DIR ${FIREBASE_CPP_SDK_DIR}/libs/ios/${CMAKE_SYSTEM_PROCESSOR})
else()
set(FIREBASE_CPP_LIB_DIR ${FIREBASE_CPP_SDK_DIR}/libs/darwin/universal)
endif()
endif()

find_library(
FIREBASE_CPP_APP_LIBRARY
NAMES firebase_app
HINTS
${FIREBASE_CPP_LIB_DIR}
Copy link
Member

Choose a reason for hiding this comment

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

This only works on apple, because this variable is otherwise empty. Patch for linux below. You're on your own for windows.

diff --git a/cmake/Findfirebase_cpp_sdk.cmake b/cmake/Findfirebase_cpp_sdk.cmake
index df8be76c..da0f2a9f 100644
--- a/cmake/Findfirebase_cpp_sdk.cmake
+++ b/cmake/Findfirebase_cpp_sdk.cmake
@@ -34,6 +34,8 @@ if(APPLE)
   else()
     set(FIREBASE_CPP_LIB_DIR ${FIREBASE_CPP_SDK_DIR}/libs/darwin/universal)
   endif()
+elseif(UNIX)
+  set(FIREBASE_CPP_LIB_DIR ${FIREBASE_CPP_SDK_DIR}/libs/linux/${CMAKE_SYSTEM_PROCESSOR})
 endif()
 
 find_library(

Sadly, while the patch allows everything to compile, the test immediately segfaults under at least gcc 7.3.0 and clang 5.0. (https://firebase.google.com/docs/cpp/setup#desktop-workflow lists the latter as explicitly tested.) The library's been stripped of symbols, so it's a little tricky to debug. No need to figure that out before submitting though.

Copy link
Member

Choose a reason for hiding this comment

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

Though come to think of it, travis might prevent you from submitting with linux in a broken state...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think a better patch would be

elseif(${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
  set(FIREBASE_CPP_LIB_DIR ${FIREBASE_CPP_SDK_DIR}/libs/linux/${CMAKE_SYSTEM_PROCESSOR})

The crash comes from the fact that the C++ SDK is compiled with gcc4 which predates gcc5's new C++11 ABI.

I thought I could fix this by adding something like this to the test CMakeLists.txt:

+if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
+  # Disable C++11 ABI support on Linux. The Firebase C++ SDK is currently built
+  # with gcc 4 which does not yet support the new ABI.
+  #
+  # TODO(mcg): Propagate this to every C++ translation unit, including all
+  # dependencies--even the ExternalProject ones.
+  target_compile_definitions(
+    firebase_firestore_cpp_test
+    PUBLIC
+    _GLIBCXX_USE_CXX11_ABI=0
+  )
+endif()

Unfortunately, this now fails to build because gtest has been compiled with the C++11 ABI but the test iteself has not. The way to fix this is to propagate this down to all of the dependencies we build via ExternalProject. I'm working on that now.

)

find_package_handle_standard_args(
firebase_cpp_sdk
DEFAULT_MSG
FIREBASE_CPP_INCLUDE_DIR
FIREBASE_CPP_APP_LIBRARY
)

if(FIREBASE_CPP_SDK_FOUND)
set(FIREBASE_CPP_INCLUDE_DIRS ${FIREBASE_CPP_INCLUDE_DIR})

if (NOT TARGET firebase_app)
add_library(firebase_app UNKNOWN IMPORTED)
set_target_properties(
firebase_app PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES ${FIREBASE_CPP_INCLUDE_DIRS}
IMPORTED_LOCATION ${FIREBASE_CPP_APP_LIBRARY}
)
endif()
endif(FIREBASE_CPP_SDK_FOUND)
1 change: 1 addition & 0 deletions cmake/external/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ set(
)

include(benchmark)
include(firebase_cpp_sdk)
include(googletest)
include(grpc)
include(leveldb)
Expand Down
36 changes: 36 additions & 0 deletions cmake/external/firebase_cpp_sdk.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Copyright 2018 Google
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

include(ExternalProject)

if(TARGET firebase_cpp_sdk)
return()
endif()

set(version 5.4.0)

ExternalProject_Add(
firebase_cpp_sdk

DOWNLOAD_DIR ${FIREBASE_DOWNLOAD_DIR}
URL https://dl.google.com/firebase/sdk/cpp/firebase_cpp_sdk_${version}.zip
URL_HASH SHA256=e21302574a806ee6111e58eef9cf226ac61e2736bc98e7abee2b44fb41deaa6b

PREFIX ${PROJECT_BINARY_DIR}

CONFIGURE_COMMAND ""
BUILD_COMMAND ""
INSTALL_COMMAND ""
TEST_COMMAND ""
)