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

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

Merged
merged 6 commits into from
Jun 21, 2021
Merged

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

merged 6 commits into from
Jun 21, 2021

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jun 16, 2021 •
edited

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

@dconeybe dconeybe self-assigned this Jun 16, 2021
@google-cla google-cla bot added the cla: yes label Jun 16, 2021
@dconeybe dconeybe requested a review from var-const June 16, 2021 16:14
@dconeybe dconeybe assigned var-const and unassigned dconeybe Jun 16, 2021
var-const
var-const previously approved these changes Jun 18, 2021
View reviewed changes
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 for fixing this! Please make sure that this is ok to merge/the version number in the changelog is up-to-date.

release_build_files/readme.md Outdated
@@ -567,6 +567,11 @@ code.

## Release Notes

### 8.1.0
- Changes
- Firestore: Fixed a linker error when DocumentChange::npos was used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please wrap DocumentChange::npos in backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

firestore/src/common/document_change.cc Show resolved Hide resolved
@var-const var-const assigned dconeybe and unassigned var-const Jun 18, 2021
dconeybe
dconeybe commented Jun 21, 2021
View reviewed changes
Copy link
Contributor Author

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

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.

release_build_files/readme.md Outdated
@@ -567,6 +567,11 @@ code.

## Release Notes

### 8.1.0
- Changes
- Firestore: Fixed a linker error when DocumentChange::npos was used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

release_build_files/readme.md
### 8.1.0
- Changes
- Firestore: Fixed a linker error when DocumentChange::npos was used.
([#474](https://github.com/firebase/firebase-cpp-sdk/pull/474)).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@dconeybe dconeybe enabled auto-merge (squash) June 21, 2021 15:25
@dconeybe dconeybe requested a review from var-const June 21, 2021 15:25
@dconeybe dconeybe assigned var-const and unassigned dconeybe Jun 21, 2021
@dconeybe dconeybe merged commit de68c2b into main Jun 21, 2021
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Jun 21, 2021
@github-actions
Copy link

github-actions bot commented Jun 21, 2021 •
edited

❌  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

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jun 21, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 21, 2021
@dconeybe dconeybe deleted the dconeybe/DefineDocumentChangeNpos branch June 22, 2021 19:44
@firebase firebase locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@var-const var-const var-const approved these changes

Assignees

@var-const var-const

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@dconeybe @var-const