-
Notifications
You must be signed in to change notification settings - Fork 40
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 docopt, anyhow, fs-err, which crates to simplify and st… #51
Conversation
Works |
4b94979
to
6ead92d
Compare
Not sure what causes the linkage error |
This is ready for review :) |
3cdb107
to
247435a
Compare
Hello @drahnr,
|
Mostly went for it since I am in the same situation with
In that case, an explicit encoding with
I disagree.
✔️
Err, I'll fix that right away. EDIT: I thought I fixed that, either way, postponed, see below.
I tried to exclude most of these, but it seems I missed a few.
Yeah, I think that happend during a rebase. Going to split this into smaller atomic PRs then. |
I don't get it, why not using
I was thinking about the
Indeed but this part of the project is not a library and is basically a small script file with an easy stop-when-there-is-an error approach to error handling. I don't feel it is needed to formalize too much error paths. There is little to gain but code complexity.
I don't feel that way. Adding dependencies is always a liability too:
I would love if the
That would be greatly appreciated, thx :) |
4c4054f
to
f343ff2
Compare
It's a CLI wrapper, as such adding a few proven and 1.x versions don't imply having to chase the latest and greatest. If you can cleanup code and make it clearer and DRYer, I personally always prefer a crate or macro. But that might be just me 🤷
Point taken.
Is that really relevant here? It's a CLI wrapper tool of a fuzzer - exposure is really minimal :)
Build time is a factor, but the dimensions here are really tiny, and as said, it's a fuzzer. If you want to avoid the dependencies for the lib splitting it into two indepedent crates can cut down the compilation time for the lib (which is what's going to be run repeatedly, rather than the binary wrapper) would yield some gain without limiting oneself on the crates to be used.
Puh, that's a broad statement and I disagree. A single occurence of missing permission and being faced without a path already is worth the dependency.
Not saying that at all, I am just looking at the gain/pain ratio and that's (imho) pretty good for
Split it into a bunch of atomic commits, the change of Happy weekend, and thanks for you time reviewing this! |
--exitCodeUponCrash was introduced
I'll replace the |
I haven't had the time to look at your other commits yet but I can tell you more about my "philosophy" and plans about the CLI. My plan was to change from this HFUZZ_RUN_ARGS="-v -N 10000000 --run_time 120 --exit_upon_crash" cargo hfuzz run example to this cargo hfuzz run example -- -v -N 10000000 --run_time 120 --exit_upon_crash where the delimiter would be |
Aware, and this is a rather annoying property of the wrapper right now, since every usage of
I fully agree. That's what the I split this into two PRs, #54 and #53 without docopt since you are already working on the |
…ructure
This remedies some of the internal TODOs and yields better error messages.
Motivation is, to allow additional verifyable arguments to enhance practical usability without having to re-read various help texts on which param gets passed to which process.