Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Fuzzer Migration Follow-ups#1903
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fuzzer Migration Follow-ups#1903
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Prefer executing these files using the OSS-Fuzz or `python` commandmethods outlined in the `fuzzing/README`.Based on feedback and discussion on:gitpython-developers#1901
This script is meant to be sourced by the OSS-Fuzz file of the samename, rather than executed directly. The shebang may lead to theincorrect assumption that the script is meant for direct execution.Replacing it with this directive instructs ShellCheck to treatthe script as a Bash script, regardless of how it is executed.Based@EliahKagan's suggestion and feedback on:gitpython-developers#1901
This script is executed directly, not sourced as is the case with`build.sh`, so it should have an executable bit set to avoid ambiguity.Based@EliahKagan's suggestion and feedback on:gitpython-developers#1901
- Make the link text for the OSS-Fuzz test status URL more descriptive- Fix capitalization of GitPython repository nameBased@EliahKagan's suggestion and feedback on:gitpython-developers#1901
Replaces the null character delimiter `-d $'\0'` with the simplerempty string `-d ''` in the fuzzing harness build loop.This changes leverages the Bash `read` builtin behavior to avoidunnecessary complexity and improving script readability.Based@EliahKagan's suggestion and feedback on:gitpython-developers#1901
Based@EliahKagan's suggestion and feedback on:gitpython-developers#1901
A misspelling in thehttps://github.com/gitpython-developers/qa-assetsrepository is still present here. It will need to be fixed in thatrepository first."corpora" is a difficult word to spell consistently I guess. This madefor a good opportunity to improve the phrasing of two other comments atat least.Based@EliahKagan's suggestion and feedback on:gitpython-developers#1901
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks good.
I wonder ifcorpra should be renamed tocorporain that path as well, or if that would cause problems.
If and only if that is changed, this line would then need to be changed accordingly:
| rsync -avc qa-assets/gitpython/corpra/"$SEED_DATA_DIR/"&& |
Unrelated to that, there is a tiny style nit commented on below (which you may even decide to leave as-is).
Uh oh!
There was an error while loading.Please reload this page.
DaveLak commentedApr 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It definitely should but among everything else related to the fuzzing work, updating that repo feels like the lowest priority. Especially considering right now it works exactly as we need it to, I've opted to defer any changes for now. After the outstanding change requests are addressed, I would like (and plan) to revisithttps://github.com/gitpython-developers/qa-assets. I think it would benefit from a structure similar tohttps://github.com/bitcoin-core/qa-assets. Specifically:
I'm happy to discuss further if you would like but that feels like it would be best in a discussion thread (or an issue onhttps://github.com/gitpython-developers/qa-assets, but I'm not sure there is a desire to turn issues on there.) |
Also makes come structural improvements to how the local instructionsfor running OSS-Fuzz are presented now that only the single `address`sanitizer is a valid option.The `undefined` sanitizer was removed from GitPython's `project.yaml`OSS-Fuzz configuration file at the request of OSS-Fuzz project reviewersingoogle/oss-fuzz#11803.The `undefined` sanitizer is only useful in Python projects that usenative exstensions (such as C, C++, Rust, ect.), which GitPython doesnot currently do. This commit updates the `fuzzing/README` reference tothat sanitizer accoirdingly.See:-google/oss-fuzz@b210fb2-google/oss-fuzz#11803 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot for this follow-up and for sharing your plan of the additional improvements.
Not using zip blobs stored in theqa-assets repository is my favorite :).
00bc707 intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
The upstream GitPython repository merged a change that set theexecutable bit on `container-environment-bootstrap.sh` in Git, so the`chmod` is no longer needed and the `RUN` instruction can execute thescript directly.See:-gitpython-developers/GitPython#1903-gitpython-developers/GitPython@b0a5b8e
Uh oh!
There was an error while loading.Please reload this page.
This PR addresses most unresolved review comments from#1901. It also updates the README
Addressed in This PR
build.shwith ShellCheck directive (Comment)fuzzing/oss-fuzz-scripts/container-environment-bootstrap.shto mark it executable in Git (Comment)fuzzing/README.mdfor the OSS-Fuzz test status URL more descriptive (Comment)fuzzing/README.md(Comment)build.sh's build fuzz harness loop (Comment)fuzzing/oss-fuzz-scripts/container-environment-bootstrap.shfor consistent script formatting (Comment)Misc
undefinedas a valid option ingoogle/oss-fuzz@b210fb2(#11803) (Context:Comment in OSS-Fuzz PR #11803)TODO / Pending Further Discussion
I felt that these items are better addressed in separate PRs.
Add GitPython's standard license header comments to non-Apache 2.0 licensed files (Comment)Done inAdd GitPython's Standard License Header Comments to Shell Scripts #1907fuzzing/fuzz-targets/fuzz_tree.pyrefactoring (Comment 1 &Comment 2)$SRC,$OUT, &$WORK) infuzzing/oss-fuzz-scripts(Comment)