Fix the fuzz bug - Githubissues

rust-bitcoin / rust-bitcoin-maintainer-tools

Scripts, test vectors, and other things used by or across multiple repositories in the rust-bitcoin ecosystem.

Creative Commons Zero v1.0 Universal

5 stars 5 forks source link

Fix the fuzz bug #11

Closed tcharding closed 2 months ago

tcharding commented 2 months ago

This PR dose a few things:

Clear shellhcheck warnings due to dynamic file paths (not sure why this passed CI before)
Use say_err to clear another shellcheck warning (that I was unable to repro locally)
Runs cargo build and cargo test for all crates instead of treating fuzz as a special case

Resolve: #10

tcharding commented 2 months ago

We need to be careful before merging this one. Its a good chance to check all our repos that use this tool to see that they either:

Use a specific commit hash so we can merge
Have no bugs so we can increase coverage without breaking

apoelstra commented 2 months ago

If anything is not using a specific commit hash then we need to change them to do so. We can't paralyze this repo by having its master branch automatically deployed to a dozen other projects.

apoelstra commented 2 months ago

In 3c31ce7794d0b12fa3c2a54edbf43e193ea7ff80:

This fix is fine though I think we should just stop treating the name fuzz specially. I don't treat it specially in my local CI setup. I look for honggfuzz deps and run cargo hfuzz on all binaries in crates that they appear (and recently, I even choose the correct version of honggfuzz based on the lockfile).

CI here is failing. I think we should fix it.

tcharding commented 2 months ago

You were reviewing while I was hacking :)

tcharding commented 2 months ago

If anything is not using a specific commit hash then we need to change them to do so. We can't paralyze this repo by having its master branch automatically deployed to a dozen other projects.

Agreed, I'll check the users to make sure this is the case.

tcharding commented 2 months ago

I'm going to come back to this, its more important to check all the users. (And I'm confused why shellcheck -x foo is warning, that is a sign to come back with a fresh brain.

tcharding commented 2 months ago

Only miniscript was missing the use of rev (https://github.com/rust-bitcoin/rust-miniscript/pull/720).

tcharding commented 2 months ago

This fix is fine though I think we should just stop treating the name fuzz specially.

I had a go doing this but if we treat it like other crates it requires a contrib/test_vars.sh file which seems clunky.

tcharding commented 2 months ago

I'm not sure what is going on with the CI job, shellcheck ci/run_task.sh runs cleanly for me locally. Can you check on your machine please @apoelstra

apoelstra commented 2 months ago

I had a go doing this but if we treat it like other crates it requires a contrib/test_vars.sh file which seems clunky.

Doing this would correctly capture the fact that some of our fuzz harnesses still have honggfuzz features while others don't.

But in any case, it seems less clunky to me to have a contrib/test_vars.sh than to have a particular workspace which we special-case based on its directory name, and don't even try to run the unit tests on.

apoelstra commented 2 months ago

@tcharding when I run shellcheck locally the error appears. It is complaining that the say_err function is defined but never called.

We probably want to globally whitelist "unused code" type warnings on this repo.

tcharding commented 2 months ago

Cheers, I found a place that we were using echo to print an error message so I used say_err. I'm still unsure why my shellcheck was not showing the warning.

tcharding commented 2 months ago

We probably want to globally whitelist "unused code" type warnings on this repo.

Not done but I'll keep it in mind.