spotify / sparkey

Simple constant key/value storage library, for read-heavy systems with infrequent large bulk inserts.
Apache License 2.0
1.18k stars 81 forks source link

Add CMake support #34

Open Qix- opened 8 years ago

Qix- commented 8 years ago

Because CMake is becoming increasingly popular.

Qix- commented 8 years ago

The tests fail due to not having strdup() available. It's not standard to begin with and I'm pretty sure Autotools is shimming it in or doing magic on the compiler detection, which it technically shouldn't be doing. The general "best practice" for strdup() is to write it yourself to be completely portable.

char * strdup(const char *str) {
    char * dup = malloc(sizeof(char) * (strlen(str) + 1));
    if (!dup) return 0;
    return strcpy(dup, str);
}

I can go ahead and do this in this PR or in a separate one if you'd like.

I believe the alternative is to detect Linux in the CMake configuration file and specify -std=g99 rather than -std=c99, which would probably enable it - however, g99 isn't available on LLVM-based compilers IIRC (i.e. CLang).

a1k0n commented 8 years ago

It looks like AC_PROG_CC_C99 is doing -std=gnu99:

checking for gcc option to accept ISO C99... -std=gnu99

I'm sort of surprised strdup doesn't work in c99 mode anymore.

Qix- commented 8 years ago

Like I said, strdup was never standardized, so c99 technically doesn't include it (I suppose some compilers could offer it, but you can't guarantee it). g99 is GNU's version of the standard which IIRC includes a few extensions (strdup being one of them).

The solution here can take a few forms. :)

spkrka commented 8 years ago

I am not sure we want to maintain both automake and cmake. Should we instead switch to cmake and remove automake? If so, is there any particular reason why we should do that? Preferably with some more technical arguments than "it's more popular" :)

As for the strdup issue, I think that's a valid problem but should be raised and fixed as a separate issue. strdup seems to be available in POSIX.1-2001 while we are compiling for c99. I made this PR to fix that: https://github.com/spotify/sparkey/pull/35

Qix- commented 8 years ago

CMake is more popular but that's not why I use it.

Firstly, it supports Ninja (and a slew of other build systems). Ninja has cut down my build times immensely, and I take any chance I get to work with it.

It also supports windows out of the box, including support for both MSVC as well as Cygwin or any other Unix-ey subsystem you want to run.

This also means that if a downsteam project relies on there being automake and then a subsequent call to make, you can instead run CMake (which by default generates Makefiles on non-windows systems) and run make after it instead.

Though the number one reason why I use it is for its include_subdirectory command, which lets me nest all of my projects with the dependencies they need and keep a clean, out of source build for all of it (assuming all of the dependencies use CMake).

This lets me build my entire game engine, for example, with a single command and have the engine and all of its dependencies build using Ninja (takes ~30 seconds to build 6000 files).

The only down side is its sometimes awkward syntax. However, in my personal opinion it's loads more readable and maintainable than Automake's cryptic macro system. Plus, by dropping a system that is used by many dependencies, all of which use Automake or Make alone, you don't have to write bootstrap scripts or hack in awkward pre-build steps to your build systems.

a1k0n commented 8 years ago

Poul-Henning Kamp wrote a pretty amusing thrashing of autoconf and why we shouldn't use it anymore: https://queue.acm.org/detail.cfm?id=2349257

BTW, echo nest is all cmake, and a lot of spotify stuff is too from what I remember...

Qix- commented 8 years ago

@a1kon Thanks for the link, that article speaks to my soul.

spkrka commented 8 years ago

I am fine with changing to cmake if you also remove automake usage in the same PR

Qix- commented 8 years ago

:+1: Will do, and will rebase on top of #35 as well.

Qix- commented 8 years ago

Couldn't rebase off of #35 as it looks like @spkrka has a private repo.

Qix- commented 8 years ago

Rebased.

New errors are due to non-standard GNU extensions (other byproducts of using -std=gnu99).

Not sure how standard you want this to be, but if requiring a gnu99-compliant compiler is required we can do that as well.