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

Updated CLI Argument Parsing (New) #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JamesDevlin5
Copy link

@JamesDevlin5 JamesDevlin5 commented Dec 23, 2021

Re-implemented the argument parsing module, now using the nightly version of clap, and a derive-attribute pattern instead of the previous builder pattern, as suggested by @Xunjin in the previous pull request. The help page is colorized and very clean looking. Some small changes have yet to be fully implemented:

  • The glob attribute on the cli arguments struct is commented out
  • The unkillables cli argument is not split by tilde

Other than that, everything looks pretty good!

Transitioned from crate `argh` to `clap`. Created identical
  argument-parsing framework, with some additional elements, such as
  short and long flags for all items, instead of the previous only short
  flags. Currently `cargo check` passes with no warnings and the help
  page that is printed is fairly pretty.
Implemented the derive-attributes pattern for the config struct, moving
  away from the builder pattern. Almost identical to the previous
  version except the unkillable list of globs is not split by the tilde
  delimeter character. That will likely have to be implemented via the
  validator attribute, I just haven't done it yet.
Copy link

@Xunjin Xunjin left a comment

Choose a reason for hiding this comment

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

Looks awesome to me, what do you think @vrmiguel and @marcospb19 ?

@JamesDevlin5
Copy link
Author

Also, not sure where else to post this but I wanted to let you know I created an arch linux package for bustd, on the aur. And additionally, created a packge for pacman hooks to enable, restart, and disable the bustd service with systemd.

@marcospb19
Copy link
Contributor

Also, not sure where else to post this but I wanted to let you know I created an arch linux package for bustd, on the aur. And additionally, created a packge for pacman hooks to enable, restart, and disable the bustd service with systemd.

Awesome! Thanks for shipping bustd!


The only drawback of using clap is the increase in binary size:

$ du bustd pr-bustd --apparent-size -h
747K	pr-bustd
355K	bustd

All binaries compiled in release profile, and stripped.

New size (with this pr) is 210% of the previous one.

I am not sure if the gains in ergonomy are more important, as I don't expect people to be invoking bustd manually.


Note: I also tried compiling with opt-level = 'z':

347K	old
647K	new

Here the it drops from 210% to 186%, which is a bit better.


By running the following script:

B=binary ; ./$B ; sleep 1 ; pmap $(pgrep $B) ; pkill $B

For both binaries, I compared the memory consumption.

Current: 3040K
PR: 3432K

With opt-level = 'z':

Current: 3036K
PR: 3336K


Final decision of merging is up to @vrmiguel, but I know that he used argh instead of clap for this project with the purpose of keeping it tiny.

@Xunjin
Copy link

Xunjin commented Dec 27, 2021

Also, not sure where else to post this but I wanted to let you know I created an arch linux package for bustd, on the aur. And additionally, created a packge for pacman hooks to enable, restart, and disable the bustd service with systemd.

Awesome! Thanks for shipping bustd!

The only drawback of using clap is the increase in binary size:

$ du bustd pr-bustd --apparent-size -h
747K	pr-bustd
355K	bustd

All binaries compiled in release profile, and stripped.

New size (with this pr) is 210% of the previous one.

I am not sure if the gains in ergonomy are more important, as I don't expect people to be invoking bustd manually.

Note: I also tried compiling with opt-level = 'z':

347K	old
647K	new

Here the it drops from 210% to 186%, which is a bit better.

By running the following script:

B=binary ; ./$B ; sleep 1 ; pmap $(pgrep $B) ; pkill $B

For both binaries, I compared the memory consumption.

Current: 3040K PR: 3432K

With opt-level = 'z':

Current: 3036K PR: 3336K

Final decision of merging is up to @vrmiguel, but I know that he used argh instead of clap for this project with the purpose of keeping it tiny.

Well I agree it does increase a lot if we look at percentage wise, however if you look at UX I believe it's the choice for this project to focus on. Sorry, but the final binary still under 1 MB, that's not a problem at all.

Could you give any update here @vrmiguel ?

@JamesDevlin5
Copy link
Author

Another thought too, I also added the version flag, and all options have both long and short options, and the unkillables is included by default, whereas before it was only if the glob feature was enabled (I was just having some trouble implementing this, I've never done it before...). I'd guess that the clap crate does add some memory baggage, but if these flags were implemented using the previous framework they'd add at least a bit to the binary size, leveling the playing field a bit. (I just don't want to go back and implement these and I'm not familiar with argv.)

@vrmiguel
Copy link
Owner

vrmiguel commented Jun 3, 2023

Hey @JamesDevlin5! I'm very sorry for the catastrophic response to this PR 😢

I'm very glad you took time out to contribute to this project but a multitude of reasons made me take wayyy too long to consider if clap would be good for bustd, which led to this PR getting out of sync (and well, very old), for which I'd like to apologize

In any case, I'd argue that bustd benefits from a lower-profile smaller crate such as argh since it allows the binary to be smaller and use (very slightly) less RAM

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.

None yet

4 participants