-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Provide libgcc linker script workaround for NDK >= 23 #67
Conversation
You might want to look at how we implemented and evolved this workaround in |
er, okey? I mean fwiw I did skim it before I opened this PR and it vaguely looked similar - not sure what the frowny face is about here - I guess I've stepped on a nerve? maybe you could say what's wrong with this? |
For reference this is the code in // Workaround for https://github.com/rust-windowing/android-ndk-rs/issues/149:
// Rust (1.56 as of writing) still requires libgcc during linking, but this does
// not ship with the NDK anymore since NDK r23 beta 3.
// See https://github.com/rust-lang/rust/pull/85806 for a discussion on why libgcc
// is still required even after replacing it with libunwind in the source.
// XXX: Add an upper-bound on the Rust version whenever this is not necessary anymore.
if ndk.build_tag() > 7272597 {
let cargo_apk_link_dir = target_dir
.as_ref()
.join("cargo-apk-temp-extra-link-libraries");
std::fs::create_dir_all(&cargo_apk_link_dir)
.map_err(|e| NdkError::IoPathError(cargo_apk_link_dir.clone(), e))?;
let libgcc = cargo_apk_link_dir.join("libgcc.a");
std::fs::write(&libgcc, "INPUT(-lunwind)").map_err(|e| NdkError::IoPathError(libgcc, e))?;
// cdylibs in transitive dependencies still get built and also need this
// workaround linker flag, yet arguments passed to `cargo rustc` are only
// forwarded to the final compiler invocation rendering our workaround ineffective.
// The cargo page documenting this discrepancy (https://doc.rust-lang.org/cargo/commands/cargo-rustc.html)
// suggests to resort to RUSTFLAGS.
// Note that `rustflags` will never be empty because of an unconditional `.push_str` above,
// so we can safely start with appending \x1f here.
rustflags.push_str("\x1f-L\x1f");
rustflags.push_str(
cargo_apk_link_dir
.to_str()
.expect("Target dir must be valid UTF-8"),
);
}
cargo.env("CARGO_ENCODED_RUSTFLAGS", rustflags); Although I wrote the patch before going to check this, I did actually already check this out of curiosity and it looks pretty similar to what my PR does? There are more comments here about It would probably be good to use |
libgcc was removed from NDK v23 which breaks Rust builds against pre-built standard libraries that were linked against libgcc instead of libunwind. As a workaround this ensures that there is a libgcc.a in the library search paths which is a linker script that will redirect to link with libunwind instead. The libgcc.a linker script is created under: target/cargo-ndk/libgcc-workaround/libgcc.a Fixes: bbqsrc#22
0b4a77f
to
f2637c8
Compare
Okey, the updated PR now:
|
Ok(val) => val, | ||
Err(std::env::VarError::NotPresent) => "".to_string(), | ||
Err(std::env::VarError::NotUnicode(_)) => { | ||
log::error!("RUSTFLAGS environment variable contains non-unicode characters"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/RUSTFLAGS/CARGO_ENCODED_RUSTFLAGS
?
// Note: there is a tiny chance of a false positive here while the first two | ||
// beta releases for NDK v23 didn't yet include libunwind for all | ||
// architectures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're copying things from ndk-build
anyway, why not use the same r23 beta3
tag: https://github.com/rust-windowing/android-ndk-rs/blob/814be0864bd05f6fff616299ce25c075487fa244/ndk-build/src/cargo.rs#L54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I considered it but it didn't look like cargo-ndk
parsed the build tag in the same way.
Since it doesn't look like you can download the first two betas anymore and also since by their very nature beta releases shouldn't be relied on it seems reasonable in this case to just check the major number.
The official term is the
Looks like you found some yourself, though I wouldn't have bothered for the Regardless my point still stands; this crate should use (or merge with) |
libgcc was removed from NDK v23 which breaks Rust builds against pre-built standard libraries that were linked against libgcc instead of libunwind.
As a workaround this ensures that there is a libgcc.a in the library search paths which is a linker script that will redirect to link with libunwind instead.
The libgcc.a linker script is created under:
target/cargo-ndk/libgcc_workaround/libgcc.a
Fixes: #22
For reference the underlying issue has been fixed upstream but it still depends on building the std library from source (via
-Zbuild-std
) instead of using the pre-built libraries distributed via rustup. This currently means using a nightly toolchain. See: rust-lang/rust#85806