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.

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

Add openssl/fuzz-corpora as submodule #20683

Closed
wants to merge 5 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Apr 5, 2023

Instead of carrying the fuzz corpora data in the source code repository we move it into a submodule pulled from https://github.com/openssl/fuzz-corpora

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: refactor The issue/pr requests/implements refactoring tests: exempted The PR is exempt from requirements for testing labels Apr 5, 2023
@t8m t8m mentioned this pull request Apr 5, 2023
Otherwise the fuzz/corpora won't be present.
@levitte levitte removed the approval: otc review pending This pull request needs review by an OTC member label Apr 5, 2023
@t-j-h t-j-h added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 5, 2023
@t8m
Copy link
Member Author

t8m commented Apr 5, 2023

Fixup was needed - we do not want submodules for check update and check docs CI runs.

@t8m
Copy link
Member Author

t8m commented Apr 5, 2023

@levitte @t-j-h please reconfirm

@t-j-h
Copy link
Member

t-j-h commented Apr 5, 2023

Reconfirmed.

@t8m
Copy link
Member Author

t8m commented Apr 5, 2023

Additional question - do we want to move the fuzz/corpora data to submodule also in 3.1 and 3.0 branches?

@t-j-h
Copy link
Member

t-j-h commented Apr 5, 2023

I would leave stable branches alone for this sort of stuff - but I won't put a hold if others think differently.

@t8m
Copy link
Member Author

t8m commented Apr 5, 2023

I would leave stable branches alone for this sort of stuff - but I won't put a hold if others think differently.

Yeah, unless we would like to add more testing data to older branches we can keep them as is.

@levitte
Copy link
Member

levitte commented Apr 5, 2023

I would actually do this same thing on the release branches too, simply to avoid all the duplication. Stuff in new subdirectories will be ignored by them, as they won't have the corresponding fuzz testing driver.

But, I don't see a need to do this nownownow.

@slontis
Copy link
Member

slontis commented Apr 5, 2023

Just an observation, looking at some other builds this seems to add a few minutes of build time to some of the builds (e.g basic_gcc) (i.e. we are now always retrieving 10 submodules)

@paulidale
Copy link
Contributor

Any chance of limiting the submodules to just the fuzzer?
I don't see an option to do this in the checkout action 😞

@t8m
Copy link
Member Author

t8m commented Apr 6, 2023

Any chance of limiting the submodules to just the fuzzer? I don't see an option to do this in the checkout action disappointed

Yeah, there is no such option. I assume we would have to call git explicitly.

@paulidale
Copy link
Contributor

Assuming the last change works 😁

@t8m
Copy link
Member Author

t8m commented Apr 6, 2023

Assuming the last change works grin

Yeah, it seems to.

@levitte, @t-j-h please reconfirm

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@kroeckx kroeckx added approval: done This pull request has the required number of approvals and removed approval: done This pull request has the required number of approvals labels Apr 10, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 11, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member Author

t8m commented Apr 11, 2023

Merged to master branch. Thank you for the reviews.

@t8m t8m closed this Apr 11, 2023
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
To be replaced with a git submodule.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20683)
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20683)
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
Otherwise the fuzz/corpora won't be present.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants