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

Add support for Multicall executables as subcommands with a Multicall setting #2817

Merged
merged 13 commits into from Oct 16, 2021

Conversation

fishface60
Copy link
Contributor

If the AppSettings::Multicall setting is given
then argv0 is parsed as the first subcommand argument
or parsed as normal with any other name.

This handles the feature from the linked issue of hostname-like behaviour where applets shouldn't be callable from the main command, by having a subcommand with the same name as the main command i.e. hostname has a hostname subcommand and being called with the executable name hostname runs the hostname subcommand name and runs the dnsdomainname subcommand if linked as dnsdomainname

This doesn't handle the busybox feature of nicely reporting that an applet isn't found when a subcommand doesn't match.

This doesn't handle the uutils/coreutils feature of matching names with an optional prefix.

The known unhandled features were left unimplemented for lack of a good idea how to implement and they could be added as additional flags.

Closes #1120

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

epage commented Oct 5, 2021

This doesn't handle the uutils/coreutils feature of matching names with an optional prefix.

This could always be added in the future as an App method for people to set the prefix. I say future because this is an incremental improvement on top of what you have and wouldn't want to see the existing work be blocked on it.

@fishface60
Copy link
Contributor Author

Pushed an update for everything discussed except for checking against name instead of searching for applets, which I'll have to look at tomorrow.


fn main() {
let args = Args::parse();
if let Some(path) = args.install {
Copy link
Member

Choose a reason for hiding this comment

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

I'm open to opinions here, but I would take out all of the --install logic for the examples as it gets in the way of demo'ing this feature. I would instead just put a comment at the top of the example either quickly explaining what Multicall is and how to set it up manually (i.e. just some links), or give a URL to a blog/article explaining the subject.

Also, the choice of using true and false might also get in the way since many shells will use a builtin, and it requires checking exit codes to verify which isn't explicitly explained. Just using foo / bar and printing those words I think will demo this just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a more well-known example of a multicall binary than busybox, and --install is a feature of it that I think is useful to demonstrate how it would work, even if you'd typically see the links installed via the package manager.
But I can limit that to one example, and doing the hostname example as the clap_derive example will solve a bunch of problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular example no longer exists, though the non-derive equivalent has an --install option.
It's a lot less noise since it's unimplemented.
true and false were implemented as they're trivial, but if you think commands which aren't available as builtins would be more useful, cat, cp and rm might be relevant even if unimplemented.

src/build/app/mod.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Looks good as first pass.

I agree that we make this unstable because I think there are a few ways we can resolve the "unresolved" questions by creating a flag like @kbknapp proposed. But that's for later.

clap_derive/examples/hostname.rs Outdated Show resolved Hide resolved
examples/24_multicall_busybox.rs Outdated Show resolved Hide resolved
src/build/app/settings.rs Show resolved Hide resolved
src/build/app/settings.rs Outdated Show resolved Hide resolved
tests/subcommands.rs Outdated Show resolved Hide resolved
clap_derive/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 16, 2021

Build succeeded:

@bors bors bot merged commit b835ce9 into clap-rs:master Oct 16, 2021
@fishface60
Copy link
Contributor Author

I was expecting an autosquash to happen before merge, sorry about the noisy commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for multicall binaries
4 participants