bbqsrc / cargo-ndk

Compile Rust projects against the Android NDK without hassle
Apache License 2.0
712 stars 64 forks source link

Avoid clang wrapper scripts and pass --target instead #106

Closed rib closed 1 year ago

rib commented 1 year ago

_I'm just sharing this as a draft PR to show another attempt at solving #92 based on the approach currently used in ndk-build, but as you can see from the description below this actually ends up being a lot more complex that it appears on the surface. I tried to even jump the extra hurdles to parse the cargo config files to read configured rustflags but I think this approach would be too fragile to adopt. Notably, for my own use case this solution doesn't work because our project configures rustflags via .cargo/config.toml, including target.<cfg>.rustflags which are the the most problematic to handle here_

Cc: @MarijnS95

--

This is an alternative fix for #92 that avoids hitting command line length limitations and avoids needing to use cargo-ndk as a spring board for running clang, which avoids needing temporary hard links or copying the cargo-ndk binary to the target/ directory.

This takes a similar approach to ndk-build here: https://github.com/rust-mobile/ndk/pull/306 but also tries to be more thorough with reading configured rustflags.

Although this solution initially looked like it would be much simpler it soon became apparent that for it to work reliably it needs to be able to read the user's configured rustflags before adding the required -Clink-arg=--target rustflag (otherwise we would potentially discard rustflags needed by the project).

Unfortunately it turns out to be practically impossible to reliably read configured rustflags from outside cargo!

Rustflags are read by cargo from one of four, mutually-exclusive, places:

  1. CARGO_ENCODED_RUSTFLAGS environment variable.
  2. RUSTFLAGS environment variable.
  3. All matching target..rustflags and target..rustflags config entries joined together.
  4. build.rustflags config value.

(cargo also supports --config command line options that can set these)

Although we can easily handle the first two environment variables, the second two involve:

This experimental patch handles the first two cases and by adding a dependency on the cargo crate will even attempt to handle the second two options, with these caveats:

  1. --config options on the command line aren't handled
  2. Any use of target..rustflags are treated as an error because we have no way of knowing if these are needed but we can't handle them
  3. It's always possible that future versions of Cargo could add new ways to configure rustflags that we might not handle
rib commented 1 year ago

I think it's useful to have this PR for posterity, and to document the challenges that arose from trying to parse cargo's config files but I'll close this for now in favor of https://github.com/bbqsrc/cargo-ndk/pull/108 as a solution instead - can potentially re-open later if it makes sense.