-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
android: Set SDK/API level via version-suffxed --target
triple
#209
Open
MarijnS95
wants to merge
1
commit into
master
Choose a base branch
from
android-set-sdk-level
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note that this change might cause an error like the following, if you use the
This will also affect The change, issue and solution are: |
We haven't set the SDK/API level via the `__ANDROID_API__` define for a very long time and so far got away with it. However, while debugging why `backtrace` (and by extension Rust `std` which reuses that crate) wasn't generating symbolicated stacktraces in `panic_log`, and why `findshlibs` wasn't providing the list of loaded libraries to `sentry`, we found that both rely on expanding the `__ANDROID_API__` define via compiling a small C file via `cc` to make the code conditional on SDK/API >= 21: gimli-rs/findshlibs#65 rust-lang/backtrace-rs#415 (It would have been lovely if these crates emitted a `cargo:warning` when the define wasn't set at all, indicating an "incomplete" cross-compiler setup, and/or looked at the runtime Android API version via something like rust-mobile/ndk#479.) Note that `backtrace 0.3.74` / Rust 1.82 (rust-lang/rust@0763a3a) no longer rely on this because Rust 1.82 bumped the minimum SDK/API level to 21: rust-lang/backtrace-rs#656 We could set this define directly, or rely on `clang` to set it for us by appending the SDK/API level to the target triple, of the form `<arch>-linux-android<sdk level>`. The latter is more common. Keep in mind that the `cc` crate adds an unversioned `--target=<arch>-linux-android` to the command line arguments as well, but clang seems to deduplicate them (or look at the latter `--target` which contains our version). Note that this effectively reverts 32efed6 because we must now always pass the SDK level via the triple again, even if the host also happens to be Android with the same architecture.
MarijnS95
force-pushed
the
android-set-sdk-level
branch
from
January 9, 2025 13:11
c4d4f6e
to
42648ec
Compare
MarijnS95
added a commit
to MarijnS95/findshlibs
that referenced
this pull request
Jan 9, 2025
Since Rust 1.82 the minimum Android API level is 21. In `backtrace`, this warranted the removal of the API version check via `__ANDROID_API__` in rust-lang/backtrace-rs#656 since it is now always known to be >=21. When `findshlibs` bumps its MSRV the same could be done. This change is sparked by a long search for why backtraces always had `<unknown>` symbols on Android - and when solving that by upgrading Rust (for `std::backtrace`) or our `backtrace` dependency, why Sentry reports did not record what images/libraries were loaded at which offsets to resolve stack addresses back to function symbols. It turned out that the `xbuild` build tool never set `__ANDROID_API__` (by not telling `clang` about the target API level via the triple) which would cause this code to never set `feature = "dl_iterate_phdr"` to search for libraries on Android: rust-mobile/xbuild#209
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We haven't set the SDK/API level via the
__ANDROID_API__
define for a very long time and so far got away with it. However, while debugging whybacktrace
(and by extension Ruststd
which reuses that crate) wasn't generating symbolicated stacktraces inpanic_log
, and whyfindshlibs
wasn't providing the list of loaded libraries tosentry
, we found that both rely on expanding the__ANDROID_API__
define via compiling a small C file viacc
to make the code conditional on SDK/API >= 21:gimli-rs/findshlibs#65
rust-lang/backtrace-rs#415
(It would have been lovely if these crates emitted a
cargo:warning
when the define wasn't set at all, indicating an "incomplete" cross-compiler setup, and/or looked at the runtime Android API version via something like rust-mobile/ndk#479.)Note that
backtrace 0.3.74
/ Rust 1.82 (rust-lang/rust@0763a3a) no longer rely on this because Rust 1.82 bumped the minimum SDK/API level to 21: rust-lang/backtrace-rs#656We could set this define directly, or rely on
clang
to set it for us by appending the SDK/API level to the target triple, of the form<arch>-linux-android<sdk level>
. The latter is more common. Keep in mind that thecc
crate adds an unversioned--target=<arch>-linux-android
to the command line arguments as well, but clang seems to deduplicate them (or look at the latter--target
which contains our version).Note that this effectively reverts 32efed6 because we must now always pass the SDK level via the triple again, even if the host also happens to be Android with the same architecture.