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

Added CMake support #247

Closed
wants to merge 7 commits into from
Closed

Conversation

SteveGremory
Copy link
Contributor

I needed to use BLAKE3 as a CMake submodule and luckily someone had done the work for me.

This PR makes BLAKE3 usable as a CMake submodule and helps with rather easy compilation of the library across platforms too.

c/CMakeLists.txt Show resolved Hide resolved
c/CMakeLists.txt Outdated Show resolved Hide resolved
@oconnor663
Copy link
Member

Hmm, is this some weird GitHub Actions limitation on CI changes? Lemme make a branch of my own and push this.

@oconnor663
Copy link
Member

Ah no I see the problem. The build matrix for the new jobs doesn't have a target field, so you need to do this:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index dec49e2..3ed564b 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -236,8 +236,8 @@ jobs:

   # CMake build test (Library only), current macOS/Linux only.
   cmake_build:
-      name: CMake ${{ matrix.target.os }}
-      runs-on: ${{ matrix.target.os }}
+      name: CMake ${{ matrix.os }}
+      runs-on: ${{ matrix.os }}
       strategy:
         fail-fast: false
         matrix:

Very frustrating that Actions handles this error by silently dropping the jobs...

@SteveGremory
Copy link
Contributor Author

made the changes, perhaps it should work now?

@oconnor663
Copy link
Member

(I tried to resolve a merge conflict by clicking a button here on GitHub, and I'm really not sure I did it right...)

Copy link
Contributor Author

@SteveGremory SteveGremory left a comment

Choose a reason for hiding this comment

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

Figured it out, I had to send it for a re-review (makes sense) so if @oconnor663 approves of it now, it will probably be merge-able. Sorry that I kind of deserted the PR after a while 😔

@BurningEnlightenment
Copy link
Collaborator

BurningEnlightenment commented Apr 29, 2023

I would like to propose a few changes (less flag hardcoding, symbol visibility macros, target_sources() instead of accumulating sources in a macro, windows/msvc compat) to the CMake buildsystem. But before I invest time I'd like to know, whether the blake3 team is generally interested in adding CMake build files.

@oconnor663
Copy link
Member

I've rebased this branch and manually landed it on master. Still don't understand how reviews work but now I don't have to :) Thanks @SteveGremory!

@BurningEnlightenment I'd love more PRs to improve things. I'm not sure what the modern CMake idioms are. Also Windows compat sounds great if you have the interest.

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.

None yet

3 participants