Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 BLAKE3 hash #13194
base: master
Are you sure you want to change the base?
add BLAKE3 hash #13194
Changes from 10 commits
187c8e9
14cd419
fcc16d3
bf8a527
6634154
6f40213
882466b
61f4b26
012d660
de9e7e9
6693562
ab2229e
2a040f7
9439644
d704780
f12caf0
1954b37
9504fa1
2a2b5ad
06e5a75
ad9c31e
09a34ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
just a personal opinion, but pcre2 nor opcache's JIT code are cloned. I do not see a particular reason to make an exception.
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.
@devnexen this script makes it easy to see exactly how blake3 1.5.0 was cloned, and should make it easy to update to future versions (like 1.6.0) in the future - IMO it's worthwhile to keep this script. idk if it should be part of
.github/actions/verify-generated-files/action.yml
tho (there's a discussion on that topic here #13194 (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.
I do not see the appeal nor the necessity. Couple of counter arguments from the top of my head :
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.
@divinity76 The point of
verify-generated-files
is to avoid sneaking malicious code into generated files that are collapsed during code reviews that people don't generally look at. This risk doesn't exist here. IMO, if the intent is to avoid people mistakenly editing these files, the same issue already exists for all other copied projects, and should be solved universally (if at all).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.
@iluuu1994
Interesting, thanks for the explanation!
Maybe I snuck some malicious code into one of the upstream_blake3/* files? If I did, this script would actually revert it, and then
verify-generated-files
would notice it!@devnexen
My main appeal is that if I want to update the bundled blake3 in the future, i can just change the 1 line
git clone --branch '1.5.0'
withgit clone --branch '1.5.1'
and the script should do the rest.fair
How does upstream pcre2 fixes get applied to the internal pcre2 library? i don't know
There's good updates sometimes, for example the latest v1.5.0 fixed a arm big-endian compile issue, v1.4.1 increased ARM performance by ~10-30%, 1.3.3 fixed a GCC6.1 AVX512 debug corruption issue.
Yeah that's a good point. If we get to that point, this script is useless. Anecdotal evidence, the only time I made a PR to BLAKE3, it was accepted in 11 hours ( BLAKE3-team/BLAKE3#131 ), but it was a trivial one.
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.
Those are valid reasons to upgrade but again upgrades can be handled in same manner at the aforementioned php components. Note that I m not against having blake3 feature, I see it as a "nice to have", just the way of the dependency being handled.