ndk-build: Invoke clang directly instead of through wrapper scripts
#306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarijnS95
force-pushed
the
invoke-clang-directly
branch
from
d2927f7 to
0f61129
Compare
msiglreith
approved these changes
Jul 2, 2022
MarijnS95
force-pushed
the
invoke-clang-directly
branch
2 times, most recently
from
e11fab6 to
cb3a4cb
Compare
The NDK's wrapper scripts for `clang`/`clang++` currently only, and will only ever pass `--target` to the compiler. That is something we can do ourselves, especially now that we're already updating `RUSTFLAGS` anyway. [Upstream even clarified] that it is recommended to pass `--target` yourself instead of incurring extra overhead while going through the wrapper scripts (especially costly on Windows), further guaranteeing that we won't miss out on any flags possibly being added to the wrapper scripts in the future. [Upstream even clarified]: https://r.android.com/2134712
MarijnS95
force-pushed
the
invoke-clang-directly
branch
from
cb3a4cb to
bc6eb22
Compare
This was referenced May 16, 2023
rib
added a commit
to rib/cargo-ndk
that referenced
this pull request
May 18, 2023
This is an alternative fix for bbqsrc#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: rust-mobile/ndk#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.<triple>.rustflags and target.<cfg>.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: - parsing all of Cargo's config files - evaluating cfg options and handling a paradox whereby --cfg arguments specified as rustflags need to then also affect which target.<cfg>rustflags to use! (cargo does this internally with 2 iterations) 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.<cfg>.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
This was referenced May 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The NDK's wrapper scripts for
clang/clang++currently only, and will only ever pass--targetto the compiler. That is something we can do ourselves, especially now that we're already updatingRUSTFLAGSanyway.Upstream even clarified that it is recommended to pass
--targetyourself instead of incurring extra overhead while going through the wrapper scripts (especially costly on Windows), further guaranteeing that we won't miss out on any flags possibly being added to the wrapper scripts in the future.