Skip to content

Commit 55c5e22

Browse files
Fix Firebase Storage C++ SDK desktop build issues
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.
1 parent 0f08f58 commit 55c5e22

15 files changed

+266
-112
lines changed

cmake/external/leveldb.cmake

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,22 @@ if(TARGET leveldb)
1818
return()
1919
endif()
2020

21+
set(current_patch_file "") # Explicitly initialize for this scope
2122
if (DESKTOP AND MSVC)
22-
set(patch_file
23-
${CMAKE_CURRENT_LIST_DIR}/../../scripts/git/patches/leveldb/0001-leveldb-1.23-windows-paths.patch)
23+
set(current_patch_file
24+
${CMAKE_CURRENT_LIST_DIR}/../../scripts/git/patches/leveldb/0001-leveldb-1.23-windows-paths.patch)
2425
endif()
2526

2627
# This version must be kept in sync with the version in firestore.patch.txt.
2728
# If this version ever changes then make sure to update the version in
2829
# firestore.patch.txt accordingly.
2930
set(version 1.23)
3031

32+
set(final_patch_command "")
33+
if(current_patch_file)
34+
set(final_patch_command git apply ${current_patch_file} && git gc --aggressive)
35+
endif()
36+
3137
ExternalProject_Add(
3238
leveldb
3339

@@ -42,5 +48,5 @@ ExternalProject_Add(
4248
INSTALL_COMMAND ""
4349
TEST_COMMAND ""
4450
HTTP_HEADER "${EXTERNAL_PROJECT_HTTP_HEADER}"
45-
PATCH_COMMAND git apply ${patch_file} && git gc --aggressive
51+
PATCH_COMMAND ${final_patch_command}
4652
)

storage/src/common/list_result.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ using internal::ListResultInternal;
4242
// Global function to be called by CleanupNotifier
4343
// This function is responsible for cleaning up the internal state of a
4444
// ListResult object when the App is being shut down.
45-
static void GlobalCleanupListResult(void* list_result_void) {
45+
void GlobalCleanupListResult(void* list_result_void) {
4646
if (list_result_void) {
4747
ListResult* list_result = static_cast<ListResult*>(list_result_void);
4848
// This method will delete internal_ and set it to nullptr.

storage/src/common/storage_reference.cc

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,14 @@ StorageReference::StorageReference(StorageReferenceInternal* internal)
8787
}
8888

8989
StorageReference::StorageReference(const StorageReference& other)
90-
: internal_(other.internal_ ? new StorageReferenceInternal(*other.internal_)
91-
: nullptr) {
90+
: internal_(other.internal_ ? other.internal_->Clone() : nullptr) {
9291
StorageReferenceInternalCommon::RegisterForCleanup(this, internal_);
9392
}
9493

9594
StorageReference& StorageReference::operator=(const StorageReference& other) {
95+
if (this == &other) return *this;
9696
StorageReferenceInternalCommon::DeleteInternal(this);
97-
internal_ = other.internal_ ? new StorageReferenceInternal(*other.internal_)
98-
: nullptr;
97+
internal_ = other.internal_ ? other.internal_->Clone() : nullptr;
9998
StorageReferenceInternalCommon::RegisterForCleanup(this, internal_);
10099
return *this;
101100
}
@@ -269,9 +268,7 @@ Future<ListResult> StorageReference::ListAll() {
269268
}
270269

271270
Future<ListResult> StorageReference::ListLastResult() {
272-
if (!internal_) return Future<ListResult>();
273-
return static_cast<const Future<ListResult>&>(
274-
internal_->future()->LastResult(kStorageReferenceFnList));
271+
return internal_ ? internal_->ListLastResult() : Future<ListResult>();
275272
}
276273

277274
} // namespace storage
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
#ifndef FIREBASE_STORAGE_SRC_COMMON_STORAGE_REFERENCE_INTERNAL_H_
2+
#define FIREBASE_STORAGE_SRC_COMMON_STORAGE_REFERENCE_INTERNAL_H_
3+
4+
#include <string>
5+
#include <vector>
6+
7+
#include "firebase/future.h"
8+
#include "firebase/storage/metadata.h"
9+
// Listener and Controller are used in method signatures, need full declaration or forward
10+
#include "firebase/storage/listener.h"
11+
#include "firebase/storage/controller.h"
12+
13+
namespace firebase {
14+
namespace storage {
15+
16+
// Forward declarations from public API
17+
class Storage;
18+
class ListResult;
19+
class StorageReference;
20+
21+
22+
namespace internal {
23+
24+
class StorageInternal; // Platform-specific internal helper for Storage.
25+
26+
// Defines the common internal interface for StorageReference.
27+
// Platform-specific implementations (Desktop, Android, iOS) will derive from this.
28+
class StorageReferenceInternal {
29+
public:
30+
virtual ~StorageReferenceInternal() = default;
31+
32+
// Interface methods mirroring the public StorageReference API
33+
virtual Storage* storage() const = 0;
34+
virtual StorageReferenceInternal* Child(const char* path) const = 0;
35+
virtual std::string bucket() const = 0;
36+
virtual std::string full_path() const = 0;
37+
virtual std::string name() = 0; // Desktop implementation is not const
38+
virtual StorageReferenceInternal* GetParent() = 0; // Desktop implementation is not const
39+
40+
virtual Future<void> Delete() = 0;
41+
virtual Future<void> DeleteLastResult() = 0;
42+
43+
virtual Future<size_t> GetFile(const char* path, Listener* listener,
44+
Controller* controller_out) = 0;
45+
virtual Future<size_t> GetFileLastResult() = 0;
46+
47+
virtual Future<size_t> GetBytes(void* buffer, size_t buffer_size,
48+
Listener* listener,
49+
Controller* controller_out) = 0;
50+
virtual Future<size_t> GetBytesLastResult() = 0;
51+
52+
virtual Future<std::string> GetDownloadUrl() = 0;
53+
virtual Future<std::string> GetDownloadUrlLastResult() = 0;
54+
55+
virtual Future<Metadata> GetMetadata() = 0;
56+
virtual Future<Metadata> GetMetadataLastResult() = 0;
57+
58+
virtual Future<Metadata> UpdateMetadata(const Metadata* metadata) = 0;
59+
virtual Future<Metadata> UpdateMetadataLastResult() = 0;
60+
61+
virtual Future<Metadata> PutBytes(const void* buffer, size_t buffer_size,
62+
Listener* listener,
63+
Controller* controller_out) = 0;
64+
virtual Future<Metadata> PutBytes(const void* buffer, size_t buffer_size,
65+
const Metadata* metadata, Listener* listener,
66+
Controller* controller_out) = 0;
67+
virtual Future<Metadata> PutBytesLastResult() = 0;
68+
69+
virtual Future<Metadata> PutFile(const char* path, Listener* listener,
70+
Controller* controller_out) = 0;
71+
virtual Future<Metadata> PutFile(const char* path, const Metadata* metadata,
72+
Listener* listener,
73+
Controller* controller_out) = 0;
74+
virtual Future<Metadata> PutFileLastResult() = 0;
75+
76+
virtual Future<ListResult> List(int32_t max_results) = 0;
77+
virtual Future<ListResult> List(int32_t max_results,
78+
const char* page_token) = 0;
79+
virtual Future<ListResult> ListAll() = 0;
80+
virtual Future<ListResult> ListLastResult() = 0;
81+
82+
// Common utility methods needed by StorageReference and platform implementations
83+
virtual StorageInternal* storage_internal() const = 0;
84+
virtual StorageReferenceInternal* Clone() const = 0;
85+
86+
protected:
87+
StorageReferenceInternal() = default;
88+
89+
private:
90+
// Public class is a friend to access constructor & internal_
91+
friend class firebase::storage::StorageReference;
92+
93+
// Disallow copy/move of the base class directly; cloning is explicit.
94+
StorageReferenceInternal(const StorageReferenceInternal&) = delete;
95+
StorageReferenceInternal& operator=(const StorageReferenceInternal&) = delete;
96+
StorageReferenceInternal(StorageReferenceInternal&&) = delete;
97+
StorageReferenceInternal& operator=(StorageReferenceInternal&&) = delete;
98+
};
99+
100+
} // namespace internal
101+
} // namespace storage
102+
} // namespace firebase
103+
104+
#endif // FIREBASE_STORAGE_SRC_COMMON_STORAGE_REFERENCE_INTERNAL_H_

storage/src/desktop/controller_desktop.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ int64_t ControllerInternal::total_byte_count() {
8282
// Returns the StorageReference associated with this Controller.
8383
StorageReferenceInternal* ControllerInternal::GetReference() const {
8484
MutexLock lock(mutex_);
85-
return reference_.is_valid()
86-
? new StorageReferenceInternal(*reference_.internal_)
85+
return reference_.is_valid() && reference_.internal_ != nullptr
86+
? reference_.internal_->Clone()
8787
: nullptr;
8888
}
8989

storage/src/desktop/metadata_desktop.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,9 @@ void MetadataInternal::UpdateStorageInternal() {
144144
}
145145

146146
StorageReferenceInternal* MetadataInternal::GetReference() const {
147-
return new StorageReferenceInternal(*storage_reference_.internal_);
147+
return storage_reference_.is_valid() && storage_reference_.internal_ != nullptr
148+
? storage_reference_.internal_->Clone()
149+
: nullptr;
148150
}
149151

150152
std::string MetadataInternal::LookUpString(Variant* root, const char* key,

storage/src/desktop/storage_desktop.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,20 +77,20 @@ StorageInternal::~StorageInternal() {
7777

7878
// Get a StorageReference to the root of the database.
7979
StorageReferenceInternal* StorageInternal::GetReference() const {
80-
return new StorageReferenceInternal(url_, const_cast<StorageInternal*>(this));
80+
return new StorageReferenceInternalDesktop(url_, const_cast<StorageInternal*>(this));
8181
}
8282

8383
// Get a StorageReference for the specified path.
8484
StorageReferenceInternal* StorageInternal::GetReference(
8585
const char* path) const {
86-
return new StorageReferenceInternal(root_.GetChild(path),
86+
return new StorageReferenceInternalDesktop(root_.GetChild(path),
8787
const_cast<StorageInternal*>(this));
8888
}
8989

9090
// Get a StorageReference for the provided URL.
9191
StorageReferenceInternal* StorageInternal::GetReferenceFromUrl(
9292
const char* url) const {
93-
return new StorageReferenceInternal(url, const_cast<StorageInternal*>(this));
93+
return new StorageReferenceInternalDesktop(url, const_cast<StorageInternal*>(this));
9494
}
9595

9696
// Returns the auth token for the current user, if there is a current user,

storage/src/desktop/storage_desktop.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ class StorageInternal {
8282
// Whether this object was successfully initialized by the constructor.
8383
bool initialized() const { return app_ != nullptr; }
8484

85+
// Conforms to common interface, equivalent to initialized() for desktop.
86+
bool app_valid() const { return initialized(); }
87+
8588
// When this is deleted, it will clean up all StorageReferences and other
8689
// objects.
8790
FutureManager& future_manager() { return future_manager_; }

0 commit comments

Comments
 (0)