Provide arguments through struct which can also be parsed through `Clap` #15

dvc94ch · 2022-03-08T18:11:00Z

So this performs the refactoring we discussed and adds support for [lib] name

dvc94ch · 2022-03-12T11:30:38Z

dvc94ch · 2022-03-16T11:03:50Z

@MarijnS95 had time to take a look yet?

MarijnS95 · 2022-03-16T16:49:18Z

@dvc94ch Not yet, I've only just returned and am already in over my head on other issues, but at a quick glance this looks very promising.

I'll give it more thorough review in the off-hours, maybe later this week or over the weekend.

Meanwhile, you know I'm a big fan of descriptive titles 😉 - how about something like Provide arguments through struct which can also be parsed through `Clap`?

MarijnS95

Overall very glad to see clap support but I'm not sure we can get away with passing just the few arguments that we support to cargo, and that's not very future-proof either?

Additionally the [lib] change should IMO be done in a separate (fllowup?) PR for easier review and discussion, as this is a bit bigger topic that should be solved for [[bin]] at the same time.

src/args.rs

src/profile.rs

MarijnS95 · 2022-03-22T11:51:51Z

src/args.rs

+    pub fn apply(&self, cmd: &mut Command) {
+        if self.quiet {
+            cmd.arg("--quiet");
+        }
+        if self.release {
+            cmd.arg("--release");
+        }
+        if let Some(target) = self.target.as_ref() {
+            cmd.arg("--target").arg(target);
+        }
+        if let Some(profile) = self.profile.as_ref() {
+            cmd.arg("--profile").arg(profile.to_string());
+        }
+        for example in &self.example {
+            cmd.arg("--example").arg(example);
+        }
+        if self.examples {
+            cmd.arg("--examples");
+        }
+        for bin in &self.bin {
+            cmd.arg("--bin").arg(bin);
+        }
+        if self.bins {
+            cmd.arg("--bins");
+        }
+        if let Some(package) = self.package.as_ref() {
+            cmd.arg("--package").arg(package);
+        }
+        if let Some(target_dir) = self.target_dir.as_ref() {
+            cmd.arg("--target-dir").arg(target_dir);
+        }
+        if let Some(manifest_path) = self.manifest_path.as_ref() {
+            cmd.arg("--manifest-path").arg(manifest_path);
+        }
+    }


Seems to be fine for now, but it's a bit of hand-writing and should be carefully maintained when new fields are added. As mentioned https://github.com/rust-windowing/android-ndk-rs/pull/238/files#r832076774 I think we should keep passing the original array of arguments as-is. I don't think we have any cargo-apk-specific fields that need to be filtered out, and it is probably possible for clap to give us an unparsed array of all cmdline arguments that follow after parsing the initial subcommand selections?

Alternatively I have a serde::Serializer implementation locally that generates a cmdline (which we use internally for many, many structures that are all parsable through clap) which I could share or publish as a separate crate.
The only downside is requiring serde as an unconditional dependency here to make this work.

(That said, this apply() function is probably mandatory for the case where the caller constructs the Args struct directly by hand instead of having a list of string arguments that can be parsed through clap)

don't think all args should be passed to cargo. it is possible for a subcommand to have it's own args that cargo doesn't understand.

We don't have any for the time being. And it seems this PR misses some high-grossing flags like those relating to feature flags. Those should undoubtedly be passed to cargo.

src/artifact.rs

src/subcommand.rs

MarijnS95 · 2022-03-22T12:15:10Z

Fwiw the [lib] change is a separate commit already so it should be pretty trivial to pop that off here and stage it for a second PR?

MarijnS95 · 2022-03-22T12:20:35Z

Another thing that jumps to mind: is there any possibility that we could get cargos "official" argument and manifest parsing/combining logic/implementation in a separate crate, so that we can reuse it instead of trying to mimic it to the best of our abilities?

Even better would be if cargo-apk and friends are extensions to cargo with a well-defined interface, but that's long-term future plans.

dvc94ch · 2022-03-22T13:10:51Z

Overall very glad to see clap support but I'm not sure we can get away with passing just the few arguments that we support to cargo, and that's not very future-proof either?

I think the correct way to do it would be to support something like -- to forward args to cargo that cargo-apk doesn't know about. didn't figure out how to do that with clap yet.

MarijnS95 · 2022-03-22T13:47:53Z

I found some mentions on the clap repo, though it seems most examples use the hand-crafted parser and not the derive one.

something like -- to forward args to cargo that cargo-apk doesn't know about

Not sure about that since the user should be oblivious to whether cargo-apk supports things or not, everything should just be uniformly passed to cargo IMO.

The only tricky bit seems getting everything after the cargo-apk subcommands, ie. cargo apk run <everything here we want to pass to `cargo` as-is> because some of those arguments (the ones listed in your struct Args specifically) need to be read by cargo-subcommand.

If it wasn't for the few prefixed clap subcommand things, we could just pass std::env::args() to the cargo process directly. Alternatively clap needs to give us the remainder of unparsed arguments which we then have to combine back with the stuff from struct Args here, or parse the top-level subcommands separately from cargo_subcommand::Args.

dvc94ch · 2022-03-25T10:13:54Z

Not sure about that since the user should be oblivious to whether cargo-apk supports things or not, everything should just be uniformly passed to cargo IMO.

I disagree. This causes confusion. Now when using clap all the args cargo apk understands are in the help message. Of these args that cargo-apk understands, cargo-apk decides which to forward to cargo and how. What's missing is passing args that cargo-apk does not understand.

MarijnS95 · 2022-03-25T10:29:52Z

For the time being cargo-apk/cargo-subcommand support and use all the arguments that cargo build and friends support too.

I agree that it should be clear what cargo-apk and friends do and don't support. For example no --bench support should result in a cargo-subcommand error instead of being silently passed to cargo without the caller knowing what to do with it. However, this PR also misses --feature (and friends) support and there's probably a couple more flags that are important to pass through.

The bit you quoted is where I specifically condemn the (ab)use of -- for this purpose. I don't want to see a cargo apk r -p something --examples -- --feature foo,bar, just because cargo-subcommand happens to know what to do with -p and --examples but has no idea about --feature. That's not friendly to the user. Neither because in cargo run, -- is used to pass arguments to the running executable rather than cargo, and I want to do the same in cargo-apk (pass flags to adb shell am start, to set extra intent data in a cmdline fashion).

dvc94ch · 2022-03-25T11:01:50Z

well I added the missing args, but looks like the logic/api will need to get quite a bit more complicated now. I think we need something like this:

Artifact {
    manifest: PathBuf,
    package: String,
    source: PathBuf,
    destination: PathBuf,
    ty: bin/example/lib
}

really don't have time to work on this at the moment, do you want to take over?

MarijnS95 · 2022-03-25T11:35:54Z

Don't worry about that for now. Let's keep this PR exclusively about migrating to clap, then we can take on artifact changes and renamings in a separate PR.

What of the above made you expand Artifact like this? Seems like we can still gather most from the Subcommand around it, but it would be nice to have a specific ty so that cargo-apk can for example demand that the artifacts are all libraries.

EDIT: Yes I guess for --all/--workspace we'll have to make our artifacts aware of the package+bin/lib/example/whatever name. Let's take that on in a followup change.

dvc94ch · 2022-03-25T11:45:47Z

Well, cargo-subcommand assumes that we're building one package. But cargo supports taking multiple packages as args. Each package has it's own manifest. So cargo apk build --workspace --examples should result in an apk for each example in each package in the workspace. If we only support a single package arg and remove support for the workspace arg we can get away with what we did so far.

MarijnS95 · 2022-03-25T12:12:32Z

@dvc94ch Right, I only thought about adding --feature etc. We can comment out these members for now, or add an assert, explaining that this functionality isn't yet available. Also something I feel we should tackle in a separate PR, as this is already pretty much good to go for a merge.

src/subcommand.rs

src/args.rs

src/subcommand.rs

Co-authored-by: Marijn Suijten <[email protected]>

dvc94ch requested a review from MarijnS95 March 8, 2022 18:11

dvc94ch force-pushed the refactoring branch from a94080d to d361169 Compare March 8, 2022 19:11

dvc94ch changed the title ~~Refactoring~~ Provide arguments through struct which can also be parsed through Clap Mar 16, 2022

MarijnS95 reviewed Mar 22, 2022

View reviewed changes

MarijnS95 mentioned this pull request Mar 24, 2022

Support autobins and target renaming [lib] and [[bin]] #17

Merged

dvc94ch added 2 commits March 25, 2022 10:58

Remove Cargo.lock

653ee0d

Use clap instead of hacky arg parsing.

d4a2cdf

dvc94ch force-pushed the refactoring branch from 6f6abbc to d4a2cdf Compare March 25, 2022 10:06

Update args.

d142e7a

Make it build again.

6a5ed47

MarijnS95 reviewed Mar 25, 2022

View reviewed changes

src/subcommand.rs Outdated Show resolved Hide resolved

MarijnS95 reviewed Mar 25, 2022

View reviewed changes

src/args.rs Outdated Show resolved Hide resolved

MarijnS95 reviewed Mar 25, 2022

View reviewed changes

src/subcommand.rs Outdated Show resolved Hide resolved

MarijnS95 approved these changes Mar 25, 2022

View reviewed changes

Update src/subcommand.rs

dc9b294

Co-authored-by: Marijn Suijten <[email protected]>

dvc94ch force-pushed the refactoring branch from 9f4d3b9 to dc9b294 Compare March 25, 2022 14:02

MarijnS95 approved these changes Mar 25, 2022

View reviewed changes

dvc94ch merged commit 6c77c51 into master Mar 25, 2022

dvc94ch deleted the refactoring branch March 25, 2022 15:13

MarijnS95 mentioned this pull request Mar 29, 2022

Update to cargo-subcommand 0.8.0 with clap argument parser rust-mobile/ndk#238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide arguments through struct which can also be parsed through `Clap` #15

Provide arguments through struct which can also be parsed through `Clap` #15

dvc94ch commented Mar 8, 2022

dvc94ch commented Mar 12, 2022

dvc94ch commented Mar 16, 2022

MarijnS95 commented Mar 16, 2022

MarijnS95 left a comment

MarijnS95 Mar 22, 2022

dvc94ch Mar 25, 2022

MarijnS95 Mar 25, 2022

MarijnS95 commented Mar 22, 2022

MarijnS95 commented Mar 22, 2022

dvc94ch commented Mar 22, 2022 •

edited

Loading

MarijnS95 commented Mar 22, 2022 •

edited

Loading

dvc94ch commented Mar 25, 2022

MarijnS95 commented Mar 25, 2022

dvc94ch commented Mar 25, 2022

MarijnS95 commented Mar 25, 2022 •

edited

Loading

dvc94ch commented Mar 25, 2022 •

edited

Loading

MarijnS95 commented Mar 25, 2022

Provide arguments through struct which can also be parsed through Clap #15

Provide arguments through struct which can also be parsed through Clap #15

Conversation

dvc94ch commented Mar 8, 2022

dvc94ch commented Mar 12, 2022

dvc94ch commented Mar 16, 2022

MarijnS95 commented Mar 16, 2022

MarijnS95 left a comment

Choose a reason for hiding this comment

MarijnS95 Mar 22, 2022

Choose a reason for hiding this comment

dvc94ch Mar 25, 2022

Choose a reason for hiding this comment

MarijnS95 Mar 25, 2022

Choose a reason for hiding this comment

MarijnS95 commented Mar 22, 2022

MarijnS95 commented Mar 22, 2022

dvc94ch commented Mar 22, 2022 • edited Loading

MarijnS95 commented Mar 22, 2022 • edited Loading

dvc94ch commented Mar 25, 2022

MarijnS95 commented Mar 25, 2022

dvc94ch commented Mar 25, 2022

MarijnS95 commented Mar 25, 2022 • edited Loading

dvc94ch commented Mar 25, 2022 • edited Loading

MarijnS95 commented Mar 25, 2022

Provide arguments through struct which can also be parsed through `Clap` #15

Provide arguments through struct which can also be parsed through `Clap` #15

dvc94ch commented Mar 22, 2022 •

edited

Loading

MarijnS95 commented Mar 22, 2022 •

edited

Loading

MarijnS95 commented Mar 25, 2022 •

edited

Loading

dvc94ch commented Mar 25, 2022 •

edited

Loading