switch arg parser to cxxopts

[nilsnolde]

fixes #602

I poc'd it real quick, it's definitely not tested yet, but you'll see the way it'd be implemented.

a few questions from my side:

• the heuristic arg is now showing on help, I guess that's mostly for you to debug? I can't see a way to hide it in cxxopts, but also don't think it's that bad it's showing now
• maybe we can get rid of the CLArgs struct or even cl_args.h and pull the stuff into main.cpp? defaults are applied via cxxopts which is IMHO more idiomatic/expected. main is only passing single parameters to functions as far as I could see, so it wouldn't be a big change I think. anyways, that's maybe micro-"optimizing"..
• no git submodule? cxxopts repo has 500kB size, but rapidjson is a little bigger with 30 MB

Tasks

• Update docs/API.md (remove if irrelevant)
• Update CHANGELOG.md (remove if irrelevant)
• review

[jcoupey]

Thanks for drafting this PR, I'd like to move on with this as part of the release after v1.11. On the questions you raised:

the heuristic arg is now showing on help, I guess that's mostly for you to debug?

Yep, that is really a debugging entry, that's why it is not actually documented as such. I guess we can always raise the question of optionally showing on the cxxopts repo, but no harm done if not possible.

maybe we can get rid of the CLArgs struct or even cl_args.h and pull the stuff into main.cpp

That's an option, not sure what is the most readable/explicit between having everything straight into the main, or on the other hand having a dedicated placeholder for everything related to user input. In any case, we can always change that as another step, so let's stick to a simple replacement of the current behavior in this PR.

no git submodule? cxxopts repo has 500kB size, but rapidjson is a little bigger with 30 MB

I agree versioning dependencies along with the core code for this repo is not ideal so the whole idea of submodules is appealing. On the other hand the current include/rapidjson folder is 652K so pulling 30MB sounds weird.
What if we introduce cxxopts as a submodule and evaluate the switch to submodules separately for other dependencies (polylineencoder and rapidjson)?

[nilsnolde]

jep, agree on all points. will do a git submodule and leave the rest for now. skimming over some issues in cxxopts, I found out how to hide certain options, will implement that for the heuristic arg.

[nilsnolde]

• added cxxopts as submodule
• hide debug args from output on vroom -h

for me this is done. no stress with a review. also needs some more testing, I just did example_2.json. would be good if you could eventually try this branch with your common arg combos @jcoupey

[jcoupey]

Thanks @nilsnolde. I can't really commit to reviewing soon but I definitely want to include this early in the next iteration so we have plenty of time to test from master and feature branches.

[nilsnolde]

yeah no worries, we're not depending on this, it has infinite time from our side:)

[jcoupey]

I didn't spend much time testing so far but went through the changes and left a few comments.

[nilsnolde]

@jcoupey addressed your comments for the most part. probably we don't get around turning off clang-format, but happy to see if you can verify/dismiss my thoughts locally.

[jcoupey]

The overall changes look OK from what I can tell but I get puzzling results when running $ vroom -i docs/example_1.json

I sometimes get:

[Error] VROOM compiled without libosrm installed.
{"code":3,"error":"VROOM compiled without libosrm installed."}

while the error used to be about not being able to reach osrm-routed (maybe a change in default router).

Also I sometimes randomly get a segfault for that same command instead of the above error.

[krypt-n]

Also I sometimes randomly get a segfault for that same command instead of the above error.

It appears that cl_args.router is never set if router_arg == "osrm" (the default) and thus contains uninitialized memory

[nilsnolde]

ok resolved the outstanding issues I hope. thanks @krypt-n , you're right of course.

github seems to have a bug.. I can't comment on half of your review comments @jcoupey , so:

• -l was a string before, I changed to unsigned, now garbage is properly handled
• improved the error reporting: cxxopts tells the user the argument which failed but no context, so I added the previous (before this PR) error message, now it's e.g. Argument ‘a’ failed to parse, invalid numerical value.
• cl_args.router gets a default osrm value
• I didn't quite understand the Up ;-) comment though.. above all other includes is what you'd like to see or?

I checked a few commands and it worked fine for me now, in terms of examples only example_2.json though. let me know if you find more problems.

[nilsnolde]

oh and still no formatting..

[jcoupey]

Thanks for the changes, I think the only remaining concern is about the -l option and default value.

Oh, and the changelog entry. ;-)

[nilsnolde]

@jcoupey should we have another round so we can merge this?

actually this is (kinda) blocking windows support in the python bindings as well (VROOM-Project/pyvroom#45). let me know if there's anything left to do and we can get it out of our heads:)

[jcoupey]

Sorry, I've been taken by other stuff, and also a bit lazy on that one. You're right, let's get this done.

I just did a round of command-line tests and everything seems OK, except for the debug -e flag:

$ vroom -i my_file.json -e "0,NONE,0.3"
[Error] Invalid heuristic parameter in command-line.
{"code":2,"error":"Invalid heuristic parameter in command-line."}

The same works on master. The exception comes from here so probably there is a problem with the string used as parameter.

[jcoupey]

Actually the exception is raised in str_to_heuristic_param because it only receives the string "0" from "0,NONE,0.3". It looks like heuristic_params_arg is populated as ["0", "NONE", "0.3"]. In this case there is a misunderstanding: the string passed to -e should be a single element of heuristic_params_arg. The rationale is that it's possible to pass several set of parameters using several occurrences of -e. For example using -e "0,NONE,0.3" -e "0,HIGHER_AMOUNT,0.4" should result in heuristic_params_arg = ["0,NONE,0.3", "0,HIGHER_AMOUNT,0.4"], then str_to_heuristic_param is called twice and is responsible for parsing the values for each of the strings.

[nilsnolde]

turns out that's a bit of a tricky case.. since cxxopts reserves commas to split arguments, e.g. --location berlin,paris will lead to args.location = {"berlin", "paris"} but at the same time supports having the same option multiple times, so the result is that --location berlin,paris --location london will be parsed to a flat vector {"berlin,paris,london"}, not {{"berlin","paris},{"london"}}.

I had to change cxxopts CXXOPTS_VECTOR_DELIMITER to smth else to overcome that. which also means vroom can't have comma-separated args to parse into a vector. or we change the separator for the heuristic_params_arg?

[jcoupey]

There is yet another problem with multiple occurrences of the same flag. Say I have an instance with both "profile": "car" and "profile": "bike" vehicles and I have two OSRM instances running locally for car (port 5014) and bike (port 5001). Passing various ports used to work but now result in an error mentioning the default port value:

$vroom -i example_1.json -p car:5014 -p bike:5001
[Error] Failed to connect to 0.0.0.0:5000
{"code":3,"error":"Failed to connect to 0.0.0.0:5000"}

The same kind of odd thing happens with -a. Say I have an OSRM server running on XX.XX.XX.XX for car and locally for bike, then:

• vroom -i example_1.json -a car:XX.XX.XX.XX -a bike:0.0.0.0 does not error but both requests are sent locally to the bike instance
• vroom -i example_1.json -a bike:0.0.0.0 -a car:XX.XX.XX.XX yields {"code":2,"error":"Invalid profile: bike."}

It looks like only the last flag occurrence is taken into account?

[nilsnolde]

Ah right totally never played around with the „new“ multi profile implementation. That’s right, it’s only one taken into account. I’ll change that. Is there any other flag that can be specified multiple times?

[jcoupey]

Is there any other flag that can be specified multiple times?

No, only -a, -p and the already covered -e.

[nilsnolde]

ok, that should've done it @jcoupey

[jcoupey]

Thanks @nilsnolde for your work on this! I just made a couple adjustments on the clang-format side and took the opportunity for a bit of help rephrasing.