-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[gitpython]: Move Fuzz Tests & Configuration Upstream #11803
Conversation
The pre-installed version of `pip` (19.x) was outdated and unable to parse the `pyproject.toml` syntax during the install step in `build.sh` causing the script to error out and crash. Upgrading `pip` to the latest version in the project image resolves the issue and allows the installation to complete. [1]: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67399
Updates the gitpython project files to enable migrating and maintaining fuzz targets and build scripts upstream. Related PR in the upstream repo: gitpython-developers/GitPython#1901
DaveLak is a new contributor to projects/gitpython. The PR must be approved by known contributors before it can be merged. |
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.
Thank you!
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.
LGTM
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.
Looks good! Do we need for gitpython-developers/GitPython#1901 to be merged before the CI in this one can pass?
That is correct. I am going to push one more change there to address @EliahKagan's very helpful feedback on ensuring the two fuzz harnesses we are moving conform to the requirements of the Apache license from this repo, then that should good to merge. I'll ping you here once we've got it into |
@DavidKorczynski upstream was merged and CI is passing! |
projects/gitpython/project.yaml
Outdated
vendor_ccs: | ||
- [email protected] | ||
- address | ||
- undefined |
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.
Nit: does gitpython
have any native code being compiled with sanitizers from OSS-Fuzz, or is it pure Python? If it's pure Python, then please remove undefined
from the list here
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.
I believe Gitpython is pure python code, no native extensions. I'll update this shortly.
CC: @Byron @EliahKagan
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.
Yes, that's correct. GitPython is pure Python with no native extensions.
(Its dependencies gitdb and smmap are likewise pure Python. The gitdb dependency has in the past been possible to attempt to make faster through the use of gitdb-speedups, which is a native code module. That project is archived and no longer developed or recommended, and it's not known if, or how well, that works anymore. Furthermore, it speeds up functionality of gitdb that is no longer widely used in GitPython and itself no longer recommended, since today the main significance of gitdb and smmap are helper types, rather than the actual sliding-window memory manager and in-memory database, which have not been used by default in GitPython for quite some time.)
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, @EliahKagan! I'll update this PR to remove undefined
from this list.
FWIW, gitdb
is integrated with this project as well (see
https://introspector.oss-fuzz.com/project-profile?project=gitdb) and might be worth upstreaming in a similar fashion, but we can talk about that elsewhere.
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.
Done 👍
GitPython is a pure python code base and does not include any native extensions, so the `undefined` sanitizer is unnecessary.
Also makes come structural improvements to how the local instructions for 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 reviewers in google/oss-fuzz#11803. The `undefined` sanitizer is only useful in Python projects that use native exstensions (such as C, C++, Rust, ect.), which GitPython does not currently do. This commit updates the `fuzzing/README` reference to that sanitizer accoirdingly. See: - google/oss-fuzz@b210fb2 - google/oss-fuzz#11803 (comment)
The upstream GitPython repository merged a change that set the executable bit on `container-environment-bootstrap.sh` in Git, so the `chmod` is no longer needed and the `RUN` instruction can execute the script directly. See: - gitpython-developers/GitPython#1903 - gitpython-developers/GitPython@b0a5b8e
Updates the gitpython project files to enable migrating and maintaining fuzz targets and build scripts upstream.
Related PR in the upstream repo: gitpython-developers/GitPython#1901
project.yaml
updates:auto_ccs
list as discussed with @Byron here: Continuing the OSS-Fuzz Integration Conversation gitpython-developers/GitPython#1889 (comment)vendor_ccs
because it looked like you were included as the default when no other contacts were listed. If this was a mistake on my part and you want to remain listed as a CC, please let me know and I'll correct it.Thanks!