CFLAGS getting ignored
Marekkon5 opened this issue · 17 comments
Hello, because of the recent PR #115, CFLAGS from environment variables get ignored.
My specific use case / possible way to reproduce:
Add fdk-aac-sys = "0.5.0" as dependency. It links to internal Android libraries so it fails to build, however if you run export CFLAGS="-U__ANDROID__" before cargo ndk it will successfully compile. This works fine in 3.2.0, but breaks in 3.2.1.
Thank you.
Besides the fact that those env vars should probably be merged with the parent env, it seems rather suspicious to have to unset __ANDROID__ for an... Android build.
That's because the fdk-aac is actually stolen from Android's source code, so the __ANDROID__ variable indicates internal Android OS build, so it tries to include internal Android logging library, and fails to build. So yeah undefining __ANDROID__ makes it build properly as a normal/userspace Android library.
The cc crate checks a number of different alternatives with the following precedence:
<var>_<target>- for example, CC_x86_64-unknown-linux-gnu<var>_<target_with_underscores>- for example, CC_x86_64_unknown_linux_gnu<build-kind>_<var>- for example, HOST_CC or TARGET_CFLAGS<var>- a plain CC, AR as above.
so yeah a plain CFLAGS would have the lowest precedence here, and also since we don't currently merge you can't set CFLAGS_<target> currently.
Maybe cargo-ndk could aim to set with <var>_<target_with_underscores> and also merge with any existing value for <var>_<target_with_underscores> so then you have the option of completely overriding what cargo-ndk or merging with what cargo ndk passes.
I would generally worry though about needing to unset __ANDROID__ across all uses of the cc crate while cross compiling for Android as a workaround to make a specific crate compile.
unsetting __ANDROID__ sounds like it will easily break things that are intended to be built on android.
Yes, I do agree that this is a hacky workaround which works fine in my case, but might not everywhere, however that was just my case as an example, would be great if the CFLAGS (or with target) got merged since there possibly are other scenarios as well.
On the other hand it's tricky because "merging environment variables" typically has a very undefined/loose meaning.
Using the [env] section in .cargo/config.toml overwrites env vars. Using RUSTFLAGS vs build.rustflags etc also has defined precedence without any merging strategy.
Though for this case, when cargo-ndk only needs to patch one or two extra Android-build-specific flags in there, prepending/appending them seems to be the way to go.
EDIT:
For fdk-aac-sys, can't they/you #undef __ANDROID__ somewhere in a compilation unit(s) or via the cc crate to have it end up in the cmdline string directly? It seems this workaround belongs in their crate, not as a preliminary step before you commence any sort of cargo build.
(That would also break anyone depending on that crate, won't work out of the box with rust-analyzer, etc...)
To fdk-aac-sys - I made a PR, but I don't seem to find the changes in latest release. Anyway in my project it's dependency of dependency of dependency of dependency of dependency of dependency.... So I have to depend on older version for now and this workaround works.
Anyway - my whole point was, ignore the whole __ANDROID__ thing, that was an example of my use case. CFLAGS worked perfectly fine in 3.2.0, in 3.2.1 they stopped working as intended. Don't you think this is more of a breaking change? Shouldn't this be fixed for purposes other than __ANDROID__?
@rib any reason to not just use the CFLAGS and CXXFLAGS directly (i.e. the lowest precedence env vars) and prepend the cargo-ndk hacks to the beginning of it if it is present already?
I think setting the unadorned CFLAGS/CXXFLAGS variables could be bad in situations where your build also uses the cc crate or shells out to another build system that might need to build something for the host machine instead of the target.
We should probably at least stick with one of the target-specific names (especially since we are literally specifying the --target then we wouldn't want that affecting any host machine compilation)
concatenating with an existing set value sounds reasonable though for addressing this issue and if we set the _<target_with_underscores> variables then we also give an extra escape hatch for people to completely override what we set.
It would just mean that @Marekkon5 would need to pass -U__ANDROID__ or other cflags via CFLAGS_<target_with_underscores> but the usecase itself would be catered to I think.
It is just a bit inconvenient in my eyes to set the variable for 4 different targets (arm32, arm64, x86, 64x) and CFLAGS not working "as intended". Perhaps some note in README would be useful.
We could potentially merge in anything from a lower precedence, including CFLAGS/CXXFLAGS.
My initial concern with that is that I was then thinking we'd unset CFLAGS/CXXFLAGS - which would then potentially stop the flags from reaching other external build systems and affecting compiling stuff for the host.
It's probably ok to concatenate and also leave the original CFLAGS/CXXFLAGS as they are. In the case of the cc crate at least the CFLAGS/CXXFLAGS will anyway get ignored due to having a lower precedence so don't need to worry about them being merged again and duplicated.
Combining flags is probably fragile considering quoting issues but still on a best-effort basis it might be reasonable to do.
I would say the least dangerous path is to take CFLAGS and CXXFLAGS if they're defined and concatenate them to the cargo-ndk-provided <var>_<target> flags, and carry on from there.
I'm struggling to see how the prefixing of our clang target to the beginning would introduce quoting issues, given we don't use quotes, and presumably would just concatenate with a single space.
We can take a more advanced approach in the future if it's necessitated, of scanning provided flags to see if someone is providing their own target, or whatever, but for the majority of use cases I don't see that being necessary.
If we want to be clever and safe-ish, we can just hoist the "next most precedent" env var we detect as set to the <var>_<target> flag as well, and just log we've done that.
yep I just mentioned quoting as a general concern with dealing with cflags - I didn't really mean to suggest that we should try and do anything fancy about that ourselves - I was also just assuming we'd concatenate with a space.
I think the only real difference from what you just said was that I was thinking we may as well switch from setting <var>_<target> to setting <var>_<target_with_underscores>. It doesn't make much difference but it does then give users the ability to set <var>_<target> and completely override what cargo ndk passes.
Yeah, I understood your suggestion, but from experience with this tool over the last few years, we should either prefer "violently clobber" or "sit at the absolute bottom of the precedence" pile, due to the principle of least astonishment. In this case we seem to only have the option of violently clobber to actually make things work.
Basically, what is the value of implicitly taking precedence only sometimes? It will surprise a number of people (albeit a subset of a subset of people doing crimes with C flags lol)
In any case, what we do with each flag is going to need to be documented in the README, and to specify how cargo-ndk deals with tooling like Rust's cc crate.
Ah, and of course I hit send without finishing my thoughts, heh.
Insofar as allowing people to completely override the flags we now rely on to pass --target to clang, I feel like that should be a CLI flag/CARGO_NDK_ env var to override the behaviour, because it's not something I particularly want to see bug reports about.
Yeah, it could potentially be surprising for someone setting <var>_<target> not intending to override cargo ndk and then inadvertently losing the --target argument
I propose we just take the logic wholesale from here, and prepend our target to the result if it exists: https://github.com/rust-lang/cc-rs/blob/f074cd9900ab76454d25b52dc9bb9fbd4a0e0e47/src/lib.rs#L3198