Fix "Undefined symbols" linker error when DocumentChange::npos was used by dconeybe · Pull Request #474 · firebase/firebase-cpp-sdk · GitHub

Fix a bug where accessing firebase::firestore::DocumentChange::npos from a non-Android platform results in a linker error:

Undefined symbols for architecture x86_64:
  "firebase::firestore::DocumentChange::npos", referenced from:
      firebase_testapp_automated::FirebaseFirestoreBasicTest_TestDocumentChangeNpos_Test::TestBody() in integration_test.cc.o
ld: symbol(s) not found for architecture x86_64

Here is the declaration of npos:

firebase-cpp-sdk/firestore/src/include/firebase/firestore/document_change.h

Lines 66 to 72 in 7ae9569

    
           #if defined(ANDROID) 
        
             // Older NDK (r16b) fails to define this properly. Fix this when support for 
        
             // the older NDK is removed. 
        
             static const std::size_t npos; 
        
           #else 
        
             static constexpr std::size_t npos = static_cast<std::size_t>(-1); 
        
           #endif  // defined(ANDROID)

The fix is to add a definition of npos into the corresponding .cc file

Thanks for fixing this! Please make sure that this is ok to merge/the version number in the changelog is up-to-date.

Nit: please wrap DocumentChange::npos in backticks.

Done.

Optional: I think the changelog usually links to issues, not PRs, so this link could probably be omited.

It looks like there is a mixture of issues and PRs linked to in previous change log entries. I see your point but I personally feel like adding a link to additional context is useful for future readers. If this issue had been discovered externally then there would be an issue number; however, since it was discovered by me, a developer on the Firestore team, no issue was created. IMO, providing this link adds value at no cost. If you feel strongly I can remove it, but I like having it here.

Please make sure that this is ok to merge/the version number in the changelog is up-to-date.

I confirmed the version number at go/firebase-cpp-release.

Done.

It looks like there is a mixture of issues and PRs linked to in previous change log entries. I see your point but I personally feel like adding a link to additional context is useful for future readers. If this issue had been discovered externally then there would be an issue number; however, since it was discovered by me, a developer on the Firestore team, no issue was created. IMO, providing this link adds value at no cost. If you feel strongly I can remove it, but I like having it here.

❌ Integration test FAILED

Requested by @dconeybe on commit de68c2b
Last updated: Mon Jun 21 13:28 PDT 2021
View integration test log & download artifacts

Failures	Configs
remote_config	[TEST] [FAILURE] [iOS] [macos] [ios_target](1 failed tests) TestFetchInterval

Fix "Undefined symbols" linker error when DocumentChange::npos was used.

77d3475

dconeybe added the api: firestore label Jun 16, 2021

dconeybe self-assigned this Jun 16, 2021

google-cla Bot added the cla: yes label Jun 16, 2021

Add change log entry

ee4fa77

dconeybe requested a review from var-const June 16, 2021 16:14

dconeybe assigned var-const and unassigned dconeybe Jun 16, 2021

dconeybe added 2 commits June 16, 2021 14:28

Fix version number in change log entry (8.0.1 -> 8.1.0)

3be0e69

Merge remote-tracking branch 'origin/main' at 3803777

544ae52

dconeybe commented Jun 17, 2021

View reviewed changes

Comment thread firestore/src/common/document_change.cc

var-const previously approved these changes Jun 18, 2021

View reviewed changes

var-const assigned dconeybe and unassigned var-const Jun 18, 2021

Merge 'origin/main' at 287f764

005ae9b

dconeybe commented Jun 21, 2021

View reviewed changes

Add backticks around DocumentChange::npos.

1e1c41d

dconeybe dismissed var-const’s stale review via 1e1c41d June 21, 2021 15:25

dconeybe enabled auto-merge (squash) June 21, 2021 15:25

dconeybe requested a review from var-const June 21, 2021 15:25

dconeybe assigned var-const and unassigned dconeybe Jun 21, 2021

var-const approved these changes Jun 21, 2021

View reviewed changes

dconeybe merged commit de68c2b into main Jun 21, 2021

github-actions Bot added the tests: in-progress This PR's integration tests are in progress. label Jun 21, 2021

github-actions Bot added the tests: failed This PR's integration tests failed. label Jun 21, 2021

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 21, 2021

dconeybe deleted the dconeybe/DefineDocumentChangeNpos branch June 22, 2021 19:44

firebase locked and limited conversation to collaborators Jul 22, 2021

+              ### 8.1.0
+              -   Changes
+                  -   Firestore: Fixed a linker error when DocumentChange::npos was used.
+                      ([#474](https://github.com/firebase/firebase-cpp-sdk/pull/474)).

+              ### 8.1.0
+              -   Changes
+                  -   Firestore: Fixed a linker error when DocumentChange::npos was used.
+                      ([#474](https://github.com/firebase/firebase-cpp-sdk/pull/474)).

Fix "Undefined symbols" linker error when DocumentChange::npos was used by dconeybe · Pull Request #474 · firebase/firebase-cpp-sdk · GitHub

Uh oh!

Fix "Undefined symbols" linker error when DocumentChange::npos was used - #474

Fix "Undefined symbols" linker error when DocumentChange::npos was used#474
dconeybe merged 6 commits into
mainfrom
dconeybe/DefineDocumentChangeNpos

dconeybe commented Jun 16, 2021 •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

var-const left a comment

Uh oh!

var-const Jun 18, 2021

Uh oh!

dconeybe Jun 21, 2021

Uh oh!

var-const Jun 18, 2021

Uh oh!

dconeybe Jun 21, 2021

Uh oh!

Uh oh!

dconeybe left a comment

Uh oh!

dconeybe Jun 21, 2021

Uh oh!

dconeybe Jun 21, 2021

Uh oh!

github-actions Bot commented Jun 21, 2021 •
edited
Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	#if defined(ANDROID)
	// Older NDK (r16b) fails to define this properly. Fix this when support for
	// the older NDK is removed.
	static const std::size_t npos;
	#else
	static constexpr std::size_t npos = static_cast<std::size_t>(-1);
	#endif // defined(ANDROID)

Fix "Undefined symbols" linker error when DocumentChange::npos was used by dconeybe · Pull Request #474 · firebase/firebase-cpp-sdk · GitHub

Uh oh!

Fix "Undefined symbols" linker error when DocumentChange::npos was used - #474

Fix "Undefined symbols" linker error when DocumentChange::npos was used#474dconeybe merged 6 commits intomainfirebase/firebase-cpp-sdk:mainfrom dconeybe/DefineDocumentChangeNposfirebase/firebase-cpp-sdk:dconeybe/DefineDocumentChangeNposCopy head branch name to clipboard

Conversation

dconeybe commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

var-const Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

dconeybe Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

var-const Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

dconeybe Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dconeybe left a comment

Choose a reason for hiding this comment

Uh oh!

dconeybe Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

dconeybe Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix "Undefined symbols" linker error when DocumentChange::npos was used#474
dconeybe merged 6 commits into
mainfrom
dconeybe/DefineDocumentChangeNpos

dconeybe commented Jun 16, 2021 •
edited
Loading

github-actions Bot commented Jun 21, 2021 •
edited
Loading