Add generated docs check and generated headers target, and remove FIREBASE_NAMESPACE macro. (#499)

Run Doxygen as part of the common PR checks.

This will fail if Doxygen returns any errors or warnings. This could include missing or misspelled parameters, missing method documentation, or other issues.

Because we don't perform a full header scrub here, the Doxygen config file uses _sed_ to remove everything between `<SWIG>` and `</SWIG>`. We could potentially perform a second pass with SWIG enabled, or do this in the Unity repo in the future, to check C# documentation.

This PR also fixes some of those same issues present in our build right now, including a few missing method docs in Firestore, a missing parameter in Database, and bad cross-references in a few different places, including all references to App and Future, which were failing due to the FIREBASE_NAMESPACE macro which is no longer needed. _This PR removes all usages of the FIREBASE_NAMESPACE macro._

Note that because we also run Doxygen on some generated headers, this PR includes a new CMake target to build those headers without building the entire build. Any future generated headers should be added to that target.

This PR also contains a small change to print the list of files requiring code formatting directly to the GitHub workflow log.

Author: jonsimantov

.github/workflows/checks.yml

@@ -31,7 +31,7 @@ jobs:
         run: git fetch origin main
       - name: Detect Formatting Changes
         shell: bash
-        run: python3 scripts/format_code.py -git_diff -noformat_file -verbose
+        run: python3 scripts/format_code.py -git_diff -noformat_file -verbose -github_log
 
   check_integration_test_labels:
     # This check fails if integration tests are queued, in progress, or failed.
@@ -42,4 +42,37 @@ jobs:
         none_of: "${{ env.statusLabelInProgress }},${{ env.statusLabelFailed }},${{ env.triggerLabelFull }},${{ env.triggerLabelQuick }}"
         repo_token: ${{ github.token }}
 
-
+  generated_docs_check:
+    # This check succeeds if Doxygen documentation generates without errors.
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          submodules: false
+      - name: Setup python
+        uses: actions/setup-python@v2
+        with:
+          python-version: 3.7
+      - name: Install prerequisites
+        run: python scripts/gha/install_prereqs_desktop.py
+      - name: Generate headers
+        run: |
+          mkdir build
+          cd build
+          cmake ..
+          cmake --build . --target FIREBASE_GENERATED_HEADERS
+      - name: Install doxygen
+        run: sudo apt-get install doxygen
+      - name: Run doxygen
+        run: |
+          cp docs/Doxyfile /tmp/Doxyfile
+          echo INPUT = $(find */src/include/firebase/ build/generated/ -name '*.h') >> /tmp/Doxyfile
+          doxygen /tmp/Doxyfile 2>doxygen_errors.txt
+          cat doxygen_errors.txt
+      - name: Check output
+        run: |
+          if grep -Eq "error:|warning:" doxygen_errors.txt; then
+            # Grep for warnings and print them out (replacing \n with %0A for github log)
+            grep -E "error:|warning:|^  parameter" doxygen_errors.txt | sed ':a;N;$!ba;s/\n/%0A/g' | sed 's/^/::error ::DOXYGEN ERRORS:  %0A/'
+            exit 1
+          fi

CMakeLists.txt

@@ -529,8 +529,12 @@ if(FIREBASE_CPP_BUILD_TESTS)
   add_subdirectory(testing)
 endif()
 
+# Custom target containing all generated headers, used to generate docs only.
+add_custom_target(FIREBASE_GENERATED_HEADERS)
+
 if(NOT FIREBASE_CPP_USE_PRIOR_GRADLE_BUILD)
   add_subdirectory(app)
+  add_dependencies(FIREBASE_GENERATED_HEADERS FIREBASE_APP_GENERATED_HEADERS)
 else()
   # Add firebase_app as a target on the previously built app.
   add_library(firebase_app STATIC IMPORTED GLOBAL)
@@ -552,6 +556,7 @@ if (FIREBASE_INCLUDE_ADMOB)
 endif()
 if (FIREBASE_INCLUDE_ANALYTICS)
   add_subdirectory(analytics)
+  add_dependencies(FIREBASE_GENERATED_HEADERS FIREBASE_ANALYTICS_GENERATED_HEADERS)
 endif()
 if (FIREBASE_INCLUDE_AUTH)
   add_subdirectory(auth)
@@ -564,6 +569,7 @@ if (FIREBASE_INCLUDE_DYNAMIC_LINKS)
 endif()
 if (FIREBASE_INCLUDE_FIRESTORE)
   add_subdirectory(firestore)
+  add_dependencies(FIREBASE_GENERATED_HEADERS FIREBASE_FIRESTORE_GENERATED_HEADERS)
 endif()
 if (FIREBASE_INCLUDE_FUNCTIONS)
   add_subdirectory(functions)

analytics/CMakeLists.txt

@@ -50,6 +50,12 @@ generate_analytics_header(
   "${CMAKE_CURRENT_LIST_DIR}/ios_headers/FIRUserPropertyNames.h"
   "${user_property_names_header}"
 )
+add_custom_target(FIREBASE_ANALYTICS_GENERATED_HEADERS
+  DEPENDS 
+  "${user_property_names_header}"
+  "${parameter_names_header}"
+  "${event_names_header}"
+)
 
 # Common source files used by all platforms
 set(common_SRCS

app/CMakeLists.txt

@@ -43,6 +43,7 @@ binary_to_array("invites_resources"
 # Generate version.h
 set(version_header_dir ${FIREBASE_GEN_FILE_DIR}/app/src/include/firebase)
 set(version_header ${version_header_dir}/version.h)
+add_custom_target(FIREBASE_APP_GENERATED_HEADERS DEPENDS "${version_header}")
 file(MAKE_DIRECTORY ${version_header_dir})
 add_custom_command(
     OUTPUT ${version_header}

app/memory/atomic.h

@@ -22,11 +22,7 @@
 #include <atomic>
 #endif  // !defined(_STLPORT_VERSION)
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 namespace compat {
 
 // For now only the types below are allowed to be atomic. Feel free to add more
@@ -149,5 +145,5 @@ T Atomic<T>::fetch_sub(T arg) {
 
 }  // namespace compat
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 #endif  // FIREBASE_APP_CLIENT_CPP_MEMORY_ATOMIC_H_

app/memory/shared_ptr.h

@@ -23,11 +23,7 @@
 #include "app/meta/move.h"
 #include "app/src/include/firebase/internal/type_traits.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 namespace internal {
 
@@ -49,7 +45,7 @@ class ControlBlock {
   uint64_t ref_count() const { return ref_count_.load(); }
 
  private:
-  ::FIREBASE_NAMESPACE::compat::Atomic<uint64_t> ref_count_;
+  ::firebase::compat::Atomic<uint64_t> ref_count_;
 };
 
 }  // namespace internal
@@ -229,5 +225,5 @@ void SharedPtr<T>::Clear() {
   ctrl_ = nullptr;
 }
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 #endif  // FIREBASE_APP_CLIENT_CPP_MEMORY_SHARED_PTR_H_

app/memory/unique_ptr.h

@@ -20,11 +20,7 @@
 #include "app/meta/move.h"
 #include "app/src/include/firebase/internal/type_traits.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 // Smart pointer that owns another object and releases it when destroyed.
 //
@@ -123,6 +119,6 @@ UniquePtr<T> MakeUnique(Args&&... args) {
 }
 
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_MEMORY_UNIQUE_PTR_H_

app/meta/move.h

@@ -19,11 +19,7 @@
 
 #include "app/src/include/firebase/internal/type_traits.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 // Casts the argument to an rvalue-reference.
 //
@@ -51,6 +47,6 @@ inline T&& Forward(typename remove_reference<T>::type&& arg) {
 }
 
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_META_MOVE_H_

app/src/app_common.cc

@@ -40,11 +40,7 @@
 #define strtok_r strtok_s
 #endif  // FIREBASE_PLATFORM_WINDOWS
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 #ifdef FIREBASE_LINUX_BUILD_CONFIG_STRING
 void CheckCompilerString(const char* input) {
@@ -487,4 +483,4 @@ Logger* FindAppLoggerByName(const char* name) {
 
 }  // namespace app_common
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase

app/src/app_common.h

@@ -25,11 +25,7 @@
 #include "app/src/include/firebase/app.h"
 #include "app/src/logger.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 // Default app name.
 extern const char* const kDefaultAppName;
@@ -102,6 +98,6 @@ Logger* FindAppLoggerByName(const char* name);
 
 }  // namespace app_common
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_SRC_APP_COMMON_H_

app/src/app_identifier.cc

@@ -22,7 +22,7 @@
 
 #include "app/src/include/firebase/app.h"
 
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 namespace internal {
 
 // Generate a unique identifier from the Firebase app options.
@@ -42,4 +42,4 @@ std::string CreateAppIdentifierFromOptions(const AppOptions& options) {
 }
 
 }  // namespace internal
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase

app/src/app_identifier.h

@@ -21,13 +21,13 @@
 
 #include "app/src/include/firebase/app.h"
 
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 namespace internal {
 
 // Generate a unique identifier from the Firebase app options.
 std::string CreateAppIdentifierFromOptions(const AppOptions& options);
 
 }  // namespace internal
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_SRC_APP_IDENTIFIER_H_

app/src/assert.h

@@ -32,10 +32,6 @@
   __FILE__ "(" FIREBASE_EXPAND_STRINGIFY(__LINE__) "): "
 #endif  // defined(NDEBUG)
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
 // FIREBASE_ASSERT_* macros are not compiled out of release builds. They should
 // be used for assertions that need to be propagated to end-users of SDKs.
 // FIREBASE_DEV_ASSERT_* macros are compiled out of release builds, similar to
@@ -47,7 +43,7 @@
 #define FIREBASE_ASSERT_WITH_EXPRESSION(condition, expression)      \
   do {                                                              \
     if (!(condition)) {                                             \
-      FIREBASE_NAMESPACE::LogAssert(                                \
+      firebase::LogAssert(                                          \
           FIREBASE_ASSERT_MESSAGE_PREFIX FIREBASE_EXPAND_STRINGIFY( \
               expression));                                         \
     }                                                               \
@@ -113,10 +109,10 @@
 #define FIREBASE_ASSERT_MESSAGE_WITH_EXPRESSION(condition, expression, ...) \
   do {                                                                      \
     if (!(condition)) {                                                     \
-      FIREBASE_NAMESPACE::LogError(                                         \
+      firebase::LogError(                                                   \
           FIREBASE_ASSERT_MESSAGE_PREFIX FIREBASE_EXPAND_STRINGIFY(         \
               expression));                                                 \
-      FIREBASE_NAMESPACE::LogAssert(__VA_ARGS__);                           \
+      firebase::LogAssert(__VA_ARGS__);                                     \
     }                                                                       \
   } while (false)
 

app/src/callback.cc

@@ -24,11 +24,7 @@
 #include "app/src/semaphore.h"
 #include "app/src/thread.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 namespace callback {
 
 class CallbackEntry;
@@ -339,4 +335,4 @@ void PollCallbacks() {
 
 }  // namespace callback
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase

app/src/callback.h

@@ -24,9 +24,6 @@
 #include <functional>
 #endif
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
 
 /// @cond FIREBASE_APP_INTERNAL
 
@@ -36,7 +33,7 @@
 
 #include <string>
 
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 namespace callback {
 
 /// Initialize the Callback system.
@@ -397,7 +394,7 @@ void PollCallbacks();
 
 }  // namespace callback
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 /// @endcond
 
 #endif  // FIREBASE_APP_CLIENT_CPP_SRC_CALLBACK_H_

app/src/cleanup_notifier.cc

@@ -20,11 +20,7 @@
 
 #include <algorithm>
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 Mutex *CleanupNotifier::cleanup_notifiers_by_owner_mutex_ = new Mutex();
 std::map<void *, CleanupNotifier *>
@@ -121,4 +117,4 @@ CleanupNotifier *CleanupNotifier::FindByOwner(void *owner) {
 }
 
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase

app/src/cleanup_notifier.h

@@ -22,11 +22,7 @@
 
 #include "app/src/mutex.h"
 
-#if !defined(FIREBASE_NAMESPACE)
-#define FIREBASE_NAMESPACE firebase
-#endif
-
-namespace FIREBASE_NAMESPACE {
+namespace firebase {
 
 // If an object gives out other objects that refer back to it, the original
 // object can use this CleanupNotifier class to invalidate any other objects it
@@ -131,6 +127,6 @@ class TypedCleanupNotifier {
 };
 
 // NOLINTNEXTLINE - allow namespace overridden
-}  // namespace FIREBASE_NAMESPACE
+}  // namespace firebase
 
 #endif  // FIREBASE_APP_CLIENT_CPP_SRC_CLEANUP_NOTIFIER_H_