rust-mobile / cargo-subcommand

Library for implementing cargo subcommands
6 stars 6 forks source link

`--target x86...` is picked up but `--target=x86...` is not #9

Closed Shnatsel closed 2 years ago

Shnatsel commented 2 years ago

Cargo allows passing parameters with arguments both using space and the equality sign as a separator. For example, both of the following invocations are valid:

cargo build --release --target=armv7-unknown-linux-gnueabihf`
cargo build --release --target armv7-unknown-linux-gnueabihf`

cargo-subcommand does pick up the form separated with a space, but not with the = sign.

Steps to reproduce:

cargo subcommand build --target=armv7-unknown-linux-gnueabihf prints target: None

Shnatsel commented 2 years ago

Also, thanks a lot for writing this crate! Right now writing Cargo subcommands requires a ton of boilerplate, and I'm really happy to see some reusable code in this area!

I am using it to prototype cargo auditable, and I'm also considering migrating cargo supply-chain to it.

dvc94ch commented 2 years ago

The arg parsing is really terrible and causes several issues in cargo-apk. Was considering folding cargo-subcommand into cargo-apk for a while now as it's the only user, but recently I've been working on a different project that needs a lot of similar code I borrowed from here. I think it's time to fix it.

Shnatsel commented 2 years ago

Thanks a lot!

Now that I am writing my second subcommand, I am convinced that a library like cargo-subcommand is sorely needed. Although I thought that even after trying to write my first one!

Also, I like the concept of listening for some specific recognized flags and treating the rest as intended for Cargo. In my subcommands I do end up calling either cargo metadata or something normal like cargo build, and this simplifies argument handling a great deal.

Shnatsel commented 2 years ago

Oh! If you're going to make semver-breaking changes, then you might also want to consider switching from String to OsString to represent command-line arguments. This is necessary to correctly handle paths. Quoting the OsString docs:

The need for this type arises from the fact that:

  • On Unix systems, strings are often arbitrary sequences of non-zero bytes, in many cases interpreted as UTF-8.
  • On Windows, strings are often arbitrary sequences of non-zero 16-bit values, interpreted as UTF-16 when it is valid to do so.
  • In Rust, strings are always valid UTF-8, which may contain zeros.

So converting some valid paths to String will just panic when you use env::args() as opposed to env::args_os().

Also, pico-args is a lovely little library with support for both space and = separators, and operating on OsString type. It might be of use.

dvc94ch commented 2 years ago

I think the best path forward is to not handle arg parsing. We can have an args struct with any parsed args and do stuff like finding the target_dir, manifest and artifacts. Haven't done it yet because I want to complete an mvp of x first before deciding on the exact path forward.

@MarijnS95 I'm thinking x could possibly one day replace cargo-apk. It's a cargo wrapper for building rust or rust/flutter projects for all platforms and publishing to the apple/play/microsoft stores.

Shnatsel commented 2 years ago

Ah, I suppose that makes sense. No reason to keep the arguments around though - you only need to iterate over them once, might as well use an iterator instead of allocating and storing the entire list.

dvc94ch commented 2 years ago

published as 0.5.1