webai-community / aquarium

BSD 3-Clause "New" or "Revised" License
26 stars 7 forks source link

Introduce cxxopts to better parse options #124

Closed gyagp closed 4 years ago

gyagp commented 4 years ago

cxxopts (https://github.com/jarro2783/cxxopts) is an open source project as a lightweight C++ option parser. This CL introduces this library (header file only) to better parse the options.

hujiajie commented 4 years ago

Sorry about the CI instability. I'll see if any action can be taken there.

Meanwhile, I'd like to make sure if cxxopts is the right choice or not.

gyagp commented 4 years ago

The choice of cxxopts is due to limited investigations. For example, here is a post (https://attractivechaos.wordpress.com/2018/08/31/a-survey-of-argument-parsing-libraries-in-c-c/) to compare some of famous alternatives. Some traits I care most and I think suitable for Aquarium are:

  1. lightweight (only .h file can be a good option). Boost and Google gflags are a bit too heavy.
  2. Easy to config, like python argparse.
  3. supportive of features like both short and long options, type check, multi-type characters, etc.

To develop such an option parser requires non-trivial effort, which is a reason I hope we don't take at current stage. Google may also care about license, so they'd like to create a new wheel every time. It's not perfect, but a step forward comparing with the very raw way we used before. If we see a better solution in future, we may easily replace it. After all, it's just 1 day effort.

hujiajie commented 4 years ago

I still have some concerns, is there any security audit for cxxopts? I'd like to minimize the risk of backdoors. Also, I hope such a library can be placed in third_party and managed by DEPS.

hujiajie commented 4 years ago

@Jiawei-Shao @shaoboyan What's your opinion on this patch?

gyagp commented 4 years ago

If this may dismiss some concern from you, Microsoft included cxxopts in vcpkg: https://devblogs.microsoft.com/cppblog/vcpkg-a-tool-to-acquire-and-build-c-open-source-libraries-on-windows/

Jiawei-Shao commented 4 years ago

Thanks @hujiajie.

I also don't like the current implementation of command line parser inside Aquarium because right now if we want to add a new command line parameter, we should:

It would be great if we can define the command line parameter, its toggle and its parser together in one place, and generate the usage information directly from this definition instead of writing them separately in different places.

I think we are not using the command line parameters with very complicated formats ("--xxx" and "--xxx=[integer]"), so maybe it is not very hard to write our own command line parameter parser?

gyagp commented 4 years ago

To write an option parser from scratch is still a non-trivial task, which may need 1000+ lines of code. To suit our usage, it at least needs to provide following functions:

  1. support short and long options
  2. types (int, bool, string, etc.) check and parser
  3. default value
  4. formatted help message
  5. how to handle multiple same options

I think cxxopts already provides all these functions, and I just used a single day to do all the changes. It makes code more robust and maintainable.

shaoboyan commented 4 years ago

@hujiajie thanks. I tend to support this CL. My opinions:

  1. Aquarium will accept lots of incoming requests and in this state, performance is more important than security. A third-party library helps on the trivial function will save your time. (No need to pay effort to parse weired options)
  2. If we choose to handle the parser ourselves, it means we will write a subset of cxxopts functions. I think we can avoid it.
  3. We still have chance to review it when Aquarium is in stable state and decide whether we can write our own parser to handle the options or replace this library or keep it.

WDYT?

hujiajie commented 4 years ago

Thanks for the feedback. It sounds the short term plan is to pull in cxxopts, but eventually it will be replaced with our own option parser.

@gyagp Please make cxxopts a third party deps and create a corresponding GN target in //third_party/BUILD.gn.

gyagp commented 4 years ago

Thanks for the decision. cxxopts is header file only, so it doesn't need any target in //third_party/BUILD.gn. Please review the new change.

gyagp commented 4 years ago

Thanks for the comments. Please take another review!

gyagp commented 4 years ago

Thanks for the careful review! PTAL.

hujiajie commented 4 years ago

LGTM, thanks!