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.

Sign up for GitHub

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

Jump to bottom

Possibly switch to a more lightweight CLI arg parsing library #596

Open
CosmicHorrorDev opened this issue Jul 12, 2022 · 7 comments
Open

Possibly switch to a more lightweight CLI arg parsing library #596

CosmicHorrorDev opened this issue Jul 12, 2022 · 7 comments

Comments

@CosmicHorrorDev
Copy link

I see that one of the focuses of version 0.4 is to provide a slimmer dependency tree. Following that, would you be open to switching out clap for a more lightweight CLI arg parsing library?

While clap is great for providing user-friendly interfaces it is also a notably heavy dependency. Considering that likely 99% of the invocations for the benchmarking binaries would be by some harness (cargo bench or cargo criterion) we don't have as much motivation to provide user-friendly features. For this reason I think it would be reasonable to switch to a more lightweight library like pico_args

If this seems reasonable proposal then I can open a PR implementing the changes
@bheisler
Copy link
Owner

Hello! Thanks for the suggestion.

Yes, that is part of the goal. I had intended to reach that goal by moving most of the command-line parsing work over to cargo-criterion, which doesn't have to worry as much about the dependency tree because it's compiled and linked once, rather than every time the user runs their benchmarks. I think, in that context, the user-friendliness of clap outweighs the dependency tree.

In Criterion-rs, however, you're right that very nearly all invocations of Criterion-rs benchmarks are through cargo bench or cargo criterion so the user-friendliness is of less value in this context.

I think this would be a fine idea, and a breaking-change release is a good time to do it as well.

@CosmicHorrorDev
Copy link
Author

I'm glad it seems like we're on the same page! I wasn't sure if I conveyed my point very clearly

I think, in that context, the user-friendliness of clap outweighs the dependency tree.

👍 I agree that clap still makes sense for cargo-criterion

Pinging @lemmih since they're leading the whole v0.4 release initiative

I can work on getting a PR out for this based off the version-0.4 branch either later today or tomorrow, so you can merge/cherry-pick/etc it if you want. I also understand if you don't want to wait for this change since scope-creep is a pain, and the v0.4 release has been a long time in the making

@CosmicHorrorDev CosmicHorrorDev mentioned this issue Jul 17, 2022
Closed
@CryZe
Copy link

CryZe commented Aug 21, 2022

criterion also now has a RUSTSEC warning for pulling in the deprecated ansi_term through clap 0.2.

@CryZe CryZe mentioned this issue Aug 21, 2022
Closed
@Razican Razican mentioned this issue Aug 21, 2022
Closed
bors bot pushed a commit to boa-dev/boa that referenced this issue Aug 22, 2022
@Razican
Removed some non-needed dependencies (#2246)
f0caa3c 
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request removes two dependencies that were not really needed, and fixes #2244 by no longer having the package in the dependency tree.

It changes the following:

- The `structopt` dependency in `boa_tester` has been replaced by `clap` v3, the same way as we did in `boa_cli`. This means that we have one less dependency (at least), and that `clap` v2 is only used as a dev-dependency by `criterion` (which will probably be removed in 0.4, as per bheisler/criterion.rs#596).
- The no-longer-updated `num-format` dependency has been removed from `boa_tester`. We were only using it to add comma thousands separator on results, so I added a simple function to do it (not very performant, but it will only be used a few times when showing results).

Looking at this, I noticed a couple of things:

 - The `csv` dependency, used by `criterion` has not been updated in more than a year, and it's using a very old `itoa` dependency. They updated the dependency in the repository in March, but unfortunately, the release is taking some more time than expected, and a tracking issue can be found here: BurntSushi/rust-csv#271
 - `cargo update` fails, because the latest update to `tinystr` in the ICU4x breaks ICU4x 0.6. I have reported this here: unicode-org/icu4x#2428 and their recommendation is for us to use a beta version of the library, but I don't think we should go for that, since this is a semver breakage.
@Razican
Copy link

Razican commented Oct 10, 2022

clap 4.0 brings some nice enhancements in terms of build times, used dependencies and speed, along with binary size impact. One option, while this is decided, would be to upgrade to clap 4, which should be reasonably easy.

@Razican Razican mentioned this issue Oct 10, 2022
Closed
@CosmicHorrorDev
Copy link
Author

CosmicHorrorDev commented Oct 10, 2022 •
edited
Loading

The diff posted for the update to clap v4 shows that lexopt is still significantly faster for both full and incremental debug build times (which I would assume holds true for release build times as well, but I haven't checked)

Considering that criterion is intended as a dev dependency I'm not really concerned with binary size as well

I think it's still advantageous to switch to a lighter command line arg parsing library

@Razican
Copy link

Razican commented Oct 10, 2022

The diff posted for the update to clap v4 shows that lexopt is still significantly faster for both full and incremental debug build times (which I would assume holds true for release build times as well, but I haven't checked)

Considering that criterion is intended as a dev dependency I'm not really concerned with binary size as well

I think it's still advantageous to switch to a lighter command line arg parsing library

Sounds reasonable. Is there any ETA for this?

@CosmicHorrorDev
Copy link
Author

Is there any ETA for this?

On my end I have to update and fix merge conflicts for #599 which I should get to this week

No ETA on getting it merged with a new criterion version out though

bors bot pushed a commit to boa-dev/boa that referenced this issue Oct 15, 2022
@Razican @jedel1043
Upgrade clap to 4.0, add value hints for zsh and fish (#2336)
9998a67 
This Pull Request fixes/closes #2330.

It changes the following:

- Upgrades from clap 3 to clap 4 (note that criterion still uses clap 3, tracked here: bheisler/criterion.rs#596)
- Updates the derive syntax with the new 4.0 syntax
- Adds hints for fish & zsh


Co-authored-by: José Julián Espina <jedel0124@gmail.com>
otravidaahora2t added a commit to otravidaahora2t/boa that referenced this issue Aug 2, 2024
@otravidaahora2t @Razican
Removed some non-needed dependencies (#2246)
382065c 
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request removes two dependencies that were not really needed, and fixes #2244 by no longer having the package in the dependency tree.

It changes the following:

- The `structopt` dependency in `boa_tester` has been replaced by `clap` v3, the same way as we did in `boa_cli`. This means that we have one less dependency (at least), and that `clap` v2 is only used as a dev-dependency by `criterion` (which will probably be removed in 0.4, as per bheisler/criterion.rs#596).
- The no-longer-updated `num-format` dependency has been removed from `boa_tester`. We were only using it to add comma thousands separator on results, so I added a simple function to do it (not very performant, but it will only be used a few times when showing results).

Looking at this, I noticed a couple of things:

 - The `csv` dependency, used by `criterion` has not been updated in more than a year, and it's using a very old `itoa` dependency. They updated the dependency in the repository in March, but unfortunately, the release is taking some more time than expected, and a tracking issue can be found here: BurntSushi/rust-csv#271
 - `cargo update` fails, because the latest update to `tinystr` in the ICU4x breaks ICU4x 0.6. I have reported this here: unicode-org/icu4x#2428 and their recommendation is for us to use a beta version of the library, but I don't think we should go for that, since this is a semver breakage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Switch CLI arg parsing from clap to lexopt CosmicHorrorDev/criterion.rs
4 participants
@Razican @CryZe @bheisler @CosmicHorrorDev