Skip to content

Replace ThrowInvalidArgument with SimpleThrowInvalidArgument to fix iOS absl linker error in Unity SDK #618

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
Aug 25, 2021

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Aug 25, 2021

This a follow-up fix to #616 which additionally replaces ThrowInvalidArgument with SimpleThrowInvalidArgument to avoid absl-related linker errors in the Unity SDK for iOS.

The "style" of the changes made in this PR were based on those made in other files, such as:

// TODO(b/147444199): use string formatting.
// ThrowInvalidArgument(
// "Invalid query. You are trying to start or end a query using a "
// "document for which the field '%s' (used as the order by) does not
// " "exist.", field_path.CanonicalString());
auto message = std::string(
"Invalid query. You are trying to start or end a "
"query using a document for which the field '") +
field_path.CanonicalString() +
"' (used as the order by) does not exist.";
SimpleThrowInvalidArgument(message);

Googlers can see cl/289114361 for the introduction of SimpleThrowInvalidArgument for context.

@dconeybe dconeybe added api: firestore skip-release-notes Skip release notes check labels Aug 25, 2021
@dconeybe dconeybe self-assigned this Aug 25, 2021
@google-cla google-cla bot added the cla: yes label Aug 25, 2021
@dconeybe dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Aug 25, 2021
@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 Aug 25, 2021
@github-actions
Copy link

github-actions bot commented Aug 25, 2021

✅  Integration test succeeded!

Requested by @dconeybe on commit 6d3ec1d
Last updated: Wed Aug 25 10:22 PDT 2021
View integration test log & download artifacts

@dconeybe dconeybe enabled auto-merge (squash) August 25, 2021 01:34
@dconeybe dconeybe requested a review from var-const August 25, 2021 01:35
@dconeybe dconeybe assigned var-const and unassigned dconeybe Aug 25, 2021
@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Aug 25, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 25, 2021
@ehsannas
Copy link
Contributor

Do we expect the absl linker errors to be resolved anytime soon? If not, should we consider removing ThrowInvalidArgument? It may inadvertently be used again and break releases.

@dconeybe
Copy link
Contributor Author

Do we expect the absl linker errors to be resolved anytime soon? If not, should we consider removing ThrowInvalidArgument? It may inadvertently be used again and break releases.

Yes. It will be solved once the Unity SDK is built from GitHub. See cl/289114361 for more explanation.

Also, ThrowInvalidArgument is in the iOS SDK (not this SDK) so deleting it isn't really a great option since it's used harmlessly throughout the iOS SDK.

https://github.com/firebase/firebase-ios-sdk/blob/6751623a443a01ce6070f87b96c1bc56a86dc56d/Firestore/core/src/util/exception.h#L91-L96

@ehsannas
Copy link
Contributor

Sounds good, Thanks!

Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

Thanks!

@dconeybe dconeybe merged commit 6d3ec1d into main Aug 25, 2021
@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 Aug 25, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Aug 25, 2021
@firebase firebase locked and limited conversation to collaborators Sep 25, 2021
@dconeybe dconeybe deleted the dconeybe/FixThrowInvalidArgumentAbslLinkerError branch October 1, 2021 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: firestore cla: yes skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants