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

Swap blake3 to FetchContent + general cleanup #402

Merged
merged 11 commits into from
Oct 21, 2023
Merged

Conversation

cmmarslender
Copy link
Contributor

@cmmarslender cmmarslender commented Oct 13, 2023

  • Gets blake3 via fetchcontent in cmake
  • Builds windows wheels with cmake as well, vs the old split method used in setup.py
  • re-enables the tests on windows which were disabled when bladebit compress harvesting was merged
  • Fix edge case in C++ tests on mac and add mac C++ tests to CI

@cmmarslender cmmarslender changed the title Swap blake3 to FetchContent Swap blake3 to FetchContent + general cleanup Oct 21, 2023
@cmmarslender cmmarslender marked this pull request as ready for review October 21, 2023 02:55
Copy link
Member

@hoffmang9 hoffmang9 left a comment

Choose a reason for hiding this comment

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

My only question is - now that we're not specifying the assembly versions, will Windows still get the CPU optimized code?

@cmmarslender
Copy link
Contributor Author

cmmarslender commented Oct 21, 2023

My only question is - now that we're not specifying the assembly versions, will Windows still get the CPU optimized code?

it appears they handle this all on their end now (they added cmake config since we initially pulled in the files).
For instance: https://github.com/BLAKE3-team/BLAKE3/blob/master/c/CMakeLists.txt#L74:L81

As another example, they have this output that shows up when assembly is enabled: https://github.com/BLAKE3-team/BLAKE3/blob/master/c/CMakeLists.txt#L180

Which we can see in our windows build here: https://github.com/Chia-Network/chiapos/actions/runs/6594714279/job/17918903037?pr=402#step:3:40

Can also see the NEON stuff enabled automatically in the ARM builds: https://github.com/Chia-Network/chiapos/actions/runs/6594714283/job/17918903167?pr=402#step:8:285

Copy link
Member

@hoffmang9 hoffmang9 left a comment

Choose a reason for hiding this comment

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

I now remember Amine making those changes.

@cmmarslender cmmarslender merged commit df636fe into main Oct 21, 2023
31 checks passed
@cmmarslender cmmarslender deleted the blake3-fetch-content branch October 21, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants