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 RUSTC_WRAPPER to pass --target to Clang #107

Closed
wants to merge 1 commit into from

Conversation

rib
Copy link
Contributor

@rib rib commented May 18, 2023

Okey, this is a another approach, inspired by what ndk-build does but avoiding touching CARGO_ENCODED_RUSTFLAGS, due to the huge can of worms that opens

This is an alternative workaround for #92 that's similar to the solution used in ndk-build except that the -Clink-arg= rustflag is injected via a RUSTC_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.

Cc: @MarijnS95

This is an alternative workaround for bbqsrc#92 that's similar to the solution
used in ndk-build except that the `-Clink-arg rustflag` is injected via
a RUSTC_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

Closing this for now in favor of #108 as a solution instead - can potentially re-open later if it makes sense.

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