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.

Sign up for GitHub

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

Jump to bottom

Fix the fuzz bug #11

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Fix the fuzz bug #11

merged 3 commits into from
Aug 22, 2024

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Aug 9, 2024 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

In 3c31ce7:

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
Copy link
Member Author

You were reviewing while I was hacking :)

@tcharding
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

Only miniscript was missing the use of rev (rust-bitcoin/rust-miniscript#720).

@tcharding
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member Author

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

Not done but I'll keep it in mind.

apoelstra
apoelstra approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK f56e456 though I still think we should stop special-casing

apoelstra
apoelstra approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK f56e456 though I still think we should stop special-casing fuzz

@tcharding
Ignore dynamic paths with shellcheck
274bdbb 
We source a bunch of dynamic paths, `shellcheck` cannot handle dynamic
paths - configure `shellcheck` to ignore the warnings.
@tcharding
Use say_err
97fba0c 
We have an error function, use it.

This clears a shellcheck warning (unseen locally by myself) and puts the
error message below the ouput from `cargo tree` which is probably better
if its long.
@tcharding
Stop treating fuzz as a special case
6646692 
We would like to run the build and tests for all crates in the
workspace. In order to do so clear the env vars from the last
loop (setting to the empty string) and check against the empty string
when running specific `cargo` incantations.
tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Aug 21, 2024
@tcharding
WIP: Set CRATES mechanically
06bb829 
Use shell to set the `CRATES` env var used by `run_task.sh`.

This patch needs a PR to merge into maintainer tools before it can be
used, we then need to set the commit hash in
`.github/workflows/rust.yml`.

rust-bitcoin/rust-bitcoin-maintainer-tools#11
@tcharding tcharding mentioned this pull request Aug 21, 2024
Merged
@tcharding tcharding mentioned this pull request Aug 21, 2024
Merged
apoelstra
apoelstra approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 6646692

@apoelstra apoelstra merged commit 0f235e0 into rust-bitcoin:master Aug 22, 2024
1 check passed
apoelstra
apoelstra reviewed Aug 22, 2024
View reviewed changes
ci/run_task.sh
@@ -146,7 +143,7 @@ do_test() {
$cargo build
$cargo test

if [ -n "${EXAMPLES+x}" ]; then
if [ -n "${EXAMPLES}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

In 6646692:

What were these +xs supposed to mean? AFAIK they just ensure that the -n check will always pass and the -z checks never will.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, and also I was annoyed at myself for writing syntax that I had to look up when I tried to read it. Hence I added the more brain-dead set the vars to "" in the loop.

@tcharding tcharding deleted the 08-10-fix-fuzz-bug branch August 29, 2024 01:50
tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Aug 29, 2024
@tcharding
WIP: Set CRATES mechanically
74403f7 
Use shell to set the `CRATES` env var used by `run_task.sh`.

This patch needs a PR to merge into maintainer tools before it can be
used, we then need to set the commit hash in
`.github/workflows/rust.yml`.

rust-bitcoin/rust-bitcoin-maintainer-tools#11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Any crate in contrib/crates.sh after fuzz is ignored
2 participants
@tcharding @apoelstra