Skip to content

Enable Snappy compression support in LevelDb in cmake builds #9596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
May 20, 2022

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Apr 7, 2022

Enable Snappy compression support in LevelDb databases when compiled via cmake. This does not affect builds from xcode.

Snappy support is mainly required for firebase-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.

This PR was patched into the C++ SDK via firebase/firebase-cpp-sdk#885. This patching will be undone via firebase/firebase-cpp-sdk#932.

Googlers see b/227782613 for more details.

#no-changelog

@google-oss-bot
Copy link

google-oss-bot commented Apr 7, 2022

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.12% (8d65537) to 88.09% (a3ec861) by -0.03%.

    FilenameBase (8d65537)Merge (a3ec861)Diff
    exception.cc84.21%23.68%-60.53%
    exception_apple.mm65.52%96.55%+31.03%
    leveldb_key.cc98.09%98.79%+0.70%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/ucdlKC36cc.html

@dconeybe
Copy link
Contributor Author

dconeybe commented Apr 8, 2022

Some of the checks are failing because cmake's FindPythonInterp is finding a Python2 interpreter instead of Python3. I'm going to use the newer FindPython3 module, if available, to hopefully fix this issue.

@google-oss-bot
Copy link

google-oss-bot commented Apr 13, 2022

Copy link
Contributor

@jonsimantov jonsimantov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing - in the meantime, WDYT about getting the C++ SDK PR drafted (temporarily referencing this branch as its Firestore iOS SDK external cmake source) to make sure the build still works?

@dconeybe
Copy link
Contributor Author

dconeybe commented May 9, 2022

Reviewing - in the meantime, WDYT about getting the C++ SDK PR drafted (temporarily referencing this branch as its Firestore iOS SDK external cmake source) to make sure the build still works?

Done: commit 346a4a4109 from #932

Copy link
Contributor

@cherylEnkidu cherylEnkidu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your changes and the explanations!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants