Skip to content

Remove the Firestore Snappy patch #932

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
Closed
wants to merge 40 commits into from
Closed

Remove the Firestore Snappy patch #932

wants to merge 40 commits into from

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented May 6, 2022 •
edited
Loading

Remove the logic to patch the firebase-ios-sdk dependency to add Snappy support.

This PR will need to be merged when this SDK updates its dependency on the firebase-ios-sdk repository since firebase/firebase-ios-sdk#9596 has been merged to add Snappy support directly in the iOS SDK.

The Snappy patching was originally added by #885. This PR effectively reverts that PR now that Snappy support is natively added to the firebase-ios-sdk repository.

Googlers see b/227782613 for more details.

@dconeybe dconeybe added api: firestore skip-release-notes Skip release notes check labels May 6, 2022
@dconeybe dconeybe self-assigned this May 6, 2022
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label May 9, 2022
@github-actions
Copy link

github-actions bot commented May 9, 2022 •
edited
Loading

✅  Integration test succeeded!

Requested by @dconeybe on commit a31c796
Last updated: Tue Jun 21 15:49 PDT 2022
View integration test log & download artifacts

@jonsimantov
Copy link
Contributor

Looks like there are some failures, at least on Windows

@dconeybe
Copy link
Contributor Author

Looks like there are some failures, at least on Windows

I'm not sure what was going on there. I've merged in main to see if that fixes anything. The desktop.yml that was used was not consistent with HEAD of the main branch.

@dconeybe
Copy link
Contributor Author

@jonsimantov It looks like ctest is trying to run the grpc tests, and those tests don't get compiled. This is odd because when I build everything via cmake locally, ctest does not try to run the grpc tests. Any ideas why it's trying to run the grpc tests and failing to build them?

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label May 10, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label May 10, 2022
@dconeybe dconeybe mentioned this pull request May 10, 2022
Merged
@jonsimantov
Copy link
Contributor

Hmm, I'm not sure why that would be any different here than in previous occasions. @a-maurice any ideas?

@dconeybe
Copy link
Contributor Author

@jonsimantov @a-maurice Many of the same build errors occur when pointing the C++ SDK at the HEAD of the master branch of the firebase-ios-sdk repository: #950. I'm going to move my investigation to that PR until it goes green.

@dconeybe
Copy link
Contributor Author

I'm guessing that the upgrade of grpc in the iOS SDK is the culprit: firebase/firebase-ios-sdk#9488. I think we just need to set the cmake cache variable to disable the "re2" tests (RE2_BUILD_TESTING).

@dconeybe dconeybe mentioned this pull request May 20, 2022
Closed
dconeybe added a commit that referenced this pull request May 25, 2022
@dconeybe
Patch in #932 to get the C++ SDK to build successfully against HEAD o…
a1d4c3e 
…f the firebase-ios-sdk
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jun 7, 2022
@github-actions github-actions bot added tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 7, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 7, 2022
@dconeybe dconeybe requested a review from jonsimantov June 7, 2022 22:53
@dconeybe
Copy link
Contributor Author

dconeybe commented Jun 7, 2022

@jonsimantov Can you take a look at this PR? I won't merge it into main but rather into the PR that updates the iOS dependencies.

jonsimantov
jonsimantov reviewed Jun 8, 2022
View reviewed changes
cmake/external/firestore.cmake Outdated
URL https://github.com/firebase/firebase-ios-sdk/archive/${version}.tar.gz
GIT_REPOSITORY "https://github.com/firebase/firebase-ios-sdk.git"
# Pinned HEAD commit as of June 01, 2022 @ 10:51 EDT.
GIT_TAG 89c38d1ed908dbc10d5f9f7aded4cf271113773f
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't GIT_TAG be ${version}, which will be updated to 9.2.0 or whatever when the time comes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes, this particular part of the PR will be reverted. It's just here so I can test again the HEAD of the firebase-ios-sdk.

@jonsimantov jonsimantov self-requested a review June 10, 2022 17:34
jonsimantov
jonsimantov previously approved these changes Jun 10, 2022
View reviewed changes
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.

LGTM, but as noted in the PR, we'll keep this a draft and merge it into the upcoming iOS dependency update.

@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jun 21, 2022
@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-requested: quick Trigger a quick set of integration tests. tests: succeeded This PR's integration tests succeeded. labels Jun 21, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 21, 2022
firebase-workflow-trigger-bot and others added 2 commits June 22, 2022 20:30
Update iOS dependencies - Wed Jun 22 2022
68d3768 
### iOS

- Firebase/Analytics → 9.2.0
- Firebase/Auth → 9.2.0
- Firebase/Core → 9.2.0
- Firebase/Crashlytics → 9.2.0
- Firebase/Database → 9.2.0
- Firebase/DynamicLinks → 9.2.0
- Firebase/Firestore → 9.2.0
- Firebase/Functions → 9.2.0
- Firebase/Installations → 9.2.0
- Firebase/Messaging → 9.2.0
- Firebase/RemoteConfig → 9.2.0
- Firebase/Storage → 9.2.0

> Created by [Update Android and iOS dependencies workflow](https://github.com/firebase/firebase-cpp-sdk/actions/runs/2544841237).
@dconeybe
Merge remote-tracking branch 'origin/workflow/auto-update-deps-116-20…
1f15efb 
…220622-200757' into SnappyPatchRevert
@dconeybe dconeybe changed the base branch from main to workflow/auto-update-deps-116-20220622-200757 June 22, 2022 20:34
@github-actions github-actions bot dismissed jonsimantov’s stale review June 22, 2022 20:34

🍞 Dismissed stale approval on external PR.

@dconeybe dconeybe marked this pull request as ready for review June 22, 2022 20:34
dconeybe added a commit that referenced this pull request Jun 22, 2022
@dconeybe
Merge in #932 (Snappy patch revert)
77f3e70
jonsimantov pushed a commit that referenced this pull request Jun 23, 2022
@firebase-workflow-trigger @dconeybe
Update iOS dependencies - Wed Jun 22 2022 (#1003)
dc13f63 
* Update iOS dependencies - Wed Jun 22 2022

### iOS

- Firebase/Analytics → 9.2.0
- Firebase/Auth → 9.2.0
- Firebase/Core → 9.2.0
- Firebase/Crashlytics → 9.2.0
- Firebase/Database → 9.2.0
- Firebase/DynamicLinks → 9.2.0
- Firebase/Firestore → 9.2.0
- Firebase/Functions → 9.2.0
- Firebase/Installations → 9.2.0
- Firebase/Messaging → 9.2.0
- Firebase/RemoteConfig → 9.2.0
- Firebase/Storage → 9.2.0

> Created by [Update Android and iOS dependencies workflow](https://github.com/firebase/firebase-cpp-sdk/actions/runs/2544841237).

* Merge in #932 (Snappy patch revert)

Co-authored-by: firebase-workflow-trigger-bot <[email protected]>
Co-authored-by: Denver Coneybeare <[email protected]>
Base automatically changed from workflow/auto-update-deps-116-20220622-200757 to main June 23, 2022 05:15
@dconeybe
Copy link
Contributor Author

The changes in this PR were merged into the main branch in #1003

@dconeybe dconeybe closed this Jun 24, 2022
@dconeybe dconeybe deleted the dconeybe/SnappyPatchRevert branch June 24, 2022 16:12
@firebase firebase locked and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonsimantov jonsimantov jonsimantov left review comments

Assignees

@jonsimantov jonsimantov

@dconeybe dconeybe

Labels
api: firestore skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dconeybe @jonsimantov @wu-hui