Skip to content
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

nkd-build: Propagate RUSTFLAGS into CARGO_ENCODED_RUSTFLAGS #357

Merged
merged 10 commits into from
Oct 28, 2022

Conversation

Jake-Shadle
Copy link
Contributor

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
Copy link
Member

MarijnS95 commented Oct 17, 2022

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?

ndk-build/CHANGELOG.md Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 changed the title Support RUSTFLAGS in addition to CARGO_ENCODED_RUSTFLAGS nkd-build: Propagate RUSTFLAGS into CARGO_ENCODED_RUSTFLAGS Oct 25, 2022
ndk-build/src/cargo.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member

MarijnS95 commented Oct 25, 2022

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:

  • What do we do if both RUSTFLAGS and CARGO_ENCODED_RUSTFLAGS are set? Error? Merge them?
  • 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?

@Jake-Shadle
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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.

ndk-build/CHANGELOG.md Outdated Show resolved Hide resolved
ndk-build/src/cargo.rs Outdated Show resolved Hide resolved
ndk-build/src/cargo.rs Outdated Show resolved Hide resolved
MarijnS95 referenced this pull request in bbqsrc/cargo-ndk Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo APK 0.9.3 and later does not appear to respect rustflags in config.toml
3 participants