Android NDK 25 [firefox-android: main] by bendk · Pull Request #5442 · mozilla/application-services

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk · 2023-03-24T20:09:20Z

This updates the code to use android NDK 25.

The upgrade caused all kinds of issues, see #5436 for details. This PR contains a hack to work around those issues. It's very ugly, but it works at least. Is there a better way to do this?

Pull Request checklist

Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-03-24T20:21:31Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.04⚠️

Comparison is base (86c84c2) 45.11% compared to head (2ded859) 45.08%.

Additional details and impacted files

@@ Coverage Diff @@## main #5442 +/- ##
==========================================
- Coverage 45.11% 45.08% -0.04% 
==========================================
Files 172 172 Lines 14378 14387 +9 ==========================================
- Hits 6487 6486 -1 - Misses 7891 7901 +10

Impacted Files	Coverage Δ
.../support/rc_crypto/nss/nss_build_common/src/lib.rs	`65.09% <33.33%> (-1.91%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rvandermeulen · 2023-03-24T20:32:09Z

I guess @glandium can't formally review this, but ultimately I think he's the one who should sign off on whatever lands here.

ncalexan

There's nothing here that worries me beyond the obvious grossness of the workaround.

The only suggestion I have is that if it's a) helpful and b) going to be a persistent situation, we could teach the rust-android-gradle plugin to do this symbol dance (or to remove -nodefaultlibs or whatever the flag was) and generalize this slightly.

I'm going to mark this r+ and trust @bendk and @glandium to agree on the details. Ben, land when you're comfortable.

What a tremendous waste of time and effort :(

The new NDK doesn't link to `libgcc` anymore, which breaks our our NSS and SQLCipher libraries since they depended on the symbols from libclang_rt.builtins-x86_64-android` like `__extenddftf2`. See mozilla#5436 for more details. The change works around this by manually linking to the libclang_rt.builtins-x86_64-android library in this case. Added a doc on how to upgrade the Android NDK which hopefully will help us in the future. Extracted some common code from the the `build-*-android.sh` scripts to make these directions simpler.

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: https://github.com/nerdcash/Nerdbank.Cryptocurrencies/pull/262/files#diff-7cc5f1ef7cbfce3114fe631861f19de2c050c13ff71e987100669131bb9ffa25Fixesrusqlite#1380

Adds a temporary workaround for [an issue] with the Rust compiler and Android when compiling for x86_64 devices. The Android NDK used to include `libgcc` for unwind support (which is required by Rust among others). From NDK r23, `libgcc` is removed, replaced by LLVM's `libunwind`. However, `libgcc` was ambiently providing other compiler builtins, one of which we require: `__extenddftf2` for software floating-point emulation. This is used by SQLite (via the `rusqlite` crate), which defines a `LONGDOUBLE_TYPE` type as `long double`. Rust uses a `compiler-builtins` crate that does not provide `__extenddftf2` because [it involves floating-point types that are not supported by Rust][unsupported]. For some reason, they _do_ export this symbol for `aarch64-linux-android`, but they do not for `x86_64-linux-android`. Thus we run into a problem when trying to compile and run the SDK on an x86_64 emulator. The workaround comes from [this Mozilla PR]: we tell Cargo to statically link the builtins from the Clang runtime provided inside the NDK, to provide this symbol. [an issue]: rust-lang/rust#109717 [this Mozilla PR]:mozilla/application-services#5442 [unsupported]: https://github.com/rust-lang/compiler-builtins#unimplemented-functions This fix was copied from: zcash/zcash-android-wallet-sdk@1bf2f84Fixesrusqlite#1380 Co-authored-by: Jack Grigg <jack@electriccoin.co>

bendk requested review from ncalexan and rvandermeulen March 24, 2023 20:11

bendk mentioned this pull request Mar 24, 2023
Update Android NDK to 25 #5436
Closed

rvandermeulen removed their request for review March 24, 2023 20:31

ncalexan approved these changes Mar 24, 2023
View reviewed changes

bendk force-pushed the ndk25 branch 8 times, most recently from 68edfed to 6a0f8ebCompare March 29, 2023 23:29

rvandermeulenand others added 3 commits March 30, 2023 09:55

Upgrade NDK to r25b.
6fa406f

Updating Android NDK to 25c
eac793d

bendk force-pushed the ndk25 branch from 6a0f8eb to 2c97bebCompare March 30, 2023 13:55

bendk merged commit 2c97beb into mozilla:mainMar 30, 2023

thunderbiscuit mentioned this pull request Mar 30, 2023
Streamline the NDK version used in Android build and CI bitcoindevkit/bdk-ffi#242
Closed

tarikeshaq mentioned this pull request Apr 20, 2023
Can't build NSS for Android on Mac post NDK upgrade to 25 #5491
Closed

julian-CStack mentioned this pull request Sep 14, 2023
Building for android on Linux using ndk r23 and newer, builtin symbols cannot be found irondash/cargokit#24
Closed

Niels-Be mentioned this pull request Nov 4, 2024
Fix build bundle for android x86_64 (#1380) rusqlite/rusqlite#1592
Open