Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

@huangqinjin
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Sep 4, 2021
@sergiud
Copy link
Contributor

sergiud commented Sep 4, 2021

Thanks for the contribution. However, many of the changes are more than questionable. Therefore, please provide a description of the purpose of this PR.

@sergiud
Copy link
Contributor

sergiud commented Oct 1, 2021

Is there a chance you could setup a Github action that uses (any) Android NDK? Without a CI pipeline, it is very likely that the Android support will break again.

@huangqinjin
Copy link
Contributor Author

OK, I will make a separate PR for Android CI.

@sergiud
Copy link
Contributor

sergiud commented Oct 1, 2021

Thank you. A separate PR is not needed though. You can add the Github action as part of this PR. This will also ensure your changes actually work.

@huangqinjin
Copy link
Contributor Author

CI added. I disabled tests since running tests requires emulators.

@sergiud
Copy link
Contributor

sergiud commented Oct 7, 2021

Thanks, this looks great now! Some nitpicks:

  • It would be nice to document what is actually logged to logcat (e.g., in README.rst)
  • Possibly add an option that disables Android specific logging? Something like WITH_LOGCAT in CMake

-DANDROID_STL=c++_shared \
-DANDROID_NATIVE_API_LEVEL=24 \
-DANDROID_ABI=${{matrix.abi}} \
-DBUILD_TESTING=OFF
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still build the tests to ensure at least linking works.

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. I have to set ANDROID_NATIVE_API_LEVEL to at least 28. Because glob function used by tests is introduced in 28.

@sergiud
Copy link
Contributor

sergiud commented Oct 8, 2021

Thanks!

@sergiud sergiud added this to the 0.6 milestone Oct 8, 2021
@sergiud sergiud merged commit 3965584 into google:master Oct 8, 2021
@huangqinjin huangqinjin deleted the android-unwind branch October 9, 2021 13:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants