Workaround for .cmd quoting bug on Windows leads to command line is too long errors

[rib]

With our builds at Embark we're currently blocked from being able to cross compile with cargo ndk on windows due to this recent workaround leading to a new error:

The following warnings were emitted during compilation:                                                                                                                                                        
warning: The command line is too long.
warning: Error: Errored with code 1
warning: The command line is too long.
warning: Error: Errored with code 1
error: failed to run custom build command for `physx-sys v0.11.1`

This is particularly awkward for us because the recent PR to parse --release arguments is currently also required for our build system so we can't revert to an older version of cargo ndk and patch our NDK as a workaround.

I'm wondering if we could:

1. revert Solve hardlink on different disk issue and add support for incremental compiles on windows #101
2. revert Create an abomination #99
3. directly patch the .cmd files

The change needed in the .cmd files to workaround the original quoting bug turns out to be quite simple:

We need to change the if "%1" == "-cc1" to if "%~1" == "-cc1" to avoid double quotes when checking for a "-cc1" argument.

cargo ndk could potentially check the .cmd wrappers for the workaround and warn you about it, or it could apply the workaround directly (maybe with an option).

This would remove the need to springboard clang invocations through cargo ndk and juggling hard links etc.

I can look at cooking a PR for something like this if it sounds reasonable?

edit: please see the discussion below for ideas for a better solution, including PR #108

[MarijnS95]

The proper fix is instead to invoke clang directly and abandon the .cmd scripts as we have done for ndk-build and even requested a Googler to document that clang --target is the preferred method leaving the scripts as a fallback: https://android-review.googlesource.com/c/platform/ndk/+/2134712 (unfortunately I could not share this when #92 got locked).

Would this also hit the command line is too long error? If I remember correctly we once had a cargo-apk contribution which falls back to command files if the string is too long, iirc also by an Embarker 🙂

[rib]

Yeah, it would be ideal if we could reliably override the target that gets passed to clang.

Is it really enough to specify the target via -Clink-arg= though?

What about crates using the cc crate to compile code? ndk-build and cargo-ndk also set up environment variables to point the cc crate at the .cmd wrappers right?

Does the cc crate maybe already specify the target which means the wrapper doesn't end up overriding the target?

unfortunately I could not share this when #92 got locked

I was a little surprised by the issue being locked too, since I had also wanted to try and contribute to fixing the issue. Even now it's a little odd that the issue is locked since we can't join the dots with any follow up discussion.

Considering that you looked at this issue last year, it would have been good to have got your input here!

[rib]

Taking a look at the cc crate it looks like it currently specifically understands that for Android that the toolchain has these wrapper scripts that will handle passing the --target argument and so the cc crate will avoid overriding that:

https://github.com/rust-lang/cc-rs/blob/57df37c161e2c496dd8919362b618f7501b70580/src/lib.rs#L3616

[MarijnS95]

Yeah, it would be ideal if we could reliably override the target that gets passed to clang.

Is it really enough to specify the target via -Clink-arg= though?

Yes it is. As per linked upstream docs that is the only thing you'd fall back to the scripts for. Both cargo-apk and xbuild use this approach.

What about crates using the cc crate to compile code? ndk-build and cargo-ndk also set up environment variables to point the cc crate at the .cmd wrappers right?

Does the cc crate maybe already specify the target which means the wrapper doesn't end up overriding the target?

The cc crate doesn't seem to specify it, we pass it via target-specific C(XX)FLAGS: https://github.com/rust-mobile/ndk/pull/306/files#diff-33520aaf2e3ca2264c50b4159237131b9804b93d42e50b2276a281da69567b8bR28-R33 (see also the commented link to https://github.com/rust-lang/cc-rs#external-configuration-via-environment-variables).

(Curious what it does by default: C(XX)FLAGS completely overrides whatever they have, or they scan if --target is already set, or there's some precedence to command order for clang?)

EDIT: Ah you found where this happens in the comment above.

unfortunately I could not share this when #92 got locked

I was a little surprised by the issue being locked too, since I had also wanted to try and contribute to fixing the issue. Even now it's a little odd that the issue is locked since we can't join the dots with any follow up discussion.

Considering that you looked at this issue last year, it would have been good to have got your input here!

👍

[rib]

I think for now, I'm going to take another pass at fixing this issue based on @MarijnS95's solution in ndk-build

Maybe as a follow up we could reconsider the possibility of depending on ndk-build in cargo-ndk to avoid the duplication of work here.

[rib]

Hmm, so an issue I hit with the above solution is that it's not compatible with rustflags being configured via Cargo. We can quite easily check for existing flags set via RUSTFLAGS or CARGO_ENCODED_RUSTFLAGS but I'm not sure how to check for rustflags set in Cargo config files.

We currently have a number of rustflags being configured via .cargo/config.toml and we'd need to combine the -Clink-arg= with those.

I'm not sure how a tool like cargo ndk can practically access the cargo config state? it doesn't look like it's accessible via cargo metadata

[MarijnS95]

We read the config in xbuild and cargo-apk (for e.g. [env]) and could also "trivially" expand that to buildflags. One of your colleagues asked about it in rust-mobile/cargo-apk#22.

Maybe as a follow up we could reconsider the possibility of depending on ndk-build in cargo-ndk to avoid the duplication of work here.

xbuild doesn't use ndk-build and I have been porting features both ways, but we could definitely revive it and make sure all tools use ndk-build (#28).

[rib]

urgh, reading the cargo config files sounds absolutely horrible though :(

so does xbuild/ndk-build re-implement the logic for merging .cargo/ config file state that can be specified in any directory leading up to the user's home directory?

[MarijnS95]

urgh, reading the cargo config files sounds absolutely horrible though :(

Yes, I really wish more of this could be offloaded or built into or extending cargo...

so does xbuild/ndk-build re-implement the logic for merging .cargo/ config file state that can be specified in any directory leading up to the user's home directory?

Not yet :(

[rib]

Considering that trying to pass --target to clang without the wrappers ends up opening a really big can of worms once we realize we need to start parsing all of Cargo's config files I think for now that the approach of patching the NDK would be my preferred solution in the short term, as in: #105

[rib]

I actually thought I'd make a last ditch attempt at handling parsing the cargo configs properly using the cargo crate, and yeah this is basically impossible to do outside of cargo proper because the RUSTFLAGS turn out to be paradoxical in that they can affect --cfg options which then affects which target.<cfg>.rustflags should be read! 🤯

Here's are some comments from cargo/core/compiler/build_context/target_info.rs

First you have this warning:

/// Search `Tricky` to learn why querying `rustc` several times is needed.

and then there is this:

            // Tricky: `RUSTFLAGS` defines the set of active `cfg` flags, active
            // `cfg` flags define which `.cargo/config` sections apply, and they
            // in turn can affect `RUSTFLAGS`! This is a bona fide mutual
            // dependency, and it can even diverge (see `cfg_paradox` test).
            //
            // So what we do here is running at most *two* iterations of
            // fixed-point iteration, which should be enough to cover
            // practically useful cases, and warn if that's not enough for
            // convergence.
            let reached_fixed_point = new_flags == rustflags;
            if !reached_fixed_point && turn == 0 {
                turn += 1;
                rustflags = new_flags;
                continue;
            }
            if !reached_fixed_point {
                config.shell().warn("non-trivial mutual dependency between target-specific configuration and RUSTFLAGS")?;
            }

[leleliu008]

why not write your own wrapper in Windows C API?

[rib]

why not write your own wrapper in Windows C API?

That's effectively what the current workaround in cargo ndk tries to do (it aims to use itself as a linker wrapper) but with the way its implement it also doesn't work reliably currently.

The current workaround involves creating awkward hard links or copies of cargo-ndk and with the way arguments are now parsed it also leads to errors when a lot of arguments are passed to the linker, as noted above.

[rib]

Just for reference here, I've posted a draft PR that at least shows how I attempted to implement the approach suggested by @MarijnS95: #106

I even went as far as parsing the cargo configs but the target.<cfg>.rustflags options are just ridiculous to try and handle outside of Cargo.

[leleliu008]

@rib sorry, maybe I lost something. I'm not familiar with Windows C API and don't known the limitations of Windows, I just used my own wrapper written in POSIX C on GNU/LInux and macOS and didn't see such error.

[rib]

@rib sorry, maybe I lost something. I'm not familiar with Windows C API and don't known the limitations of Windows, I just used my own wrapper written in POSIX C on GNU/LInux and macOS and didn't see such error.

No worries. It it seems like it should be possible to have a native wrapper but windows also seems to have quite a few limits on command line length that can be more annoying to deal with compared to other OSs.

At least the current attempt to create a native wrapper has inadvertently also created a command line length limit that's more severe than the limit without the native wrapper.

[leleliu008]

I known gnu linker support @file

@file
           Read command-line options from file.  The options read are inserted in place of the original @file option.  If file does not exist, or
           cannot be read, then the option will be treated literally, and not removed.

maybe this is usefull on Windows.

[rib]

Thanks, yeah, it's also possible to pass arguments via @file on windows and could potentially be used, but I guess it shouldn't be needed considering that it does work to just fix the quoting bug in the .cmd wrapper and so the limit in cargo-ndk can't be a fundamental limit on windows.

[rib]

What I'm trying now, as another experiment, is similar to using cargo-ndk as a linker wrapper but making it a RUSTC_WRAPPER instead. Since Cargo lets you configure a rustc wrapper via the RUSTC_WRAPPER environment variable it seems like this could avoid the need for the hard links / file copies and hopefully we can also avoid imposing any extra limit on the command line length here.

[rib]

It's hopefully possible to unravel from the trail of opened / closed PRs but just for reference here; I implemented the idea above in #107 and then it occurred to me that it would be very similar (but a bit simpler) to just set cargo-ndk as the linker instead of as a rustc wrapper since it's more focused on only needing to pass an extra argument to clang as the linker. (This is also similar to what @leleliu008 was discussing)

So the last PR I created (#108) is a kind of combination of ideas from the ndk-build approach of trying to pass --target=<triple><api-level> to clang without needing the .cmd wrappers, but done in a way that avoids needing to touch rustflags.

A nice benefit here is that we only need to act as a wrapper for one purpose (linking via clang) and can point CARGO_<target>_LINKER at cargo-ndk without needing any filesystem hard links, and no need to use the name of the executable to differentiate what is being wrapped.