Add CMake build script

[norbertwenzel]

As mentioned in #34 I added a CMakeLists.txt to build libdivide with CMake 3.1 and newer. The build configuration is based on the Makefile currently available.

I have tested the CMake build by building all projects in Debug and Release configurations and running the tester executable for both configurations (in a local CI). The tests were done on the following platforms:

• Ubuntu 14.04 Trusty amd64
• Ubuntu 14.04 Trusty arm (v7)
• Ubuntu 14.04 Trusty x86
• Ubuntu 16.04 Xenial amd64
• Ubuntu 16.04 Xenial arm (v7)
• Ubuntu 16.04 Xenial x86
• Windows 7 / Visual Studio 2014 amd64
• Windows 7 / Visual Studio 2014 x86

What do you @ridiculousfish and @kimwalisch think of this? Did I miss anything?

Possibly open issues:

• [ ] Should build be switched to CMake entirely and Makefile and manual Visual Studio project be removed?
• [ ] Should/Can CI with Travis and AppVeyor be switched to CMake?

[kimwalisch]

Hi Norbert,

The following code in your CMakeLists.txt is not ideal in my opinion:

cmake_push_check_state()
check_cxx_compiler_flag(-msse2 MSSE2_FLAG_AVAILABLE)
cmake_pop_check_state()

option(USE_SSE2 "use SSE2 instructions" ${MSSE2_FLAG_AVAILABLE})

if(USE_SSE2)
    message(STATUS "SSE2 is used")

In fact you can literally just copy and paste my sse code and get rid of the USE_SSE2 option. My sse code will enable SSE if the compiler and CPU support it and for testing we always want to enable SSE (if the compiler and CPU support it).

Hence instead you should use something like:

include(CheckCXXCompilerFlag)
include(CMakePushCheckState)

cmake_push_check_state()
set(CMAKE_REQUIRED_FLAGS -Werror)
check_cxx_compiler_flag(-msse2 msse2)
cmake_pop_check_state()

if(msse2)
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -msse2")
    add_definitions(-DLIBDIVIDE_USE_SSE2=1)
endif()

[norbertwenzel]

Your code would be Linux only as it checks for GCC/Clang specific flags, wouldn't it? And I considered the USE_SSE2 override a feature, ie. I might not want to use SSE2, although it might be supported on the current platform. I'm thinking about GCC4.8 where the benchmark fails with errors, which is caused by a bug in GCC4.8 with SSE I think. Therefore I thought it would be beneficial to add an option to enable/disable SSE2, regardless of the automatic detection.

[kimwalisch]

It would also be great if you would add an install target at the very bottom of CMakeLists.txt:

install(FILES libdivide.h DESTINATION ${CMAKE_INSTALL_PREFIX}/include)

This way the user can install libdivide using sudo make install.

[kimwalisch]

Your code would be Linux only as it checks for GCC/Clang specific flags, wouldn't it?

No, it should also work with e.g. MSVC. The trick is set(CMAKE_REQUIRED_FLAGS -Werror), because of the -Werror option the test will fail for MSVC.

-Werror is also mandatory for another reason:

GCC might support the -msse on non x86 architectures like ARM or PPC64 (I know this from personal experience). The -msse test only fails on these architectures if -Werror is also used.

[norbertwenzel]

No, it should also work with e.g. MSVC.

It already does work with MSVC (ie. it configures and runs fine), regardless of the -Werror. But it does not detect SSE2 support for MSVC. So with the current setup as is one would have to explicitely set USE_SSE2=ON on Windows if one wants to use SSE on this platform.

Adding -Werror makes sense if there are platforms that only emit warnings when SSE2 is not supported (and you said there are). Thanks for clarifying that.

[NeroBurner]

add install target with cmake-config file for automatic find_package detection

installed files

<prefix>/include/libdivide.h
<prefix>/lib/cmake/libdivide/libdivide-config.cmake
<prefix>/lib/cmake/libdivide/libdivide-targets.cmake

example client CMakeLists.txt file

cmake_minimum_required(VERSION 3.1)
project(libdivide-import-test)

find_package(libdivide CONFIG REQUIRED)

find_package(Threads)
add_executable(tester libdivide_test.cpp)
target_link_libraries(tester libdivide::libdivide ${CMAKE_THREAD_LIBS_INIT})

[norbertwenzel]

@kimwalisch Added the -Werror flag that was missing to the SSE2 detection now. I still left the USE_SSE2 option as I did not see any definitive answer if you absolutely want this removed.

[kimwalisch]

I still left the USE_SSE2 option as I did not see any definitive answer if you absolutely want this removed.

Leave it as is for now. I will test your code either today or tomorrow and see if I can simplify it a little bit. My idea is that there should be no build options at all so the user does not need to read a lengthy paragraph on how to compile.

This way we can also clean up the README.md quite a bit, e.g. using:

Build instructions

cmake .
make -j
make test
sudo make install

Note that I also intend to delete the original Makefile.

[norbertwenzel]

My idea is that there should be no build options at all [...]

Well there are plenty of build options now, but none of them are mandatory. Simply following your instructions posted here (or the instructions I added to README.md) should be enough to build this on any platform. But If you want to/have to there are options to en-/disable certain parts of the build script. Personally I prefer having these options as long as they have sensible default values.

Anyway, looking forward to the results of your test.

[NeroBurner]

Build instructions with a build directory for a clean source directory

mkdir build
cd build
cmake ..
make
sudo make install

Or OS independent (same for linux and Windows)

cmake -H. -Bbuild
cmake --build build
cmake --build build --target install

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[kimwalisch]

@norbertwenzel Your pull request to change the libdivide build system to CMake is over-engineered. libdivide is a C/C++ header-only library consisting of a single header file. We don't need 2 build scripts with a total of 145 lines of complicated code to build the 3 test programs.

Before you started development I advised you:

try to keep the build system as simple as possible (only one CMakeLists.txt with no other modules)

Keeping the build system simple allows other users to hack the code easily even without any CMake knowledge. Most developers are also lazy and impatient, they generally don't want to read meticulously how to build or install a software program. Instead they expect it to build in a standard way e.g. using cmake .; make; make install (this is why I insisted on no build options).

I think it would have taken too much effort to fix your code, so I rewrote it from scratch myself in only 60 lines of code.

[norbertwenzel]

I've tested your script on the platforms mentioned above. All tests still pass.

It seems you've also fixed the issue with SSE2 in benchmark on GCC4.8 (Trusty), since the benchmark tool now runs for >400 runs without any issues on Trusty. This change is highly welcomed since I have to still support GCC4.8.

Regarding your classification of the PR as over-engineered I disagree. The initial PR without the install targets was only 70 lines (+1 if I added your one-line install target), ie. not that much bigger than your file. The main difference in size were the options, that you prefer to infer automatically and only automatically, whereas I prefer to allow users to override these options, in case this was necessary (eg. work around compiler bugs or enable SSE2 on Windows). The install target as proposed has been tailored to allow inclusion of this library in Hunter, which is a requirement on my side. Hopefully that explains some of the engineering.

Anyway, the build does mostly the same as it did in my PR and you seem to have fixed some issues in the code as you added your version of CMakeLists.txt, which I consider still a good outcome from my perspective.

[kimwalisch]

It seems you've also fixed the issue with SSE2 in benchmark on GCC4.8 (Trusty), since the benchmark tool now runs for >400 runs without any issues on Trusty.

When I ported the build system to CMake I tested it using many different compilers versions. Doing this I found a lot of compiler warnings that I also fixed. But I have not specifically fixed the bug you mentioned. I have no idea why this bug has vanished... I thought the GCC 4.8 bug was a compiler bug, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61108.

The install target as proposed has been tailored to allow inclusion of this library in Hunter, which is a requirement on my side. Hopefully that explains some of the engineering.

You could have explained this upfront. I couldn't understand why anyone would write such complicated code, at least it makes sense now. I was just looking at the average user who copies and pastes libdivide.h into his own source tree.

I just re-read your CMakeLists.txt and I actually found something I like:

add_test(tester-u32 tester "u32")
add_test(tester-u64 tester "u64")
add_test(tester-s32 tester "s32")
add_test(tester-s64 tester "s64")

That's pretty neat for make test, I will implement this in libdivide's ``CMakeLists.txt```.

[kimwalisch]

add_test(tester-u32 tester "u32")
add_test(tester-u64 tester "u64")
add_test(tester-s32 tester "s32")
add_test(tester-s64 tester "s64")

I just figured out that the tester is multi-threaded, it runs one thread for each type (s32, u32, s64, u64). So when you run the tester without arguments it runs all 4 tests using 4 threads. But when you run the tester by specifying a single type (e.g. ./tester "s32") it is single-threaded.

Hence I will continue to use:

add_test(tester tester)

in order to benefit from multi-threading.

[norbertwenzel]

I thought the GCC 4.8 bug was a compiler bug, [...]

I thought so too, but I figured you had maybe added a workaround, since I did not check all your commits thoroughly. I just checked the code was still using SSE2 and it was. So if and why this is resolved now remains a mystery.

You could have explained this upfront.

I'm sorry. Since I gladly took your hint for testing whether compiler flags are supported I probably assumed an explanation was not needed. I definitely could have explained the reasoning behind all this code in a discussion. The main difference between our install scripts is that you install the header in a known location whereas I install a CMake script (alongside the header) in a known location. This enables the user to use find_package(libdivide CONFIG REQUIRED) (which moves the error for a missing dependency from the compile to the configure step), as has been shown in the example by @NeroBurner. Additionally this enables libdivide to impose some flags (eg. -msse2) to the user automatically as it makes the dependency on libdivide explicit to CMake.

I actually found something I like: [...] Hence I will continue to use add_test(tester tester) [...]

And I was just about to rejoice... :wink: But you are right, tester spawns up 4 threads in any case. I assumed it would spawn only n-1 threads for n tasks. The reason I split up the tests was that, in case of an error for eg. uint32_t only, it would be visible from the test output which test exactly failed. This was a pure convenience function, disregarding any performance considerations at all.

[norbertwenzel]

I have no idea why this bug has vanished... I thought the GCC 4.8 bug was a compiler bug [...]

I probably made a mistake when testing. I've repeated my tests for GCC 4.8 again this morning and it failed as expected in Release builds.