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 CMake build files #2

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

BurningEnlightenment
Copy link
Contributor

I ported my CMake build scripts from BurningEnlightenment/libb2-cmake.

It supports building the library with all code paths or with reference sources only with MSVC, MinGW and Clang (tested only on Windows), although it currently lacks OpenMP support on Clang. Furthermore it can build the test drivers.

However, the main feature is the install target which allows the library to be used from other CMake projects (especially on Windows) very easily.

The SSE compiler feature detection was always flaky and would need to be
maintained in spite of the fact that it doesn't add much value as the
compilation would just fail if the user incorrectly assumes that his
compiler supports the required instruction sets.

- Enable cmake test support.
@sneves
Copy link
Member

sneves commented Oct 10, 2017

I definitely appreciate the CMake addition, but this brings also a few other additions that are ABI-breaking. This is basically the suggestion over at BLAKE2/BLAKE2#40 that I agree with, but have been putting off precisely because of it potentially breaking things.

So, this would definitely need to also bump the .so version up in the autoconf and CMake configuration files.

@BurningEnlightenment
Copy link
Contributor Author

@sneves oh, I'm deeply sorry; I made those changes for internal usage and hadn't had this pull request in mind when I pushed them. How do you want to proceed?

@willbryant
Copy link

Was any decision made here? Maybe cut this PR down to just the build system stuff with no internal changes?

@willbryant
Copy link

Also, I don't understand the comment on the "Remove SSE compiler feature detection" commit. Why was it flaky? Does removing it mean that compilation will fail on platforms that don't have those x86 extensions?

@BurningEnlightenment
Copy link
Contributor Author

Was any decision made here? Maybe cut this PR down to just the build system stuff with no internal changes?

Well, it hasn't been merged since 2015 and extrapolating from that I don't think it'll happen until a maintainer needs it.
Splitting the code and build system changes is easy (separate source & build system commits), but I do need blake2x and therefore would only invest time if the blake2 team indicated interest in merging this.

Also, I don't understand the comment on the "Remove SSE compiler feature detection" commit. Why was it flaky? Does removing it mean that compilation will fail on platforms that don't have those x86 extensions?

The approach I've taken back then was to grep the sources for intrinsics and add compile tests with these intrinsics. Obviously this has to be kept in sync manually which I consider an unnecessary burden (usually either all or none of those sse intrinsics are supported by compilers/instruction set). Instead I just offer an option to completely exclude the dispatch logic and SSE code.

@haubi
Copy link
Contributor

haubi commented Jun 5, 2019

Please continue using libtool, because CMake does not support, or is broken on some target platforms where libtool does work - namely AIX for example.

@BurningEnlightenment BurningEnlightenment changed the title ADD CMAKE BUILD FILES Add CMake build files Jun 15, 2019
@emmenlau
Copy link

emmenlau commented Aug 8, 2019

Can I kindly bump up this PR again? Its really challenging to build libb2 anywhere outside of GNU/Linux. cmake is widely used in the C/C++ world, and I think it would make it easier for people to adopt blake2.
Can this PR kindly be brought up to date and reconsidered?

@noloader
Copy link
Contributor

noloader commented Jul 1, 2020

I can't speak for Samuel and the others, but adding CMake to contrib/cmake should be OK as long as the README states CMake is officially unsupported.

The minimum requirements of CMake 3.0 is too high. The CMake build will fail on a lot of platforms, including Ubuntu LTS releases, AIX, some BSDs, Solaris, and OS X. That's just going to create bug reports. It would probably be better to drop the minimum to CMake 2.8.

OpenMP support has a lot of gaps. Here's a more complete list of OpenMP options from different compilers: ax_openmp.m4. That file actually has a bug - SunCC needs -xopenmp=parallel in release builds, not -xopenmp.

The reason to classify CMake as unsupported... What I found in other projects is, the CMake fans rush in and ask for CMake. After it is added, the fans leave and the bugs must be worked by the project maintainers. CMake accounted for 28% of our bugs so we eventually dropped CMake support. It had too many problems and was taking up too much of our time.

@lnjX
Copy link
Contributor

lnjX commented Jul 6, 2020

The minimum requirements of CMake 3.0 is too high. The CMake build will fail on a lot of platforms, including Ubuntu LTS releases, AIX, some BSDs, Solaris, and OS X. That's just going to create bug reports. It would probably be better to drop the minimum to CMake 2.8.

I'm not sure whether this is really a problem, especially if you say that CMake should be "officially unsupported". Also, the oldest release of Ubuntu that has still standard LTS support is 16.04 which has cmake 3.5. Only 14.04 with its extended security support still uses cmake 2.8.

@BurningEnlightenment
Copy link
Contributor Author

It would probably be better to drop the minimum to CMake 2.8.

I politely disagree; on the contrary I recommend requiring at least CMake 3.1 which introduced target_sources(). The build files currently present in this PR exhibit some archaic patterns like a custom BLAKE2_SHARED_OBJECT option, modifying CMAKE_C_FLAGS , non idiomatic install paths, accumulating BLAKE2_IMPL_SOURCES instead of using target_sources(), and some other pre-modern CMake era stuff. "Modern" CMake build files are much more declarative and therefore easier to digest and maintain than what is doable with CMake 2.8. In addition to that CMake 2.8 can't export targets/generate import target files.

That said, I also can't think of a system which could neither use autotools nor cmake 3.1, but would be able to use cmake 2.8.

@xantares
Copy link

xantares commented Sep 10, 2020

hello, could you rebase on current master ?
cmake 3.1 is very reasonable these days (oldest ubuntu LTS currently supported, 16.04, has 3.5)

@lnjX lnjX mentioned this pull request Jun 5, 2021
@emmenlau
Copy link

Thanks for the nice work! Could this be reconsidered?

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.

8 participants