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 BLAKE3 source code to third_party #18682

Closed
wants to merge 1 commit into from

Conversation

tylerwilliams
Copy link
Contributor

Add BLAKE3 source code to third_party

This PR adds the BLAKE3 C and asm sources to third_party, and includes a BUILD
file to build them.

This is a partial commit for #18658.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jun 14, 2023
@sgowroji sgowroji added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jun 15, 2023
"//src/conditions:darwin_arm64": [
"src/c/blake3_neon.c",
],
"//conditions:default": [],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with BLAKE3's source code. Are these assembly code optional so that when targeting other platforms the code can still be compiled but is just slower? I guess the copts below answers my question but just want to double check.

cc @meteorcloudy for potential caring when targeting other platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you got it. The blake3_portable.c implementation is a safe fallback for architectures which do not support the vectorized ASM implementations.

@meteorcloudy
Copy link
Member

I strongly recommend to introduce new external dependencies as external repositories instead of checking in the third_party directory.

@coeuvre
Copy link
Member

coeuvre commented Jun 15, 2023

@meteorcloudy Do we have docs/examples on how to do that?

@meteorcloudy
Copy link
Member

meteorcloudy commented Jun 15, 2023

Yes, you can add a dependency in https://github.com/bazelbuild/bazel/blob/master/distdir_deps.bzl#L120 and put a dist_http_archive in the WORKSPACE file:

bazel/WORKSPACE

Line 326 in e382abf

dist_http_archive(

Also, it'll be nice this could be submit to the BCR so that it's also available in Bzlmod.

@coeuvre
Copy link
Member

coeuvre commented Jun 15, 2023

@tylerwilliams Mind looking at what Yun suggested?

@tylerwilliams
Copy link
Contributor Author

@tylerwilliams Mind looking at what Yun suggested?

Done. I will send a separate CL to BCR to add this over there as well (but don't want to block the rest of the changes).

@tylerwilliams
Copy link
Contributor Author

Yes, you can add a dependency in https://github.com/bazelbuild/bazel/blob/master/distdir_deps.bzl#L120 and put a dist_http_archive in the WORKSPACE file:

bazel/WORKSPACE

Line 326 in e382abf

dist_http_archive(

Also, it'll be nice this could be submit to the BCR so that it's also available in Bzlmod.

@meteorcloudy I sent that CL here: bazelbuild/bazel-central-registry#698

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jun 16, 2023
@meteorcloudy
Copy link
Member

@sgowroji You can import the third_party change first, then the non-third_party change. I think the import scripts will work without having to split this PR.

@sgowroji
Copy link
Member

Hi @tylerwilliams, Could you please squash the commits in to one, So that i can initiate the import. Thanks!

@tylerwilliams
Copy link
Contributor Author

Hi @tylerwilliams, Could you please squash the commits in to one, So that i can initiate the import. Thanks!

@sgowroji Sure, done!

copybara-service bot pushed a commit that referenced this pull request Jun 18, 2023
Partial commit for third_party/*, see #18682.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
@sgowroji
Copy link
Member

Third party changes merged at 0ff4f6d

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jun 19, 2023
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Jul 25, 2023
Partial commit for third_party/*, see bazelbuild#18682.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
(cherry picked from commit 0ff4f6d)
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Jul 25, 2023
Add BLAKE3 source code to third_party

This PR adds the BLAKE3 C and asm sources to third_party, and includes a BUILD
file to build them.

This is a partial commit for bazelbuild#18658.

Closes bazelbuild#18682.

PiperOrigin-RevId: 541539341
Change-Id: I49b1edce20a7d0f986e29712e6050e4e0b9c1d44
(cherry picked from commit a3a569e)
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants