WorryFree Computers   »   [go: up one dir, main page]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable Snappy compression support in LevelDb in cmake builds #885

Merged
merged 13 commits into from
Apr 12, 2022
Merged

Enable Snappy compression support in LevelDb in cmake builds #885

merged 13 commits into from
Apr 12, 2022

Conversation

dconeybe
Copy link
Contributor
@dconeybe dconeybe commented Apr 7, 2022 •
edited

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.

@dconeybe dconeybe self-assigned this Apr 7, 2022
@dconeybe dconeybe mentioned this pull request Apr 7, 2022
Merged
@dconeybe
Copy link
Contributor Author
dconeybe commented Apr 7, 2022

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

@jonsimantov
Copy link
Contributor
jonsimantov commented Apr 8, 2022 •
edited

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.

@jonsimantov
Copy link
Contributor
jonsimantov commented Apr 8, 2022 •
edited

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).

jonsimantov
jonsimantov reviewed Apr 8, 2022
View reviewed changes
release_build_files/readme.md Outdated
@@ -571,6 +571,8 @@ code.
- 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 9, 2022
@github-actions 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
Copy link
github-actions bot commented Apr 9, 2022 •
edited

✅  Integration test succeeded!

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

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Apr 9, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 9, 2022
@dconeybe
Copy link
Contributor Author

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.

@jonsimantov
Copy link
Contributor

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?

@dconeybe
Copy link
Contributor Author

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 dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 11, 2022
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Apr 11, 2022
@github-actions 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
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Apr 11, 2022
@github-actions 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 firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 11, 2022
@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 11, 2022
@dconeybe dconeybe removed the tests: failed This PR's integration tests failed. label Apr 11, 2022
@dconeybe dconeybe merged commit f889992 into main Apr 12, 2022
@dconeybe dconeybe deleted the dconeybe/snappy branch April 12, 2022 18:24
@github-actions 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 firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 12, 2022
@dconeybe dconeybe mentioned this pull request Apr 13, 2022
Merged
@dconeybe dconeybe mentioned this pull request May 6, 2022
Closed
@firebase firebase locked and limited conversation to collaborators May 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonsimantov jonsimantov jonsimantov approved these changes

Assignees

@dconeybe dconeybe

Labels
api: firestore tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dconeybe @jonsimantov