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

Be consistent with command line parsing by standardizing on clap or argh #2373

Closed
stmcginnis opened this issue Aug 23, 2022 · 10 comments
Closed
Assignees
Labels
area/core Issues core to the OS (variant independent) area/tools Issues in the build, publishing or other tooling1 or scripts good first issue May be a good issue for new contributors status/in-progress This issue is currently being worked on type/enhancement New feature or request

Comments

@stmcginnis
Copy link
Contributor

What I'd like:

Most of our CLIs use the argh crate for parsing command line input. But some are using clap instead.

It would be good to convert these over to argh so we are consistent across our code and avoid importing two different crates that provide the same functionality.

Any alternatives you've considered:

Could keep them as is since there is no conflict between using the mix of the two, but not a real reason not to just use one or the other.

@stmcginnis stmcginnis added the good first issue May be a good issue for new contributors label Aug 23, 2022
@alok-sin
Copy link

I would like to take this. Does this concern with the testsys repo as well? bottlerocket-os/bottlerocket-test-system#535

@stmcginnis
Copy link
Contributor Author

Awesome, that would be great @alok-sin!

I'm not 100% on the bottlerocket-test-system changes yet. There has been some talk of removing some of that code, so I don't want to suggest you make changes there only to have it all be removed.

If you could start in this repo that would be ideal. Then we can hopefully have a little better understanding if anything should be updated in the other repo once that is done.

Thanks a lot!

@webern
Copy link
Contributor

webern commented Oct 10, 2022

How did you arrive at standardizing on argh instead of clap? I think clap is becoming the de-facto standard crate for this.

@stmcginnis
Copy link
Contributor Author

@bcressey had expressed a desire to go with argh over clap. If clap is becoming the standard for this, then perhaps we should reevaluate that position. I am fine either way - I would just like to see us using one or the other.

@bcressey
Copy link
Contributor

I don't feel as strongly about "only one" but I definitely don't want to add clap to the binaries shipped in the OS image, and would prefer that the mix of custom / structopt / argh that is currently shipped get trimmed down to just argh.

That's primarily for reasons of code size. It's not a huge factor for existing builds, but there are some use cases (like a microVM guest) where size is a concern. I don't like the idea of spending 10+ MiB of space for arg parsing that no one will ever see.

@webern
Copy link
Contributor

webern commented Oct 11, 2022

I don't feel as strongly about "only one"

I also don't feel strongly about "only one" and would hate to see us go from clap -> argh in places where we are using clap now.

Have you checked the size of the compiled code of clap vs argh? Just curious if it's a big contributor to binary size.

@stmcginnis stmcginnis added the status/needs-triage Pending triage or re-evaluation label Dec 1, 2022
@stmcginnis stmcginnis changed the title Be consistent with command line parsing by standardizing on the argh crate. Be consistent with command line parsing by standardizing on clap or argh Mar 30, 2023
@stmcginnis
Copy link
Contributor Author

structopt is deprecated, so we should review what we're using and migrate to whichever makes the most sense.

@stmcginnis stmcginnis added area/core Issues core to the OS (variant independent) status/icebox Things we think would be nice but are not prioritized type/enhancement New feature or request and removed status/needs-triage Pending triage or re-evaluation labels Mar 30, 2023
@jpculp
Copy link
Member

jpculp commented May 15, 2023

I looked into this last time it was brought up and while argh is smaller it is also more limited in terms of features and flexibility, requiring some refactor. I agree we should get off structopt, but I think we'd have an easier time going the clap route for things outside of the sources workspace.

@stmcginnis
Copy link
Contributor Author

Sorry, I noticed an older, closed issue and started poking around. I ended up submitting a couple PRs to address this before I realized we had this open. I'm going to assign back to myself and close this out if/when we merge #3228 and #3229.

@stmcginnis stmcginnis assigned stmcginnis and unassigned alok-sin Jun 26, 2023
@stmcginnis stmcginnis added status/in-progress This issue is currently being worked on area/tools Issues in the build, publishing or other tooling1 or scripts and removed status/icebox Things we think would be nice but are not prioritized labels Jun 26, 2023
@stmcginnis
Copy link
Contributor Author

Marking complete as #3228 and #3229 have merged and should address things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core Issues core to the OS (variant independent) area/tools Issues in the build, publishing or other tooling1 or scripts good first issue May be a good issue for new contributors status/in-progress This issue is currently being worked on type/enhancement New feature or request
Projects
Development

No branches or pull requests

5 participants