Fix Storage build by jonsimantov · Pull Request #1762 · firebase/firebase-cpp-sdk

bot and others added 12 commits

June 9, 2025 19:24
Adds the `List(max_results, page_token)` and `ListAll()` methods to the
`firebase::storage::StorageReference` C++ API.

These methods allow you to list objects and common prefixes (directories)
within a storage location.

Features:
- Paginated listing using `List(max_results, page_token)`.
- Comprehensive listing using `ListAll()`.
- Returns a `Future<ListResult>`, where `ListResult` contains a list of
  `StorageReference` objects for items and prefixes, and a page token
  for continuation.
- Implemented for Android by calling the underlying Firebase Android SDK's
  list operations via JNI.
- Implemented for iOS by calling the underlying Firebase iOS SDK's list
  operations.
- Desktop platform provides stubs that return `kErrorUnimplemented`.
- Includes comprehensive integration tests covering various scenarios such
  as basic listing, pagination, empty folders, and listing non-existent
  paths.
Here's a summary of the changes:

- Integration Tests:
    - I removed `SKIP_TEST_ON_ANDROID_EMULATOR` from the `ListAllBasic` and `ListPaginated` tests.
    - I refactored the list tests so they create their own unique root folders instead of using a shared one in the test fixture. This improves test isolation.
- Code Style:
    - I removed unnecessary comments that were explaining header includes.
    - I removed placeholder comments from public headers.
    - I updated the copyright year to 2025 in all newly added files.
- Android Performance:
    - I optimized Android's `ListResultInternal` to cache converted items, prefixes, and the page token. This will avoid repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
    - I simplified the `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.
Incorporates feedback from the initial review of the List API:

- Build Fix:
    - Explicitly namespaced StorageReference in list_result.h to resolve undeclared identifier error.
- Integration Tests:
    - Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
    - Refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, improving test isolation.
- Code Style:
    - Removed unnecessary comments explaining header includes.
    - Removed placeholder comments from public headers.
    - Updated copyright year to 2025 in all newly added files.
    - Ran code formatter (scripts/format_code.py -git_diff) on all changed files.
- Android Performance:
    - Optimized Android's ListResultInternal to cache converted items, prefixes, and page token. This avoids repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
    - Simplified ListResult cleanup by removing the ListResultInternalCommon class. ListResult now directly manages the cleanup registration of its internal_ member, similar to StorageReference.
…etween `storage_reference.h` and `list_result.h` by forward-declaring `ListResult` in `storage_reference.h`.

This commit also incorporates your previous feedback from the initial review of the List API:

- Build Fixes:
    - Forward-declared `ListResult` in `storage_reference.h`.
    - Reverted non-standard include placement in `list_result.h`.
    - Explicitly namespaced `StorageReference` in `list_result.h` (previous attempt, kept).
- Integration Tests:
    - Removed `SKIP_TEST_ON_ANDROID_EMULATOR` from `ListAllBasic` and `ListPaginated` tests.
    - Refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, improving test isolation.
- Code Style:
    - Removed unnecessary comments explaining header includes.
    - Removed placeholder comments from public headers.
    - Updated copyright year to 2025 in all newly added files.
    - Ran the code formatter on all changed files.
- Android Performance:
    - Optimized Android's `ListResultInternal` to cache converted items, prefixes, and page token. This avoids repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
    - Simplified `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.
…ack for the List API.

Here's a summary of the changes:

I resolved build errors related to includes and circular dependencies for the Storage List API. Specifically:
- I fixed a circular include dependency between `storage_reference.h` and `list_result.h` by forward-declaring `ListResult` in `storage_reference.h`.
- I corrected the include path for platform-specific `StorageInternal` in `list_result.cc`.

This update also incorporates your previous feedback from the initial review:

- Build Fixes:
    - I forward-declared `ListResult` in `storage_reference.h`.
    - I reverted non-standard include placement in `list_result.h`.
    - I explicitly namespaced `StorageReference` in `list_result.h` (this was a previous attempt that I kept).
    - I corrected the `StorageInternal` include in `list_result.cc`.
- Integration Tests:
    - I removed `SKIP_TEST_ON_ANDROID_EMULATOR` from `ListAllBasic` and `ListPaginated` tests.
    - I refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, which should improve test isolation.
- Code Style:
    - I removed unnecessary comments explaining header includes.
    - I removed placeholder comments from public headers.
    - I updated the copyright year to 2025 in all newly added files.
    - I applied code formatting to all changed files.
- Android Performance:
    - I optimized Android's `ListResultInternal` to cache converted items, prefixes, and page token. This should avoid repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
    - I simplified `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.
Corrects include directives for platform-specific StorageInternal
definitions in list_result_android.h, list_result_ios.h, and
list_result_desktop.h. Removes includes for the deleted
ListResultInternalCommon class.

This commit also incorporates previous feedback and build fixes:

- Build Fixes:
    - Resolved circular include dependency between storage_reference.h and
      list_result.h by forward-declaring ListResult in storage_reference.h.
    - Corrected includes in list_result.cc and platform-specific
      list_result_*.h files to use appropriate platform-specific
      headers for StorageInternal definitions.
- Integration Tests:
    - Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
    - Refactored list tests to create their own unique root folders.
- Code Style:
    - Removed unnecessary comments.
    - Updated copyright years to 2025.
    - Ran code formatter on all changed files.
- Android Performance:
    - Optimized Android's ListResultInternal to cache converted data.
- Cleanup Mechanism:
    - Simplified ListResult cleanup logic.
Addresses all outstanding review comments and build errors for the
Storage List API feature. This includes:

- Build Fixes:
    - Resolved circular include dependency between storage_reference.h and
      list_result.h by forward-declaring ListResult in storage_reference.h.
    - Corrected includes in list_result.cc and platform-specific
      list_result_*.h files to use appropriate platform-specific
      headers for StorageInternal definitions.
- Code Cleanup:
    - Thoroughly removed commented-out code and unused includes.
    - Removed all explanatory comments next to #include directives.
    - Eliminated stray or development-related comments.
- Integration Tests:
    - Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
    - Refactored list tests to create their own unique root folders.
- Code Style:
    - Updated copyright year to 2025 in all newly added files.
    - Ran code formatter (scripts/format_code.py -git_diff) on all changed files.
- Android Performance:
    - Optimized Android's ListResultInternal to cache converted data.
- Cleanup Mechanism:
    - Simplified ListResult cleanup logic by removing ListResultInternalCommon.
This script allows users to specify a workflow name and branch to find
the latest run. It then identifies any failed jobs within that run and
prints the last N lines of logs for each failed step.

Key features:
- Fetches the most recent workflow run for a given workflow and branch.
- Identifies jobs within the run that have a 'failure' conclusion.
- For each failed job, attempts to identify failed steps and extracts their logs.
- Prints the last 500 lines (configurable) of the log for each failed step.
- Handles various scenarios including successful runs, running workflows,
  and missing data.
- Supports GitHub token via CLI arg, env var, or ~/.github_token.
- Auto-detects repository from git remote, or accepts via CLI args.
This commit updates the `print_workflow_run_errors.py` script:

- Workflow name and branch are now optional arguments:
    - `--workflow` (or `--workflow-name`) defaults to "integration_test.yml".
    - `--branch` defaults to the current Git branch.
- Changed default log lines printed from 500 to 100 (`--log-lines`).
- Added `--all-failed-steps` flag:
    - If false (default), only logs for the first failed step in a job are printed.
    - If true, logs for all failed steps in a job are printed.

These changes provide more flexibility and sensible defaults for common use cases.
This commit introduces a grep-like feature to the
`print_workflow_run_errors.py` script.

New features:
- Added `--grep-pattern` (`-g`) argument to specify an Extended
  Regular Expression (ERE) for searching within fetched logs.
- Added `--grep-context` (`-C`) argument to specify the number of
  lines of context to show around matches (default is 5).

Behavior:
- If a grep pattern is provided, the script will use the system `grep`
  command to filter the logs of failed steps (or the full job log
  if a specific step's log cannot be isolated).
- Output clearly indicates when grep results are shown, the pattern used,
  and the context lines.
- Handles cases where `grep` finds no matches or if the `grep` command
  itself fails (e.g., not found, bad pattern).
- If no grep pattern is provided, the script defaults to its previous
  behavior of printing the last N lines of the log.
Resolved several issues preventing the Firebase Storage C++ SDK from building on desktop platforms (Linux):

1.  Corrected leveldb patching mechanism to avoid applying Windows-specific patches on Linux.
2.  Ensured BoringSSL is compiled with Position Independent Code (PIC) by adding -DCMAKE_POSITION_INDEPENDENT_CODE=ON to the main CMake configuration, resolving linker errors with libcurl.
3.  Created missing header files:
    *   `storage/src/common/storage_reference_internal.h` (defines base class for internal StorageReference implementations).
    *   `storage/src/include/firebase/storage/future_details.h` (placeholder, appears to be currently unused but was included).
4.  Refactored the Pimpl hierarchy for `StorageReference`:
    *   Renamed desktop-specific `StorageReferenceInternal` to `StorageReferenceInternalDesktop`.
    *   Made `StorageReferenceInternalDesktop` inherit from the common `StorageReferenceInternal` base class.
    *   Updated method overrides and return types in `StorageReferenceInternalDesktop` and its corresponding .cc file.
    *   Modified `StorageReference` (public class), `ControllerInternal` (desktop), and `MetadataInternal` (desktop) to correctly use `Clone()` for copying internal Pimpl objects and to instantiate the concrete `StorageReferenceInternalDesktop` where appropriate.
5.  Added the missing `kErrorUnimplemented` enum value to `firebase::storage::Error` in `firebase/storage/common.h`.
6.  Added `ClearInternalForCleanup` method declaration to `ListResult` class in its public header.
7.  Corrected various private access errors by adding necessary friend class declarations.
8.  Fixed incorrect `static` linkage for `GlobalCleanupListResult` function.

These changes allow `firebase_storage` to compile successfully on a Linux desktop environment. A C++17 `if constexpr` warning remains in `storage_reference_desktop.cc` but does not prevent the build.

@jonsimantov