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

Use cargo-ndk as a _LINKER wrapper to pass --target to Clang #108

Merged
merged 1 commit into from
May 22, 2023

Conversation

rib
Copy link
Contributor

@rib rib commented May 18, 2023

Okey, yet another iteration on figuring out how to handle this. This is my last idea for now ;-p

This is an alternative workaround for #92 that's similar to the solution used in ndk-build except that the --target=<triple><api-level> argument for Clang is injected by using cargo-ndk as a linker wrapper instead of trying to modify CARGO_ENCODED_RUSTFLAGS.

It turned out to be practically impossible to be able to reliably set CARGO_ENCODED_RUSTFLAGS without risking trampling over rustflags that might be configured via Cargo (for example target.<cfg>.rustflags are especially difficult to read outside of cargo itself)

This approach avoids hitting the command line length limitations of the current workaround and avoids needing temporary hard links or copying the cargo-ndk binary to the target/ directory.

Even though we've only seen quoting issues with the Windows .cmd wrappers this change to avoid using the wrapper scripts is being applied consistently for all platforms. E.g. considering the upstream recommendation to avoid the wrapper scripts if possible:

https://android-review.googlesource.com/c/platform/ndk/+/2134712

Fixes: #92
Fixes: #104
Addresses: android/ndk#1856

--

With this approach we're again able to build our project at Embark which currently fails to compile due to the command line is to long errors mentioned in #104. This solution is also compatible with the way that we configure rustflags via .cargo/config.toml.

Compared to #105 it seems better to be side stepping the Clang wrappers entirely and then not needing to patch them.

Compared to #107 this ends up being very similar to but simpler than wrapping rustc and is more targeted for this issue.

Cc: @MarijnS95

This is an alternative workaround for bbqsrc#92 that's similar to the solution
used in ndk-build except that the `--target=<triple><api-level>` argument
for Clang is injected by using cargo-ndk as a linker wrapper instead of
trying to modify CARGO_ENCODED_RUSTFLAGS.

It turned out to be practically impossible to be able to reliably
set CARGO_ENCODED_RUSTFLAGS without risking trampling over rustflags
that might be configured via Cargo (for example `target.<cfg>.rustflags`
are especially difficult to read outside of cargo itself)

This approach avoids hitting the command line length limitations of the
current workaround and avoids needing temporary hard links or copying
the cargo-ndk binary to the target/ directory.

Even though we've only seen quoting issues with the Windows `.cmd`
wrappers this change to avoid using the wrapper scripts is being
applied consistently for all platforms. E.g. considering the upstream
recommendation to avoid the wrapper scripts if possible:

https://android-review.googlesource.com/c/platform/ndk/+/2134712

Fixes: bbqsrc#92
Fixes: bbqsrc#104
Addresses: android/ndk#1856
@rib
Copy link
Contributor Author

rib commented May 18, 2023

To make it clear that I think this solution is probably the best so far of the different options I have tried out, I have closed my other PRs that I think are superseded by this one (#105, #106 and #107).

@bbqsrc
Copy link
Owner

bbqsrc commented May 22, 2023

Perfectly hideous. 🙂

@bbqsrc bbqsrc merged commit d6cdbf4 into bbqsrc:main May 22, 2023
@rib
Copy link
Contributor Author

rib commented May 23, 2023

Yay, thanks for landing!

@bbqsrc
Copy link
Owner

bbqsrc commented May 23, 2023

np, I'll play with it a bit today, and if I find no issues (or fix any quickly) I'll push out a new release today.

@bbqsrc
Copy link
Owner

bbqsrc commented May 23, 2023

There'll be a little delay... life has a habit of surprising me with work lol.

@rib
Copy link
Contributor Author

rib commented May 23, 2023

yeah, no worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants