cargo-apk: Reimplement "default" (--) subcommand trailing args for cargo

[MarijnS95]

clap does not currently support parsing unknown arguments into a side Vec, which is exactly what cargo apk -- needs to parse a few known cargo arguments (such as --target and -p) but forward the rest verbatim to the underlying cargo subcommand.

allow_hyphen_values = true could partially help us out with this, but it parses all remaining arguments into that Vec upon encountering the first unknown flag/arg, resulting in all known flags after that to also be treated as "unknown" instead of filling up our args: Args struct.

Since a workaround for this isn't currently functioning, introduce pure trailing args with an additional -- separator to make it clear which arguments go to cargo-apk (and are almost all, except --device, forwarded to cargo) and which are only passed to cargo <subcommand>.

---

CC @epage for seeing if we can do something about this :)
CC @kchibisov as I'd like to release cargo-apk with this soon-ish, and will make sure to add the -- separator to winit beforehand for testing.

[MarijnS95]

Fwiw the main reason I dislike this approach is because users may (accidentally) pass cargo-apk-supported arguments after the -- so that they are still passed to the underlying cargo invocation but not to cargo-apk itself, leading to either unexpected or outright broken behaviour :(

(For example cargo apk -- doc --device "xyz" -- --no-deps -p my-crate would confuse cargo-apk, as the -p has to sit before the second --; and at the same time --device isn't actually passed to cargo doc 😅)

[MarijnS95]

Realizing the original issue in winit 1 2 won't easily be solvable with this as it currently uses a generic env var to invoke cargo $CMD doc for every target, which we'd have to extend with a custom if: target == android where the arguments supported by cargo-apk are separated out from rustdoc specific args with a --.

[MarijnS95]

Another thought is that we "just migrate winit CI to xbuild", @dvc94ch is that something you can help with time-wise?

Then I'll release cargo-apk with this workaround for now while we look for a more proper solution (or not at all, pending #260 deprecation).

[dvc94ch]

Another thought is that we "just migrate winit CI to xbuild", @dvc94ch is that something you can help with time-wise?

The way this question is framed I assume they do something more complicated than just cargo apk build? I can take a look this weekend.

[MarijnS95]

@dvc94ch they do, by (ab)using cargo apk -- to run generic doc/test commands, but nothing else (no build , and clippy is ran outside of cargo apk).

Specifically, since our migration to clap we limit what arguments can be passed to cargo through apk -- and their passing of --no-deps and --document-private-items breaks it currently, hence this PR.

But then I think we may want to skip that doc step entirely?

[MarijnS95]

It seems that cargo doc doesn't need to go through cargo apk -- doc at all, so that's one case solved: https://github.com/MarijnS95/winit/compare/ci-android

Now only cargo test with more unrecognized args remains....

[dvc94ch]

Looks like it's not something xbuild intended to do. To check if the tests compile wouldn't cargo ndk do the job? I could add a cargo command x cargo that would work similar to the cargo ndk tool.

[MarijnS95]

@dvc94ch it might be good for xbuild to support this so that we - again - have one shared codebase that takes care of NDK detection and env-var setup. (Un...)fortunately xbuild uses clap too so it'll suffer from the same parsing "tangle". I think the current PR is "the best we can do" as any alternative is tricky.

I don't want to defer to cargo-ndk as it is known to significantly lag behind NDK compatibility, and has released broken updates after merging in dubious external contributions (at risk of sounding like a broken record: that wouldn't have been a problem if it Just Used ndk-build™).

[dvc94ch]

Sorry didn't have time this weekend. Had some family stuff on Friday and Saturday and then had to some work today to compensate for Friday. I can look into it towards the end of next week

[MarijnS95]

@dvc94ch No worries, we all have exactly that (plus lots of opensource / hobby projects to care about).

In any case I applied this change to winit to have more visibility into how the API is going to end up breaking.

[msiglreith]

looks fine to me, also the winit CI changes!

[MarijnS95]

@msiglreith perfect, thanks! I'll start the release train tomorrow morning 🚂!

[madsmtm]

Haven't researched how you actually use clap, just wanted to quickly note how (I think) cargo-dinghy is doing it, for inspiration:

cargo dinghy --device=... --any-other-dinghy-commands-here run --release --other-normal-cargo-commands-here

(Note that dinghy flags must come before the invoked subcommand (run, build, ...), while any cargo flag always must come after).

[MarijnS95]

@madsmtm looks like dinghy uses quite a complex setup to filter out known args from unknown args?

https://github.com/sonos/dinghy/blob/87463fec2df24bc01c98817b26f8b04c4ec007fa/cargo-dinghy/src/cli.rs#L108-L154

This is what I wanted to avoid and/or have implemented in some way in clap, though maybe @epage is interested in how the ecosystem is tackling this.

(Note that dinghy flags must come before the invoked subcommand (run, build, ...), while any cargo flag always must come after).

Fortunately we do not currently support any arguments before the subcommand, though at some point #[clap(global = true)] arguments may be nice (for e.g. --device).

---

In other words, @madsmtm would you like to see us implementing something along these lines instead of committing to -- separator "ugliness" (see also additional user-unfriendly complexity rust-windowing/winit#2561)? cargo-apk may be on a deprecation path but xbuild uses clap too and I think it'll all transfer over neatly.

[madsmtm]

In other words, @madsmtm would you like to see us implementing something along these lines instead of committing to -- separator "ugliness"

I don't have much of an opinion on this, honestly didn't know dinghy's approach was that complicated.

Would be nice though to my eyes if every cargo extension did it in roughly the same way, whichever way that may end up being.

[MarijnS95]

@madsmtm definitely!

(Note that dinghy flags must come before the invoked subcommand (run, build, ...), while any cargo flag always must come after).

Getting back to this, with a slight modification this logic actually/easily starts working with mixed arguments, which is what I originally wanted to have 🎉
(otherwise, why not use trailing_var_arg = true, allow_hyphen_values = true in cargo-dinghy which kicks in as soon as clap sees the first unknown argument...).

I'll push this after lunch and see if it works without the proposed invasive change in winit, thanks so much for pointing this out to me @madsmtm! If you need I can port it back to dingy, made some improvements along the way too :)

[MarijnS95]

And it actually works out for winit without separating out any of the original arguments https://github.com/rust-windowing/winit/actions/runs/3523743875 (disregard MSRV failures)!

Here's a short run-down of what I did and found:

First up the code from dinghy inspired me on this possible implementation, but dingy seems to have some unnecessary complexity that I do not yet understand. As mentioned above by @madsmtm and from this bit of code I gather it's collecting args for cargo as soon as encountering the first thing it doesn't recognize, that isn't a positional argument. Furthermore, initially it considers any arg that starts with - to be a valid, known option even it isn't, leading this to feel like an incomplete trailing_var_arg = true, allow_hyphen_values = true implementation...

Then, for what the implementation does here:

• Assuming (and validated by assert!) that our Args doesn't have positionals, it's pretty trivial to introspect clap for what short/long flags are accepted by Args, and filter them out with some basic logic;
  • only complication I can think of is when a value-taking argument in the cargo subcommand has a value that it also recognized as a cargo-apk arg and consumed out of the list, e.g. rustc --link-arg --quiet ..., but those should be rare and I don't know if = is required instead of a space, nor how clap deals with that;
• Added a bunch of tests that mix and match value-taking args and flags, including unknown ones;
• By keeping args: Args in the subcommand, all these fields are documented in cargo apk -- -h. Clap keeps moving all mixed args starting from the first unrecognized arg into cargo_args thanks to trailing_var_arg = true, allow_hyphen_values = true, which my logic later separates out and appends to existing args inside args: Args via update_from_arg_matches();
  • I did not get to add a test-case for this as it'd end up quite verbose with having to parse a full Cmd and match on the nested subcommands.

[msiglreith]

logic looks sound, nothing to add from me beside some nitpick (:

[dvc94ch]

@MarijnS95 sorry the current winit ci won't work with xbuild. that's not what xbuild is for. If you want to build a gui application or game then you can build/deploy it with xbuild. To test that something compiles for a specific target while respecting all the cargo flags is a different story. I doubt this is acceptable to you, but you could just set RUSTFLAGS="-C linker=echo" and test that it compiles. If you want to detect an ndk and set all the flags and support c compilation etc etc there are two options.

1. you need a complex tool with lots of edge cases that often behaves unexpectedly
2. you build your own cli interface with well defined semantics that are not dependent on cargo an then you need to give up some features that are supported by cargo

after picking 1. when building cargo-apk I decided to go with 2. for xbuild. the advantage of 2. is that I'm confident that whatever the combination of cli arguments you provide to xbuild it actually does what it's supposed to.

[dvc94ch]

the other alternative is to just hack it for the winit build, probably also unacceptable:
-C linker=clang -C link-arg=-fuse-ld=lld -L/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/aarch64-linux-android/33 -C link-arg=-B/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/aarch64-linux-android/33 -C link-arg=--target=aarch64-linux-android

[MarijnS95]

@dvc94ch That is fine then, we can look for alternatives by e.g. setting up winit's CI build in such a way that it either builds an APK target or not rely on the NDK compiler/linker.

However, this also means I'm not yet ready to deprecate cargo-apk straight away. I also don't think cargo-apk "often behaves unexpectedly" (I leave that to cargo-ndk thank you), nor is there currently any viable alternative.

Regardless, the fixes have been merged and released, so we can build on this for some time. I'll just have to backport some features to xbuild now to make it work for my case again (workspace inheritance 😰).

[dvc94ch]

one thing we might be able to do is get "-C linker=clang -C link-arg=-fuse-ld=lld -C link-arg=--target=aarch64-linux-android" into the rust target definition. then the only thing that would need to be supplied for winit would be the -L and -B path.

sorry wasn't trying to diss any tool. but I do think that cargo-apk tries to mimick the behaviour of cargo in it's various flags and we have had a few of bugs where we either oversimplified cargos behaviour or didn't quite get it right. mimicking a complex tool like cargo is always going to be a hard task and there will be edge cases that won't work like what's being emulated.

[dvc94ch]

just fyi "often behaves unexpectedly" this looks like a quote and I never said those words :)

[MarijnS95]

You are right that cargo-apk and anything else is still in catch-up phase to support everything cargo does; not only on the cmdline but also while parsing Cargo.toml manifests, .cargo/config.toml configuration, environment variables and the likes. There's no way to fix this proper other than explicitly not supporting things, or making such tools first-class cargo extensions with a (very tricky to design) carefully crafted API where cargo and the extension work together to achieve whatever is needed. Effectively we and many other projects need a way to run "post build steps" in a super fancy way while also influencing the build system directly.

just fyi "often behaves unexpectedly" this looks like a quote and I never said those words :)

#363 (comment)

1. you need a complex tool with lots of edge cases that often behaves unexpectedly

...
after picking 1. when building cargo-apk

Maybe you were not directly calling out cargo-apk, but it definitely seems like it, and that's okay. You are correct that it may not handle every case well, but it's getting better.

[dvc94ch]

Take it back, I did say those words hahha

[dvc94ch]

But I think the main point I was trying to make is valid. xbuild is not a cargo replacement and the fact that it uses cargo is an internal implementation detail as far as the user is concerned. Similar like cargo doesn't allow passing arbitrary rustc flags. Yes I know there's RUSTFLAGS and cargo rustc so you can usually somehow manage to do what you were trying to do.