Merge branch 'dev' into bugfix/integration-tests-mac-openssl-error

Author: vimanyu

.github/workflows/cpp-packaging.yml

@@ -11,6 +11,9 @@ on:
       preserveIntermediateArtifacts:
         description: 'preserve intermediate artifacts?'
         default: 0
+      verboseBuild:
+        description: 'verbose build?'
+        default: 0
       skipIntegrationTests:
         description: 'skip integration tests?'
         default: 0
@@ -60,6 +63,12 @@ jobs:
           github.event.inputs.downloadPublicVersion == '' && github.event.inputs.downloadPreviousRun == ''
         run: echo "::warning ::Intermediate artifacts will be preserved."
 
+      - name: log if verbose build enabled
+        if: |
+          github.event.inputs.verboseBuild != 0 && github.event.inputs.verboseBuild != '' &&
+          github.event.inputs.downloadPublicVersion == '' && github.event.inputs.downloadPreviousRun == ''
+        run: echo "::warning ::Verbose build enabled."
+
   build_tools:
     name: build-tools-${{ matrix.tools_platform }}
     runs-on: ${{ matrix.os }}
@@ -320,7 +329,11 @@ jobs:
       - name: Build desktop SDK
         shell: bash
         run: |
-          python scripts/gha/build_desktop.py --arch "${{ matrix.architecture }}" --config "${{ matrix.build_type }}" --msvc_runtime_library "${{ matrix.msvc_runtime }}" --linux_abi "${{ matrix.linux_abi }}" --build_dir out-${{ env.SDK_NAME }} ${{ matrix.additional_build_flags }}
+          verbose_flag=
+          if [[ -n "${{ github.event.inputs.verboseBuild }}" && "${{ github.event.inputs.verboseBuild }}" -ne 0 ]]; then
+            verbose_flag=--verbose
+          fi
+          python scripts/gha/build_desktop.py --arch "${{ matrix.architecture }}" --config "${{ matrix.build_type }}" --msvc_runtime_library "${{ matrix.msvc_runtime }}" --linux_abi "${{ matrix.linux_abi }}" --build_dir out-${{ env.SDK_NAME }} ${verbose_flag} ${{ matrix.additional_build_flags }}
           # Make a list of all the source files, for debugging purposes.
           cd out-${{ env.SDK_NAME }}
           find .. -type f -print > src_file_list.txt
@@ -433,12 +446,30 @@ jobs:
           else
             tools_platform=darwin
           fi
+          verbose_flag=
+          if [[ -n "${{ github.event.inputs.verboseBuild }}" && "${{ github.event.inputs.verboseBuild }}" -ne 0 ]]; then
+            verbose_flag=-v
+          fi
+          declare -a additional_flags
           tar -xvzf artifacts/packaging-tools-${tools_platform}/packaging-tools.tgz -C bin
           chmod -R u+x bin
           for pkg in artifacts/firebase-cpp-sdk-${{ matrix.sdk_platform }}${{ matrix.suffix }}*-build/*.tgz; do
             # determine the build variant based on the artifact filename
             variant=$(sdk-src/build_scripts/desktop/get_variant.sh "${pkg}")
-            sdk-src/build_scripts/desktop/package.sh -b ${pkg} -o firebase-cpp-sdk-${{ matrix.sdk_platform }}${{ matrix.suffix }}-package -p ${{ matrix.sdk_platform }} -t bin -d ${variant} -P python3 -j
+            additional_flags=(${verbose_flag})
+            # Several build targets require explicitly-set binutils format to be passed
+            # to package.sh (and thus, to merge_libraries).
+            if [[ "${{ matrix.sdk_platform }}" == "darwin" && "${variant}" == "arm64" ]]; then
+              # MacOS ARM
+              additional_flags+=(-f mach-o-arm64)
+            elif [[ "${{ matrix.sdk_platform }}" == "windows" && "${variant}" == *"/x64/"* ]]; then
+              # Windows x64
+              additional_flags+=(-f pe-x86-64,pe-bigobj-x86-64)
+            elif [[ "${{ matrix.sdk_platform }}" == "windows" && "${variant}" == *"/x86/"* ]]; then
+              # Windows x86
+              additional_flags+=(-f pe-i386,pe-bigobj-i386)
+            fi
+            sdk-src/build_scripts/desktop/package.sh -b ${pkg} -o firebase-cpp-sdk-${{ matrix.sdk_platform }}${{ matrix.suffix }}-package -p ${{ matrix.sdk_platform }} -t bin -d ${variant} -P python3 -j ${additional_flags[*]}
           done
           if [[ "${{ matrix.sdk_platform }}" == "darwin" ]]; then
             # Darwin has a final step after all the variants are done,
@@ -672,7 +703,7 @@ jobs:
         python scripts/gha/restore_secrets.py --passphrase "${{ secrets.TEST_SECRET }}"
     - name: Build integration tests
       run: |
-        python scripts/gha/build_testapps.py --t ${{ env.apis }} --p ${{ matrix.target_platform }} --sdk_dir firebase_cpp_sdk --output_directory "${{ github.workspace }}" --noadd_timestamp
+        python scripts/gha/build_testapps.py --t ${{ env.apis }} --p ${{ matrix.target_platform }} --packaged_sdk firebase_cpp_sdk --output_directory "${{ github.workspace }}" --noadd_timestamp
     
     - name: Run desktop integration tests
       if: matrix.target_platform == 'Desktop' && !cancelled()

.github/workflows/integration_tests.yml

@@ -136,7 +136,7 @@ jobs:
           if [[ "${{ matrix.ssl_variant }}" == "boringssl" ]]; then
             ssl_option=--cmake_flag=-DFIREBASE_USE_BORINGSSL=ON
           fi
-          python scripts/gha/build_testapps.py --t ${{ github.event.inputs.apis }} --p ${{ matrix.target_platform }} --output_directory "${{ github.workspace }}" --use_vcpkg --noadd_timestamp ${ssl_option}
+          python scripts/gha/build_testapps.py --t ${{ github.event.inputs.apis }} --p ${{ matrix.target_platform }} --output_directory "${{ github.workspace }}" --noadd_timestamp ${ssl_option}
 
       - name: Run desktop integration tests
         if: matrix.target_platform == 'Desktop' && !cancelled()

app/instance_id/instance_id_desktop_impl.cc

@@ -721,7 +721,7 @@ bool InstanceIdDesktopImpl::FetchServerToken(const char* scope, bool* retry) {
       size_t component_start = 0;
       size_t component_end;
       do {
-        component_end = error.find(":", component_start);
+        component_end = error.find(':', component_start);
         std::string error_component =
             error.substr(component_start, component_end - component_start);
         if (error_component == kPhoneRegistrationError) {

app/src/locale.cc

@@ -69,12 +69,12 @@ std::string GetLocale() {
   // Some of the environment variables have a "." after the name to specify the
   // terminal encoding, e.g.  "en_US.UTF-8", so we want to cut the string on the
   // ".".
-  size_t cut = output.find(".");
+  size_t cut = output.find('.');
   if (cut != std::string::npos) {
     output = output.substr(0, cut);
   }
   // Do the same with a "@" which some locales also have.
-  cut = output.find("@");
+  cut = output.find('@');
   if (cut != std::string::npos) {
     output = output.substr(0, cut);
   }

app/tests/locale_test.cc

@@ -49,7 +49,7 @@ TEST_F(LocaleTest, TestGetLocale) {
   // Make sure this looks like a locale, e.g. has at least 5 characters and
   // contains an underscore.
   EXPECT_GE(loc.size(), 5);
-  EXPECT_NE(loc.find("_"), std::string::npos);
+  EXPECT_NE(loc.find('_'), std::string::npos);
 }
 
 }  // namespace internal

auth/src/desktop/rpcs/error_codes.cc

@@ -225,7 +225,7 @@ AuthError GetAuthErrorCode(const std::string& error,
     // Couldn't find it, check again with just the first word.
     // Sometimes the backend will return the error code along with
     // some explanatory text (see kAuthErrorNoSuchProvider above).
-    size_t space = error.find(" ");
+    size_t space = error.find(' ');
     if (space != std::string::npos) {
       std::string first_word = error.substr(0, space);
       found = find_error(first_word);

build_scripts/desktop/package.sh

@@ -13,6 +13,7 @@ options:
   -m, merge_libraries.py path                     default: <script dir>/../../scripts/merge_libraries.py
   -P, python command                              default: python
   -t, packaging tools directory                   default: ~/bin
+  -f, binutils format                             default: [auto-detect]
   -j, run merge_libraries jobs in parallel
   -v, enable verbose mode
 example:
@@ -29,6 +30,7 @@ root_dir=$(cd $(dirname $0)/../..; pwd -P)
 merge_libraries_script=${root_dir}/scripts/merge_libraries.py
 tools_path=~/bin
 built_sdk_tarfile=
+binutils_format=
 temp_dir=
 run_in_parallel=0
 
@@ -44,8 +46,11 @@ abspath(){
     fi
 }
 
-while getopts "b:o:p:d:m:P:t:hjv" opt; do
+while getopts "f:b:o:p:d:m:P:t:hjv" opt; do
     case $opt in
+        f)
+            binutils_format=$OPTARG
+            ;;
         b)
             built_sdk_path=$OPTARG
             ;;
@@ -252,6 +257,11 @@ fi
 if [[ ${verbose} -eq 1 ]]; then
     merge_libraries_params+=(--verbosity=3)
 fi
+if [[ -n "${binutils_format}" ]]; then
+    merge_libraries_params+=(--force_binutils_target="${binutils_format}")
+fi
+
+
 
 if [[ ! -x "${binutils_objcopy}" || ! -x "${binutils_ar}" || ! -x "${binutils_nm}" ]]; then
     echo "Packaging tools not found at path '${tools_path}'."
@@ -327,12 +337,12 @@ for product in ${product_list[*]}; do
     rm -f "${outfile}"
     if [[ ${verbose} -eq 1 ]]; then
       echo "${python_cmd}" "${merge_libraries_script}" \
-		    ${merge_libraries_params[*]} \
+                    ${merge_libraries_params[*]} \
                     ${cache_param} \
-		    --output="${outfile}" \
-		    --scan_libs="${allfiles}" \
-		    --hide_c_symbols="${deps_hidden}" \
-		    ${libfile_src} ${deps[*]}
+                    --output="${outfile}" \
+                    --scan_libs="${allfiles}" \
+                    --hide_c_symbols="${deps_hidden}" \
+                    ${libfile_src} ${deps[*]}
     fi
     # Place the merge command in a script so we can optionally run them in parallel.
     echo "#!/bin/bash -e" > "${merge_libraries_tmp}/merge_${product}.sh"
@@ -342,7 +352,7 @@ for product in ${product_list[*]}; do
       echo "echo \"${libfile_out}\"" >> "${merge_libraries_tmp}/merge_${product}.sh"
     fi
     if [[ ! -z ${deps_basenames[*]} ]]; then
-	echo -n  >> "${merge_libraries_tmp}/merge_${product}.sh"
+        echo -n  >> "${merge_libraries_tmp}/merge_${product}.sh"
     fi
     echo >> "${merge_libraries_tmp}/merge_${product}.sh"
     echo "\"${python_cmd}\" \\

cmake/external_rules.cmake

@@ -148,6 +148,12 @@ function(build_external_dependencies)
         -DCMAKE_BUILD_TYPE="${CMAKE_BUILD_TYPE}")
   endif()
 
+  if (CMAKE_VERBOSE_MAKEFILE)
+    # If verbose mode was enabled, pass it on
+    set(CMAKE_SUB_CONFIGURE_OPTIONS ${CMAKE_SUB_CONFIGURE_OPTIONS}
+        "-DCMAKE_VERBOSE_MAKEFILE=${CMAKE_VERBOSE_MAKEFILE}")
+  endif()
+
   if(APPLE)
     # Propagate MacOS build flags.
     if(CMAKE_OSX_ARCHITECTURES)

firestore/src/android/firestore_android.cc

@@ -178,17 +178,24 @@ class JavaFirestoreMap {
   jni::Global<jni::HashMap> firestores_;
 };
 
-// Governed by init_mutex_below as well.
-JavaFirestoreMap* java_firestores_ = nullptr;
+// init_mutex protects all the globals below.
+Mutex init_mutex;  // NOLINT
+int initialize_count = 0;
+Loader* global_loader = nullptr;
+
+JavaFirestoreMap* java_firestores = nullptr;
+
+// The initial value for setLoggingEnabled.
+enum class InitialLogState {
+  kUnset,
+  kSetEnabled,
+  kSetDisabled,
+} initial_log_state = InitialLogState::kUnset;
 
 }  // namespace
 
 const char kApiIdentifier[] = "Firestore";
 
-Mutex FirestoreInternal::init_mutex_;  // NOLINT
-int FirestoreInternal::initialize_count_ = 0;
-Loader* FirestoreInternal::loader_ = nullptr;
-
 FirestoreInternal::FirestoreInternal(App* app) {
   FIREBASE_ASSERT(app != nullptr);
   if (!Initialize(app)) return;
@@ -200,7 +207,7 @@ FirestoreInternal::FirestoreInternal(App* app) {
   FIREBASE_ASSERT(java_firestore.get() != nullptr);
   obj_ = java_firestore;
 
-  java_firestores_->Put(env, java_firestore, this);
+  java_firestores->Put(env, java_firestore, this);
 
   // Mainly for enabling TimestampsInSnapshotsEnabled. The rest comes from the
   // default in native SDK. The C++ implementation relies on that for reading
@@ -218,21 +225,22 @@ FirestoreInternal::FirestoreInternal(App* app) {
 
 /* static */
 bool FirestoreInternal::Initialize(App* app) {
-  MutexLock init_lock(init_mutex_);
-  if (initialize_count_ == 0) {
+  MutexLock init_lock(init_mutex);
+  if (initialize_count == 0) {
     jni::Initialize(app->java_vm());
 
-    FIREBASE_DEV_ASSERT(java_firestores_ == nullptr);
-    java_firestores_ = new JavaFirestoreMap();
+    FIREBASE_DEV_ASSERT(java_firestores == nullptr);
+    java_firestores = new JavaFirestoreMap();
 
+    Env env = GetEnv();
     Loader loader(app);
     loader.AddEmbeddedFile(::firebase_firestore::firestore_resources_filename,
                            ::firebase_firestore::firestore_resources_data,
                            ::firebase_firestore::firestore_resources_size);
     loader.CacheEmbeddedFiles();
 
     jni::Object::Initialize(loader);
-
+    jni::String::Initialize(env, loader);
     jni::ArrayList::Initialize(loader);
     jni::Boolean::Initialize(loader);
     jni::Collection::Initialize(loader);
@@ -272,32 +280,42 @@ bool FirestoreInternal::Initialize(App* app) {
     TransactionInternal::Initialize(loader);
     WriteBatchInternal::Initialize(loader);
     if (!loader.ok()) {
-      ReleaseClasses(app);
+      ReleaseClassesLocked(env);
       return false;
     }
 
-    FIREBASE_DEV_ASSERT(loader_ == nullptr);
-    loader_ = new Loader(Move(loader));
+    FIREBASE_DEV_ASSERT(global_loader == nullptr);
+    global_loader = new Loader(Move(loader));
+
+    if (initial_log_state != InitialLogState::kUnset) {
+      bool enabled = initial_log_state == InitialLogState::kSetEnabled;
+      env.Call(kSetLoggingEnabled, enabled);
+    }
   }
-  initialize_count_++;
+  initialize_count++;
   return true;
 }
 
 /* static */
-void FirestoreInternal::ReleaseClasses(App* app) {
-  delete loader_;
-  loader_ = nullptr;
+void FirestoreInternal::ReleaseClassesLocked(Env& env) {
+  // Assumes `init_mutex` is held.
+  String::Terminate(env);
+
+  delete global_loader;
+  global_loader = nullptr;
 }
 
 /* static */
 void FirestoreInternal::Terminate(App* app) {
-  MutexLock init_lock(init_mutex_);
-  FIREBASE_ASSERT(initialize_count_ > 0);
-  initialize_count_--;
-  if (initialize_count_ == 0) {
-    ReleaseClasses(app);
-    delete java_firestores_;
-    java_firestores_ = nullptr;
+  MutexLock init_lock(init_mutex);
+  FIREBASE_ASSERT(initialize_count > 0);
+  initialize_count--;
+  if (initialize_count == 0) {
+    Env env(app->GetJNIEnv());
+    ReleaseClassesLocked(env);
+
+    delete java_firestores;
+    java_firestores = nullptr;
   }
 }
 
@@ -316,7 +334,7 @@ FirestoreInternal::~FirestoreInternal() {
 
   future_manager_.ReleaseFutureApi(this);
 
-  java_firestores_->Remove(env, obj_);
+  java_firestores->Remove(env, obj_);
 
   Terminate(app_);
   app_ = nullptr;
@@ -512,13 +530,28 @@ void Firestore::set_log_level(LogLevel log_level) {
   // "Info", "warning", "error", and "assert" map to logging disabled.
   bool logging_enabled = log_level < LogLevel::kLogLevelInfo;
 
+  {
+    MutexLock lock(init_mutex);
+
+    // Set the initial_log_state on every invocation, just in case Firestore
+    // is terminated for long enough to unload the Firestore classes within
+    // the JVM.
+    initial_log_state = logging_enabled ? InitialLogState::kSetEnabled
+                                        : InitialLogState::kSetDisabled;
+
+    if (initialize_count < 1) {
+      // Avoid invoking Java methods before Firestore has been initialized.
+      return;
+    }
+  }
+
   Env env = FirestoreInternal::GetEnv();
   env.Call(kSetLoggingEnabled, logging_enabled);
 }
 
 FirestoreInternal* FirestoreInternal::RecoverFirestore(
     Env& env, const Object& java_firestore) {
-  return java_firestores_->Get(env, java_firestore);
+  return java_firestores->Get(env, java_firestore);
 }
 
 void FirestoreInternal::SetClientLanguage(const std::string& language_token) {

firestore/src/android/firestore_android.h

@@ -178,13 +178,9 @@ class FirestoreInternal {
   void ShutdownUserCallbackExecutor(jni::Env& env);
 
   static bool Initialize(App* app);
-  static void ReleaseClasses(App* app);
+  static void ReleaseClassesLocked(jni::Env& env);
   static void Terminate(App* app);
 
-  static Mutex init_mutex_;
-  static int initialize_count_;
-  static jni::Loader* loader_;
-
   jni::Global<jni::Object> user_callback_executor_;
 
   App* app_ = nullptr;