Enable Snappy compression support in LevelDb in cmake builds by dconeybe · Pull Request #885 · firebase/firebase-cpp-sdk · GitHub

This PR patches in firebase/firebase-ios-sdk#9596 to enable Snappy compression support in LevelDb databases. This only affects cmake builds.

This change should be reverted once this repository's dependency on the firebase-ios-sdk is upgraded to a version that includes firebase/firebase-ios-sdk#9596.

Snappy support is mainly required for the Unity SDK. All builds of the Unity SDK to date had inadvertently included Snappy support by virtue of being compiled in Google's internal build environment. Builds of the Unity SDK from GitHub, however, did not include Snappy support. Therefore, if a customer upgraded from a version of the Unity SDK that included Snappy support to a version that does not, they would experience a crash like this:

ERROR: FIRESTORE INTERNAL ASSERTION FAILED: /Users/[redacted]/Documents/GitHub/firebase-unity-sdk/macos_unity/bin/external/src/firestore/Firestore/core/src/local/leveldb_transaction.cc(76) void firebase::firestore::local::LevelDbTransaction::Iterator::Seek(const std::string &): leveldb iterator reported an error: Corruption: corrupted compressed block contents (expected db_iter_->status().ok())

The crash occurs because LevelDb tries to read a database that is compressed with Snappy, but Snappy compression support is not compiled in.

Note that only desktop platforms are affected by this crash: Windows, macOS, and Linux. Namely, the mobile platforms are not affected: Android, iOS, and watchOS. Android is not affected because it uses sqlite, not leveldb. iOS and watchOS are not affected because they get their LevelDb dependency from CocoaPods/Swift Package Manager, which never included Snappy support.

Googlers see b/227782613 for more details.

I started the "CPP binary SDK packaging" action to make sure that a usable package is produced: https://github.com/firebase/firebase-cpp-sdk/actions/runs/2109321878

You can see the integration test linker error when using the binary SDK here: https://github.com/firebase/firebase-cpp-sdk/runs/5888940999?check_suite_focus=true

/usr/bin/ld: /home/runner/work/firebase-cpp-sdk/firebase-cpp-sdk/downloaded_sdk/firebase_cpp_sdk/libs/linux/x86_64/legacy/libfirebase_app.a(b9b8ea7a0a0fc9e48d816a3f5d721023_table_builder.cc.o): in function `f_b_leveldb::TableBuilder::WriteBlock(f_b_leveldb::BlockBuilder*, f_b_leveldb::BlockHandle*)':
table_builder.cc:(.text+0x7fc): undefined reference to `snappy::MaxCompressedLength(unsigned long)'
/usr/bin/ld: table_builder.cc:(.text+0x83d): undefined reference to `snappy::RawCompress(char const*, unsigned long, char*, unsigned long*)'
/usr/bin/ld: /home/runner/work/firebase-cpp-sdk/firebase-cpp-sdk/downloaded_sdk/firebase_cpp_sdk/libs/linux/x86_64/legacy/libfirebase_app.a(b9b8ea7a0a0fc9e48d816a3f5d721023_format.cc.o): in function `f_b_leveldb::ReadBlock(f_b_leveldb::RandomAccessFile*, f_b_leveldb::ReadOptions const&, f_b_leveldb::BlockHandle const&, f_b_leveldb::BlockContents*)':
format.cc:(.text+0x5a2): undefined reference to `snappy::GetUncompressedLength(char const*, unsigned long, unsigned long*)'
/usr/bin/ld: format.cc:(.text+0x5d3): undefined reference to `snappy::RawUncompress(char const*, unsigned long, char*)'
collect2: error: ld returned 1 exit status

I'll work on getting the packaging stuff merged into this branch, stand by.

…ace to rename list.

…e-cpp-sdk into dconeybe/snappy

Integration tests are now building successfully on desktop against binary SDK: https://github.com/firebase/firebase-cpp-sdk/actions/runs/2117250300

Result looks good (except unrelated flake in RTDB).

Is this enabled in RTDB now as well? (I noticed the linker error happened in the database integration test too, so it may be using it in both?

Also a nit: Desktop and LevelDB need more capital letters (I think probably also fix LevelDB above)

Yes. In cmake, if Firestore is included in the build then both RTDB and Firestore share the LevelDb library from Firestore. However, if Firestore is excluded from the build then RTDB will use a LevelDb without Snappy support. We should probably do something about this. We had a meeting yesterday where it sounded like a good idea might be to unconditionally download the firebase_ios_sdk and use LevelDb from it regardless of whether or not Firestore is being used. Would you like to hold off merging this until we figure that out?

Oh wait, I see you were pointing out that the release notes entry needed a correction. I've updated it in 8bb6bf5.

✅ Integration test succeeded!

Requested by @dconeybe on commit f889992
Last updated: Tue Apr 12 13:43 PDT 2022
View integration test log & download artifacts

The failing tests show that Snappy support is NOT enabled in iOS. I think this will be fine in Unity but I need to confirm.

The failing tests show that Snappy support is NOT enabled in iOS. I think this will be fine in Unity but I need to confirm.

Will we disable the test on iOS to get this checked in, or wait for the iOS dependency to be updated?

Will we disable the test on iOS to get this checked in, or wait for the iOS dependency to be updated?

I will disable the test on iOS. There are no plans to add Snappy support to iOS. I had just assumed that iOS had Snappy support, so configured the test run on iOS. Since iOS does not, that's totally fine and I'll just disable the test. The only reason I'm adding Snappy support at all is for the Desktop builds in the Unity SDK, whose previous releases inadvertently included Snappy support because they were compiled internally in Google instead of in GitHub.

dconeybe added 2 commits April 7, 2022 00:22

firestore.cmake: patch leveldb to enable snappy support

19b11f4

external_rules.cmake: do not overwrite leveldb.cmake from firestore

5eb52fc

dconeybe added the api: firestore label Apr 7, 2022

dconeybe self-assigned this Apr 7, 2022

dconeybe mentioned this pull request Apr 7, 2022

Enable Snappy compression support in LevelDb in cmake builds firebase/firebase-ios-sdk#9596

Merged

release_build_files/readme.md: release notes entry added

7898b46

dconeybe added 3 commits April 7, 2022 17:37

leveldb_snappy_test.cc added

7a057de

leveldb_snappy_test.cc: add newline at end of file

c4b8ed5

Merge remote-tracking branch 'origin/main' into snappy

f9a0ef7

jonsimantov added 2 commits April 8, 2022 11:00

Add snappy to embedded libraries in packaging step, and snappy namesp…

7dfbda9

…ace to rename list.

Merge branch 'dconeybe/snappy' of https://github.com/firebase/firebas…

f6ed5df

…e-cpp-sdk into dconeybe/snappy

Add leveldb_snappy_test.cc into project.pbxproj

0f694b3

jonsimantov reviewed Apr 8, 2022

View reviewed changes

Fix release notes entry to mention RTDB as well as Firestore

8bb6bf5

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 9, 2022

github-actions Bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Apr 9, 2022

github-actions Bot added the tests: failed This PR's integration tests failed. label Apr 9, 2022

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 9, 2022

dconeybe added 2 commits April 11, 2022 14:24

Avoid even compiling leveldb_snappy_test.cc on Android

be2a2c7

leveldb_snappy_test.cc: add SKIP_TEST_ON_IOS

07023a9

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 11, 2022

github-actions Bot added the tests: in-progress This PR's integration tests are in progress. label Apr 11, 2022

github-actions Bot added tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. tests: failed This PR's integration tests failed. labels Apr 11, 2022

leveldb_snappy_test.cc: add missing #include "firebase_test_framework.h"

ec98825

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 11, 2022

github-actions Bot added tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. tests: failed This PR's integration tests failed. labels Apr 11, 2022

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 11, 2022

github-actions Bot added the tests: succeeded This PR's integration tests succeeded. label Apr 11, 2022

dconeybe removed the tests: failed This PR's integration tests failed. label Apr 11, 2022

dconeybe requested a review from jonsimantov April 11, 2022 22:32

jonsimantov approved these changes Apr 12, 2022

View reviewed changes

dconeybe merged commit f889992 into main Apr 12, 2022

dconeybe deleted the dconeybe/snappy branch April 12, 2022 18:24

github-actions Bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests: succeeded This PR's integration tests succeeded. labels Apr 12, 2022

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 12, 2022

dconeybe mentioned this pull request Apr 13, 2022

firestore_snappy.patch.txt: fix linux gcc inlining build error #895

Merged

dconeybe mentioned this pull request May 6, 2022

Remove the Firestore Snappy patch #932

Closed

firebase locked and limited conversation to collaborators May 13, 2022

               -   Changes
                   -   Firestore/Database (Desktop): Upgrade LevelDb dependency to 1.23
                       ([#886](https://github.com/firebase/firebase-cpp-sdk/pull/886)).
+                  -   Firestore (desktop): Enabled Snappy compression support in LevelDb

Enable Snappy compression support in LevelDb in cmake builds by dconeybe · Pull Request #885 · firebase/firebase-cpp-sdk · GitHub

Uh oh!

Enable Snappy compression support in LevelDb in cmake builds - #885

Enable Snappy compression support in LevelDb in cmake builds#885dconeybe merged 13 commits intomainfirebase/firebase-cpp-sdk:mainfrom dconeybe/snappyfirebase/firebase-cpp-sdk:dconeybe/snappyCopy head branch name to clipboard

Conversation

dconeybe commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dconeybe commented Apr 7, 2022

Uh oh!

jonsimantov commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonsimantov commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonsimantov Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

jonsimantov Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe Apr 9, 2022

Choose a reason for hiding this comment

Uh oh!

dconeybe Apr 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Integration test succeeded!

Uh oh!

dconeybe commented Apr 11, 2022

Uh oh!

jonsimantov commented Apr 11, 2022

Uh oh!

dconeybe commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Enable Snappy compression support in LevelDb in cmake builds#885
dconeybe merged 13 commits into
mainfrom
dconeybe/snappy

dconeybe commented Apr 7, 2022 •
edited
Loading

jonsimantov commented Apr 8, 2022 •
edited
Loading

jonsimantov commented Apr 8, 2022 •
edited
Loading

jonsimantov Apr 8, 2022 •
edited
Loading

github-actions Bot commented Apr 9, 2022 •
edited
Loading