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

Rewrite Multicall handling to just strip path off argv0 #3041

Merged
merged 3 commits into from Dec 13, 2021

Conversation

fishface60
Copy link
Contributor

@fishface60 fishface60 commented Nov 19, 2021

This follows the suggestion of Ed Page in #2861 that simplifying the behaviour and making it less magic could solve some problems.
Having spent some time writing an app to try it out this has proven correct.
Now if you want busybox-style where each applet is also available as a subcommand, you have to code it explicitly.
This is inconvenient when using it as a builder, but not too much trouble with clap_derive

#[derive(Args)]
struct False {}

#[derive(Args)]
struct True {}

#[derive(Parser)]
enum Busybox {
    False(False),
    True(True),
}

#[derive(Parser)]
#[clap(setting = AppSettings::Multicall)]
enum Applet {
    #[clap(subcommand)]
    Busybox(Busybox),
    False(False),
    True(True),
}

let applet = Applet::parse();
match applet {
    Applet::Busybox(Busybox::True(true_args)) | Applet::True(true_args) {
        std::process:exit(0)
    }
    Applet::Busybox(Busybox::False(false_args)) | Applet::False(false_args) {
        std::process:exit(1)
    }
}

Prefix stripping

Support for prefix stripping becomes easier because now the prefix may be explicitly added to the beginning of the applet name without it being added to the subcommand name.
This is more convenient with clap_derive since the name only needs to be set in the struct definition and the result is accessed symbolically.

Assuming the PROGRAM_PREFIX variable is set by build.rs, the busybox definition is modified to become:

#[derive(Parser)]
#[clap(setting = AppSettings::Multicall)]
enum Applet {
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "busybox"), subcommand)]
    Busybox(Busybox),
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "false"))]
    False(False),
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "true"))]
    True(True),
}

Closes #2864

Support programs with a subcommand of the same name

This becomes trivial because the main applet and its subcommands are added explicitly.

Closes #2865

Performance impact on startup

This was because deciding whether to run the main applet or not was conditional on whether the subcommand matched.
Now that it depends on the developer specifying the mechanism for how to run the main applet, if you don't have a main applet you don't waste time checking for it.

Closes #2866
Closes #3074

@fishface60 fishface60 marked this pull request as draft November 19, 2021 22:27
@fishface60 fishface60 marked this pull request as ready for review November 20, 2021 00:09
@pksunkara pksunkara requested a review from epage December 7, 2021 05:44
@epage
Copy link
Member

epage commented Dec 7, 2021

Sorry for how long is has sat open. I'm hoping to get to this soon,. I'm assuming this has rebased off of the latest master, which has same major code organization changes.

examples/multicall_busybox.rs Outdated Show resolved Hide resolved
examples/multicall_busybox.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Show resolved Hide resolved
src/build/app/settings.rs Show resolved Hide resolved
clap_generate/examples/multicall.rs Outdated Show resolved Hide resolved
examples/multicall_busybox.rs Outdated Show resolved Hide resolved
examples/multicall_busybox.rs Outdated Show resolved Hide resolved
@epage
Copy link
Member

epage commented Dec 8, 2021

Forgot to comment on it

#[derive(Args)]
struct False {}

#[derive(Args)]
struct True {}

#[derive(Parser)]
enum Busybox {
    False(False),
    True(True),
}

#[derive(Parser)]
#[clap(setting = AppSettings::Multicall)]
enum Applet {
    #[clap(subcommand)]
    Busybox(Busybox),
    False(False),
    True(True),
}

let applet = Applet::parse();
match applet {
    Applet::Busybox(Busybox::True(true_args)) | Applet::True(true_args) {
        std::process:exit(0)
    }
    Applet::Busybox(Busybox::False(false_args)) | Applet::False(false_args) {
        std::process:exit(1)
    }
}

Why aren't you doing

#[derive(Args)]
struct False {}

#[derive(Args)]
struct True {}

#[derive(Parser)]
enum Busybox {
    False(False),
    True(True),
}

#[derive(Parser)]
#[clap(setting = AppSettings::Multicall)]
enum Applet {
    #[clap(subcommand)]
    Busybox(Busybox),
    #[clap(flatten)]
    Applet(Busybox),
}

let applet = Applet::parse();
match applet {
    Applet::Busybox(Busybox::True(true_args)) | Applet::Applet(Busybox::True(true_args)) {
        std::process:exit(0)
    }
    Applet::Busybox(Busybox::False(false_args)) | Applet::Applet(Busybox::False(false_args)) {
        std::process:exit(1)
    }
}

At that point, you could add exec functions to True / False, and all the enums on up to dispatch to the variant.

@fishface60
Copy link
Contributor Author

Sorry for how long is has sat open. I'm hoping to get to this soon,. I'm assuming this has rebased off of the latest master, which has same major code organization changes.

No worries, looks like you've been busy yourselves 🙂

Forgot to comment on it

At that point, you could add exec functions to True / False, and all the enums on up to dispatch to the variant.

Yeah, I think that would be a neater way of doing it in this case.
I was cribbing from my hobby project 's parser for the example, which can't do that because there's more subcommands than applets and the subcommands for a deeper hierarchy. impling an exec on all the variants in-between didn't feel worth the effort compared to dispatching in a top-level match.

Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we drop the example and you don't feel like changing to match, then this looks ready to merge.

Let me know either way you want to go. I'll not be available tomorrow to look at this but can the day after

examples/multicall_busybox.rs Outdated Show resolved Hide resolved
examples/multicall_busybox.rs Show resolved Hide resolved
clap_generate/examples/multicall.rs Outdated Show resolved Hide resolved
The executable suffix is unconditionally stripped off the file path
so that the file name matches subcommands names
without having to add the EXE suffix on different platforms.
Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@epage epage merged commit b0f1750 into clap-rs:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants