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

Refactor CMake buildsystem to be portable and modern #304

Merged

Conversation

BurningEnlightenment
Copy link
Collaborator

@BurningEnlightenment BurningEnlightenment commented May 11, 2023

This is building on top of @SteveGremory's #247
Resolves #102

Aggreggate source files directly in the target instead of a proxy variable.

Install CMake package config files in order to allow the project to be found via find_package() by dependents.

Remove hard coded compiler flags (including -fPIC). These are not portable and should be set by the toolchain file or on the CLI.

Replace hard coded SIMD compiler flags with configurable options. Retain the current GCC/Clang flags as defaults for these compilers. Add default SIMD compiler flags for MSVC.

Guard ASM sources with triplet compatibility checks.

Remove the BLAKE3_STATIC option in favor of BUILD_SHARED_LIBS.

Aggreggate source files directly in the target instead of a proxy
variable.

Install CMake package config files in order to allow the project to be
found via `find_package()` by dependents.

Replace hard coded SIMD compiler flags with configurable options. Retain
the current GCC/Clang flags as defaults for these compilers. Add default
SIMD compiler flags for MSVC.

Remove hard coded compiler flags (including -fPIC). These are not
portable and should be set by the toolchain file or on the CLI.

- Guard ASM sources with triplet compatibility checks.
- Remove the `BLAKE3_STATIC` option in favor of [`BUILD_SHARED_LIBS`].

[`BUILD_SHARED_LIBS`]: https://cmake.org/cmake/help/v3.9/variable/BUILD_SHARED_LIBS.html
In order for blake3 to be usable as a shared library on Windows it is
required to annotate public symbols. Use this as an opportunity to prune
the symbol table for other OSes, too.
c/CMakeLists.txt Outdated Show resolved Hide resolved
c/CMakeLists.txt Outdated Show resolved Hide resolved
oconnor663 and others added 2 commits May 11, 2023 22:11
Co-authored-by: Henrik Gaßmann <BurningEnlightenment@users.noreply.github.com>
Co-authored-by: Henrik Gaßmann <BurningEnlightenment@users.noreply.github.com>
size_t out_len);
BLAKE3_API void blake3_hasher_finalize_seek(const blake3_hasher *self, uint64_t seek,
uint8_t *out, size_t out_len);
BLAKE3_API void blake3_hasher_reset(blake3_hasher *self);
Copy link
Member

Choose a reason for hiding this comment

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

Could you say more about what these macros are doing? I'm a little nervous about leaking complexity into the header files that might affect folks who aren't using CMake to build this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These macros control which symbols (=> C functions / global variables) end up in the shared object's (.so) symbol table, i.e. what you can access via dlsym(). The default on gcc and clang is to export all entities with external linkage while Windows / MSVC defaults to exporting only entities annotated with __declspec(dllexport). Without these annotations you basically can't use the blake3.dll. So these aren't actually CMake specific; they are useful regardless of the buildsystem. You can see a similar macro used by libb2 here:
https://github.com/BLAKE2/libb2/blame/6fafe5c546860ecbdd2b97093a93ae4ce54dfe52/src/blake2.h#L20-L44

You can read more about symbol visibility on the GCC Wiki and the MSVC docs.

Also note that I configured CMake to set the default visibility to hidden for GCC / clang.

@oconnor663
Copy link
Member

Thanks for putting this together! I'm going to learn a lot reading through it :)

@BurningEnlightenment
Copy link
Collaborator Author

@oconnor663 do you have further questions or can we get this merged?

@oconnor663 oconnor663 merged commit ef5679e into BLAKE3-team:master May 23, 2023
33 checks passed
@oconnor663
Copy link
Member

Merged! Thank you! I'll admit I still have no idea what I'm doing, and I only kicked the tires a little bit, but it makes me really happy that this works:

cmake .
make
gcc example.c libblake.a

I guess the next question is how we should document this? :)

@BurningEnlightenment
Copy link
Collaborator Author

I can write a section for the c/README.md tomorrow or so.

On a related note I'm going to update the vcpkg port based on this; maybe someone else does the work for conan.

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.

add a CMake build system for the C code?
2 participants