Fix Storage build #1762
Draft
Fix Storage build #1762
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
❌ Integration test FAILEDRequested by @jonsimantov on commit cc7b54f
Add flaky tests to go/fpl-cpp-flake-tracker |
firebase-workflow-trigger
bot
removed
the
tests: in-progress
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Testing
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.