rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.14k stars 112 forks source link

nkd-build: Propagate `RUSTFLAGS` into `CARGO_ENCODED_RUSTFLAGS` #357

Closed Jake-Shadle closed 2 years ago

Jake-Shadle commented 2 years ago

When running the actual cargo build, the CARGO_ENCODED_RUSTFLAGS was being read so that the rustflags set by the user are not overwritten when cargo-apk is adding its flags. However, this environment variable will only be set if you are building the APK via a build script, if you are invoking cargo apk build cargo will not create that environment variable since it doesn't know what actual target/profile is going to be built. If CARGO_ENCODED_RUSTFLAGS is not found, this change now falls back to RUSTFLAGS, and interprets it the same as cargo does before writing it in the encoded form expected by CARGO_ENCODED_RUSTFLAGS

MarijnS95 commented 2 years ago

I think this PR simply refers to the documented behaviour of CARGO_ENCODED_RUSTFLAGS being mutually exclusive wrt RUSTFLAGS? https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags If so, please mention that explicitly in the commit message and code implementation.

CARGO_ENCODED_RUSTFLAGS (just RUSTFLAGS, previously) wasn't really read because of cargo possibly setting it, but to update the user environment rather than blindly overwriting it like other tools.

Do you want to document this feature in the changelog?

MarijnS95 commented 2 years ago

Suggestion for the commit/PR message:

ndk-build uses `CARGO_ENCODED_RUSTFLAGS` to get around flags/arguments
with spaces in `RUSTFLAGS`, with no defined way to escape them.  For
correctness this implementation reads `CARGO_ENCODED_RUSTFLAGS` instead
of blindly overwriting its contents, but as per [`build.rustflags`
precedence rules] setting this environment variable would disregard any
contents in the more common `RUSTFLAGS` environment variable. To make
this more convenient for users, propagate the contents of `RUSTFLAGS`
into `CARGO_ENCODED_RUSTFLAGS` to ensure its contents don't get ignored.

[`build.rustflags` precedence rules]: https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags

Couple followup questions:

Jake-Shadle commented 2 years ago

What do we do if both RUSTFLAGS and CARGO_ENCODED_RUSTFLAGS are set? Error? Merge them?

I think the only case that would be relevant would be if there was another non-cargo tool that invoked cargo-apk and set CARGO_ENCODED_RUSTFLAGS incorrectly based on the RUSTFLAGS/profile/etc which would then cause cargo-apk to propagate the incorrect ones, but that seems like a bit of a stretch.

Do we unset RUSTFLAGS after propagating its contents into CARGO_ENCODED_RUSTFLAGS, pre-empting any duplication issues that might otherwise possibly happen if things change up/downstream?

I've cleared that env var now, since once cargo is invoked the ENCODED ones are the only ones that it cares about barring bugs, and there is already eg https://github.com/dtolnay/rustflags for parsing those if user's need to access them in build scripts.

MarijnS95 commented 2 years ago

I think the only case that would be relevant would be if there was another non-cargo tool that invoked cargo-apk and set CARGO_ENCODED_RUSTFLAGS incorrectly based on the RUSTFLAGS/profile/etc which would then cause cargo-apk to propagate the incorrect ones, but that seems like a bit of a stretch.

I wouldn't mind (prefer, actually) to be on the defensive side, and just disallow the case where RUSTFLAGS happens to be set when we just found CARGO_ENCODED_RUSTFLAGS to be set, too.

Then only @msiglreith's comment remains :)

Jake-Shadle commented 2 years ago

I mean I could do that suggestion but would mean an unnecessary extra vector allocation since join is a slice method not an iterator method but if you're ok with that.