Skip to content

When patching snappy for Firestore, add --binary flag to patch command. #9941

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 1 commit into from
Jun 23, 2022

Conversation

jonsimantov
Copy link
Contributor

@jonsimantov jonsimantov commented Jun 23, 2022

Firestore's snappy patching in the firebase-cpp-sdk is broken on GitHub Actions runners after upgrading its firebase-ios-sdk dependency from 9.1.0 to 9.2.0 in firebase/firebase-cpp-sdk#1003:

https://github.com/firebase/firebase-cpp-sdk/runs/7023311255

Note that this PR has some special logic in firebase/firebase-cpp-sdk@77f3e70 to remove the patching of the "snappy" library.

The error from the build log is:

Performing patch step for 'snappy'
patching file snappy.cc
Assertation failed!

Program: C:\Strawberry\c\bin\patch.exe
File: .\src\patch\2.5.9\patch-2.5.9-src\patch.c, Line 354

Expression: hunk

Originally, I was not able to reproduce this locally; however, I was able to reproduce it by doing the following:

  1. Download the latest version of Strawberry Perl portable build (e.g. https://strawberryperl.com/download/5.32.1.1/strawberry-perl-5.32.1.1-64bit-portable.zip)
  2. Extract patch.exe from its c\bin subdirectory into an empty directory (e.g. c:\StrawberryPatch).
  3. Prepend the directory containing patch.exe to the PATH environment variable (e.g. set PATH=c:\StrawberryPatch;%PATH%).
  4. Run the cmake configure command (e.g. cmake -S . -B build -DFIREBASE_CPP_BUILD_TESTS=ON -DCMAKE_TOOLCHAIN_FILE=external\vcpkg\scripts\buildsystems\vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static -A x64)

Watch the output from cmake closely, and you will see the error.

The problem appears to be that the GitHub Actions runners use the patch command that is bundled with Strawberry Perl, which cannot seem to handle the patch file format of cmake/external/firestore.patch.txt.

Note that the patch.exe bundled with git (v2.7.6) does handles the patch just fine. The version of patch bundled with Strawberry Perl, and installed via choco install patch is 2.5.9, which appears to have some sort of bug.

This PR fixes the issue by specifying --binary to patch.exe, which is a no-op on posix systems.

#no-changelog

No-op on POSIX systems but should fix patch.exe crashing on Windows.
@jonsimantov jonsimantov requested a review from dconeybe June 23, 2022 21:04
@jonsimantov jonsimantov changed the title Add --binary flag to patch command. When patching snappy for Firestore, add --binary flag to patch command. Jun 23, 2022
@google-oss-bot
Copy link

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.40% (4cf275f) to 88.36% (a5902be) by -0.04%.

    FilenameBase (4cf275f)Merge (a5902be)Diff
    exception.cc84.21%23.68%-60.53%
    exception_apple.mm65.52%96.55%+31.03%
    leveldb_key.cc98.14%98.82%+0.69%
    ordered_code.cc94.39%93.90%-0.49%

Test Logs

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

@jonsimantov jonsimantov merged commit 6c8a010 into master Jun 23, 2022
@jonsimantov jonsimantov deleted the bugfix/firestore-windows-patch-binary branch June 23, 2022 23:04
@dconeybe
Copy link
Contributor

This fix was effectively patched into the firebase-cpp-sdk in firebase/firebase-cpp-sdk#1006

DellaBitta pushed a commit to firebase/firebase-cpp-sdk that referenced this pull request Jun 24, 2022
In the Firestore iOS SDK, the usage of patch.exe crashes on Windows due to CRLF issues. This is fixed in that repo by firebase/firebase-ios-sdk#9941; use that commit in the C++ SDK, since it was not included in the 9.2.0 release. (The commit was branched off of the CocoaPods-9.2.0 tag.)
@firebase firebase locked and limited conversation to collaborators Jul 24, 2022
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.

3 participants