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

refactor: use argp library for parsing command line options and arguments #1525

Closed
wants to merge 3 commits into from

Conversation

rjd15372
Copy link
Contributor

@rjd15372 rjd15372 commented Jan 8, 2025

This commit changes the command line parsing code of valkey-benchmark to use the argp library (https://www.gnu.org/software/libc/manual/html_node/Argp.html), which is included in libc.

This makes the options and arguments parsing more structured and easy to extend.

…uments

This commit changes the command line parsing code of `valkey-benchmark`
to use the `argp` library (https://www.gnu.org/software/libc/manual/html_node/Argp.html),
which is included in libc.

This makes the options and arguments parsing more structured and easy to
extend.

Signed-off-by: Ricardo Dias <[email protected]>
@rjd15372 rjd15372 force-pushed the refactor-benchmark-cli branch from a2b73d7 to ac868bd Compare January 8, 2025 15:36
@madolson
Copy link
Member

madolson commented Jan 8, 2025

argp is apparently not on macos.

@rjd15372
Copy link
Contributor Author

rjd15372 commented Jan 9, 2025

argp is apparently not on macos.

Shoot. I should have checked that before implementing this. I guess that's why the parsing was being done manually.

@rjd15372 rjd15372 force-pushed the refactor-benchmark-cli branch from e0082be to 9513c43 Compare January 9, 2025 15:31
@rjd15372 rjd15372 force-pushed the refactor-benchmark-cli branch from 9513c43 to fb64423 Compare January 9, 2025 15:55
Signed-off-by: Ricardo Dias <[email protected]>
@rjd15372 rjd15372 force-pushed the refactor-benchmark-cli branch from f18a6d6 to 83d001c Compare January 10, 2025 10:32
@rjd15372
Copy link
Contributor Author

To fix the problem of argp not being available in macosx libc, I've added arpg-standalone package as a build dependency for macosx. argp-standalone is easily installed using homebrew, as we do in the github action that runs the build in macosx.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 53.10345% with 68 lines in your changes missing coverage. Please review.

Project coverage is 70.90%. Comparing base (b3b4bdc) to head (83d001c).
Report is 31 commits behind head on unstable.

Files with missing lines Patch % Lines
src/valkey-benchmark.c 53.10% 68 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1525      +/-   ##
============================================
+ Coverage     70.83%   70.90%   +0.06%     
============================================
  Files           120      120              
  Lines         64911    64999      +88     
============================================
+ Hits          45982    46086     +104     
+ Misses        18929    18913      -16     
Files with missing lines Coverage Δ
src/valkey-benchmark.c 59.05% <53.10%> (-1.27%) ⬇️

... and 53 files with indirect coverage changes

Comment on lines +184 to +188
# GNU libc argp library needs to be installed with homebrew:
# `brew install argp-standalone`
FINAL_LIBS+= -largp
FINAL_LDFLAGS+= -L/opt/homebrew/lib
FINAL_CFLAGS+= -I/opt/homebrew/include
Copy link
Member

@madolson madolson Jan 10, 2025

Choose a reason for hiding this comment

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

This requires all mac users to use homebrew to use the cli doesn't it, where it is working just fine today.

I guess I'm wondering is it really so important to remove our custom parsing logic?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I can't use homebrew on my company issued mac

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 guess it's not worth changing the custom parsing logic if it does not work in all platforms out of the box.

@rjd15372
Copy link
Contributor Author

Closing this PR since argp is not available in macosx.

@rjd15372 rjd15372 closed this Jan 15, 2025
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.

3 participants