[Buildroot] [PATCH v4] leveldb: new package

[Buildroot] [PATCH v4] leveldb: new package

[Steve James]

Adds new package: leveldb

  LevelDB is a fast key-value storage library written at Google that
  provides an ordered mapping from string keys to string values.

Signed-off-by: Steve James <ste@junkomatic.net>
Cc: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
Changes v3 -> v4:
  - Don't damage global scope make macros
  - Add dependency on threads toolchain
Changes v2 -> v3:
  - Re-submit without line wrapping
Changes v1 -> v2:
  - Upstream release labels don't include package name, giving us an
    anonymous download archive, so use $(call github) method instead
  - Use $(TARGET_CONFIGURE_OPTS)
  - Support new shared vs static vs shared+static possibilities

 package/Config.in                           |  1 +
 package/leveldb/001-ssize_t-undefined.patch | 17 +++++++++
 package/leveldb/Config.in                   | 13 +++++++
 package/leveldb/leveldb.mk                  | 57 +++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+)
 create mode 100644 package/leveldb/001-ssize_t-undefined.patch
 create mode 100644 package/leveldb/Config.in
 create mode 100644 package/leveldb/leveldb.mk

diff --git a/package/Config.in b/package/Config.in
index 8d91b04..25efec5 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -630,6 +630,7 @@ menu "Database"
 	source "package/berkeleydb/Config.in"
 	source "package/cppdb/Config.in"
 	source "package/gdbm/Config.in"
+	source "package/leveldb/Config.in"
 	source "package/mysql/Config.in"
 	source "package/postgresql/Config.in"
 	source "package/redis/Config.in"
diff --git a/package/leveldb/001-ssize_t-undefined.patch b/package/leveldb/001-ssize_t-undefined.patch
new file mode 100644
index 0000000..9a958d8
--- /dev/null
+++ b/package/leveldb/001-ssize_t-undefined.patch
@@ -0,0 +1,17 @@
+Fix leveldb issue 233 leveldb does not compile with g++ 4.8.2
+
+Where db_iter.cc fails to get a typedef for ssize_t when compiled by GCC.
+
+Upstream-Status: Submitted [https://github.com/google/leveldb/issues/233]
+Signed-off-by: Steve James <ste@junkomatic.net>
+
+--- a/db/db_iter.cc.orig	2014-12-08 16:54:31.384615752 +0000
++++ b/db/db_iter.cc	2014-12-08 16:54:35.464656890 +0000
+@@ -13,6 +13,7 @@
+ #include "util/logging.h"
+ #include "util/mutexlock.h"
+ #include "util/random.h"
++#include <sys/types.h> // for ssize_t
+ 
+ namespace leveldb {
+ 
diff --git a/package/leveldb/Config.in b/package/leveldb/Config.in
new file mode 100644
index 0000000..535e217
--- /dev/null
+++ b/package/leveldb/Config.in
@@ -0,0 +1,13 @@
+config BR2_PACKAGE_LEVELDB
+	bool "leveldb"
+	depends on BR2_INSTALL_LIBSTDCPP
+	depends on BR2_TOOLCHAIN_HAS_THREADS
+	select BR2_PACKAGE_SNAPPY
+	help
+	  LevelDB is a fast key-value storage library written at Google that
+	  provides an ordered mapping from string keys to string values.
+
+	  https://github.com/google/leveldb
+
+comment "leveldb needs a toolchain w/ C++, threads"
+	depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_HAS_THREADS)
diff --git a/package/leveldb/leveldb.mk b/package/leveldb/leveldb.mk
new file mode 100644
index 0000000..1612c9d
--- /dev/null
+++ b/package/leveldb/leveldb.mk
@@ -0,0 +1,57 @@
+################################################################################
+#
+# leveldb
+#
+################################################################################
+
+LEVELDB_VERSION = 803d69203a62faf50f1b77897310a3a1fcae712b
+LEVELDB_SITE = $(call github,google,leveldb,$(LEVELDB_VERSION))
+LEVELDB_LICENSE = BSD-3c
+LEVELDB_LICENSE_FILES = LICENSE
+LEVELDB_INSTALL_STAGING = YES
+LEVELDB_DEPENDENCIES = snappy
+
+LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX -DLEVELDB_PLATFORM_POSIX
+LEVELDB_CFLAGS += $(LEVELDB_FLAGS) $(TARGET_CFLAGS)
+LEVELDB_CXXFLAGS += $(LEVELDB_FLAGS) $(TARGET_CXXFLAGS) -std=c++0x
+
+LEVELDB_MAKE_ARGS += CFLAGS="$(LEVELDB_CFLAGS)" CXXFLAGS="$(LEVELDB_CXXFLAGS)"
+
+# The lelveldb build has some basic autoconf-style behaviour.
+# We hard-wire it here to the right outcome for Buildroot
+LEVELDB_MAKE_ARGS += TARGET_OS=Linux
+
+define LEVELDB_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) $(LEVELDB_MAKE_ARGS) -C $(@D)
+endef
+
+ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+define LEVELDB_INSTALL_STAGING_STATIC
+	$(INSTALL) -D -m 0644 $(@D)/libleveldb.a $(STAGING_DIR)/usr/lib
+endef
+endif
+
+ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+define LEVELDB_INSTALL_STAGING_SHARED
+	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(STAGING_DIR)/usr/lib
+	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so.1
+	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so
+endef
+endif
+
+define LEVELDB_INSTALL_STAGING_CMDS
+	$(LEVELDB_INSTALL_STAGING_STATIC)
+	$(LEVELDB_INSTALL_STAGING_SHARED)
+	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include/leveldb
+	$(INSTALL) -D -m 0644 $(@D)/include/leveldb/*.h $(STAGING_DIR)/usr/include/leveldb
+endef
+
+ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
+define LEVELDB_INSTALL_TARGET_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(TARGET_DIR)/usr/lib
+	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so.1
+	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so
+endef
+endif
+
+$(eval $(generic-package))
-- 
1.9.1

[Buildroot] [PATCH v4] leveldb: new package

[Thomas Petazzoni]

Dear Steve James,

On Mon,  5 Jan 2015 14:47:47 +0000, Steve James wrote:

> +comment "leveldb needs a toolchain w/ C++, threads"
> +	depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_HAS_THREADS)

I find it more natural to have

	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS

> +LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX -DLEVELDB_PLATFORM_POSIX
> +LEVELDB_CFLAGS += $(LEVELDB_FLAGS) $(TARGET_CFLAGS)
> +LEVELDB_CXXFLAGS += $(LEVELDB_FLAGS) $(TARGET_CXXFLAGS) -std=c++0x
> +
> +LEVELDB_MAKE_ARGS += CFLAGS="$(LEVELDB_CFLAGS)" CXXFLAGS="$(LEVELDB_CXXFLAGS)"

I don't really like this solution, because you have to duplicate flags
from the leveldb Makefile into Buildroot, which isn't very future
proof. Instead, I would prefer:

 (1) A patch for leveldb that changes the CFLAGS, LDFLAGS, LIBS and
     CXXFLAGS line from:

       CFLAGS += ...

     to

       override CFLAGS += ...

     This way, we can pass CFLAGS and LDFLAGS in the environment, and
     leveldb Makefile can still add more flags.

     This patch can (should?) be submitted to leveldb upstream
     developers.

 (2) A simplification of leveldb.mk to remove all this complexity. I
     got it to work with the following change:

diff --git a/package/leveldb/leveldb.mk b/package/leveldb/leveldb.mk
index 1612c9d..8688599 100644
--- a/package/leveldb/leveldb.mk
+++ b/package/leveldb/leveldb.mk
@@ -11,18 +11,9 @@ LEVELDB_LICENSE_FILES = LICENSE
 LEVELDB_INSTALL_STAGING = YES
 LEVELDB_DEPENDENCIES = snappy
 
-LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX -DLEVELDB_PLATFORM_POSIX
-LEVELDB_CFLAGS += $(LEVELDB_FLAGS) $(TARGET_CFLAGS)
-LEVELDB_CXXFLAGS += $(LEVELDB_FLAGS) $(TARGET_CXXFLAGS) -std=c++0x
-
-LEVELDB_MAKE_ARGS += CFLAGS="$(LEVELDB_CFLAGS)" CXXFLAGS="$(LEVELDB_CXXFLAGS)"
-
-# The lelveldb build has some basic autoconf-style behaviour.
-# We hard-wire it here to the right outcome for Buildroot
-LEVELDB_MAKE_ARGS += TARGET_OS=Linux
-
 define LEVELDB_BUILD_CMDS
-       $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) $(LEVELDB_MAKE_ARGS) -C $(@D)
+       $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
+               OPT=-DNDEBUG TARGET_OS=Linux -C $(@D)
 endef
 
 ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)


> +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +define LEVELDB_INSTALL_STAGING_STATIC
> +	$(INSTALL) -D -m 0644 $(@D)/libleveldb.a $(STAGING_DIR)/usr/lib
> +endef
> +endif
> +
> +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +define LEVELDB_INSTALL_STAGING_SHARED
> +	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(STAGING_DIR)/usr/lib

Full path for the destination is needed:

							$(STAGING_DIR)/usr/lib/libleveldb.so.1.18

> +	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so.1
> +	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so
> +endef
> +endif
> +
> +define LEVELDB_INSTALL_STAGING_CMDS
> +	$(LEVELDB_INSTALL_STAGING_STATIC)
> +	$(LEVELDB_INSTALL_STAGING_SHARED)
> +	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include/leveldb
> +	$(INSTALL) -D -m 0644 $(@D)/include/leveldb/*.h $(STAGING_DIR)/usr/include/leveldb

I'm not sure of the behavior of $(INSTALL) -D for multiple source files.

> +endef
> +
> +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> +define LEVELDB_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(TARGET_DIR)/usr/lib
> +	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so.1
> +	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so
> +endef
> +endif

It would be good to add a comment above all these installation rules to
indicate that the leveldb build system doesn't provide any "make
install" rule.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v4] leveldb: new package

[Steve James]

Hello Thomas, Thanks for the feedback...

On Wednesday 07 Jan 2015 09:12:18 Thomas Petazzoni wrote:
> Dear Steve James,
> 
> On Mon,  5 Jan 2015 14:47:47 +0000, Steve James wrote:
> > +comment "leveldb needs a toolchain w/ C++, threads"
> > +	depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_HAS_THREADS)
> 
> I find it more natural to have
> 
> 	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS

OK, if you wish (I just copy-pasted from elsewhere). BTW I don't find this 
statement natural to read whichever way the logic is expressed. I think it 
would read better if "depends on" where replaced with "when" (or "if") 
instead:-

comment "leveldb needs a toolchain w/ C++, threads"
	when !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS)

But maybe that's just something I have to get used to.

> > +LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX
> > -DLEVELDB_PLATFORM_POSIX +LEVELDB_CFLAGS += $(LEVELDB_FLAGS)
> > $(TARGET_CFLAGS)
> > +LEVELDB_CXXFLAGS += $(LEVELDB_FLAGS) $(TARGET_CXXFLAGS) -std=c++0x
> > +
> > +LEVELDB_MAKE_ARGS += CFLAGS="$(LEVELDB_CFLAGS)"
> > CXXFLAGS="$(LEVELDB_CXXFLAGS)"
> 
> I don't really like this solution, because you have to duplicate flags
> from the leveldb Makefile into Buildroot, which isn't very future
> proof.

Fair enough. I don't like it either, for the same reason (but I thought I 
might get away with it :). I have been resisting making a patch for leveldb 
but...

> Instead, I would prefer:
> 
>  (1) A patch for leveldb that changes the CFLAGS, LDFLAGS, LIBS and
>      CXXFLAGS line from:
> 
>        CFLAGS += ...
> 
>      to
> 
>        override CFLAGS += ...
> 
>      This way, we can pass CFLAGS and LDFLAGS in the environment, and
>      leveldb Makefile can still add more flags.
> 
>      This patch can (should?) be submitted to leveldb upstream
>      developers.

OK I give in. I'll make a patch to adjust the leveldb Makefile and I'll send 
it upstream.

>  (2) A simplification of leveldb.mk to remove all this complexity. I
>      got it to work with the following change:
> 
> diff --git a/package/leveldb/leveldb.mk b/package/leveldb/leveldb.mk
> index 1612c9d..8688599 100644
> --- a/package/leveldb/leveldb.mk
> +++ b/package/leveldb/leveldb.mk
> @@ -11,18 +11,9 @@ LEVELDB_LICENSE_FILES = LICENSE
>  LEVELDB_INSTALL_STAGING = YES
>  LEVELDB_DEPENDENCIES = snappy
> 
> -LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX
> -DLEVELDB_PLATFORM_POSIX -LEVELDB_CFLAGS += $(LEVELDB_FLAGS)
> $(TARGET_CFLAGS)
> -LEVELDB_CXXFLAGS += $(LEVELDB_FLAGS) $(TARGET_CXXFLAGS) -std=c++0x
> -
> -LEVELDB_MAKE_ARGS += CFLAGS="$(LEVELDB_CFLAGS)"
> CXXFLAGS="$(LEVELDB_CXXFLAGS)" -
> -# The lelveldb build has some basic autoconf-style behaviour.
> -# We hard-wire it here to the right outcome for Buildroot
> -LEVELDB_MAKE_ARGS += TARGET_OS=Linux
> -
>  define LEVELDB_BUILD_CMDS
> -       $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS)
> $(LEVELDB_MAKE_ARGS) -C $(@D) +       $(TARGET_MAKE_ENV) $(MAKE)
> $(TARGET_CONFIGURE_OPTS) \
> +               OPT=-DNDEBUG TARGET_OS=Linux -C $(@D)
>  endef

Yes, the ugly complexity can go away when the Makefile isn't fighting back.

>  ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> 
> > +ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +define LEVELDB_INSTALL_STAGING_STATIC
> > +	$(INSTALL) -D -m 0644 $(@D)/libleveldb.a $(STAGING_DIR)/usr/lib
> > +endef
> > +endif
> > +
> > +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +define LEVELDB_INSTALL_STAGING_SHARED
> > +	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(STAGING_DIR)/usr/lib
> 
> Full path for the destination is needed:
> 
> 							
$(STAGING_DIR)/usr/lib/libleveldb.so.1.18

No, I don't think so. From install(1) man page:-

SYNOPSIS
       install [OPTION]... [-T] SOURCE DEST
       install [OPTION]... SOURCE... DIRECTORY
       install [OPTION]... -t DIRECTORY SOURCE...
       install [OPTION]... -d DIRECTORY...

I'm just using the syntax on the second line.

> > +	$(HOSTLN) -sf libleveldb.so.1.18 
$(STAGING_DIR)/usr/lib/libleveldb.so.1
> > +	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so
> > +endef
> > +endif
> > +
> > +define LEVELDB_INSTALL_STAGING_CMDS
> > +	$(LEVELDB_INSTALL_STAGING_STATIC)
> > +	$(LEVELDB_INSTALL_STAGING_SHARED)
> > +	$(INSTALL) -d -m 0755 $(STAGING_DIR)/usr/include/leveldb
> > +	$(INSTALL) -D -m 0644 $(@D)/include/leveldb/*.h
> > $(STAGING_DIR)/usr/include/leveldb
> 
> I'm not sure of the behavior of $(INSTALL) -D for multiple source files.

I have accidentally used -D redundantly here, since the preceding line creates 
the destination directory first any way.

> > +endef
> > +
> > +ifeq ($(BR2_SHARED_LIBS)$(BR2_SHARED_STATIC_LIBS),y)
> > +define LEVELDB_INSTALL_TARGET_CMDS
> > +	$(INSTALL) -D -m 0755 $(@D)/libleveldb.so.1.18 $(TARGET_DIR)/usr/lib
> > +	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so.1
> > +	$(HOSTLN) -sf libleveldb.so.1.18 $(TARGET_DIR)/usr/lib/libleveldb.so
> > +endef
> > +endif
> 
> It would be good to add a comment above all these installation rules to
> indicate that the leveldb build system doesn't provide any "make
> install" rule.

Or better: I'll add the missing install recipe to the Makefile.

> Thanks!
> 
> Thomas

Thanks,
Steve.

[Buildroot] [PATCH v4] leveldb: new package

[Thomas Petazzoni]

Dear Steve James,

On Thu, 8 Jan 2015 12:09:41 +0000, Steve James wrote:

> OK, if you wish (I just copy-pasted from elsewhere). BTW I don't find this 
> statement natural to read whichever way the logic is expressed. I think it 
> would read better if "depends on" where replaced with "when" (or "if") 
> instead:-
> 
> comment "leveldb needs a toolchain w/ C++, threads"
> 	when !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS)
> 
> But maybe that's just something I have to get used to.

Well, Buildroot is re-using kconfig directly from the Linux kernel
sources. So we haven't defined the kconfig language, and we don't want
to change it specifically for Buildroot.

> Fair enough. I don't like it either, for the same reason (but I thought I 
> might get away with it :). I have been resisting making a patch for leveldb 
> but...

Yes, I agree there is sometimes a balance between having a patch, and
doing a little bit more work in the Buildroot .mk file. In this case, I
believe having a patch makes the whole thing a lot nicer, and hopefully
the patch can be applied upstream.


> > Full path for the destination is needed:
> > 
> > 							
> $(STAGING_DIR)/usr/lib/libleveldb.so.1.18
> 
> No, I don't think so. From install(1) man page:-

If you don't use a full path and do:

	$(INSTALL) -D $(@D)/foo $(STAGING_DIR)/usr/lib

and $(STAGING_DIR)/usr/lib doesn't already exist as a directory, then a
file named $(STAGING_DIR)/usr/lib will be created, with the contents of
$(@D)/foo. Cleary not what we want.

> > It would be good to add a comment above all these installation rules to
> > indicate that the leveldb build system doesn't provide any "make
> > install" rule.
> 
> Or better: I'll add the missing install recipe to the Makefile.

Indeed.

However, I looked quickly at the contribution process for leveldb and
it's a bit annoying: you have to sign a CLA to assign the copyright of
your contributions. Or hopefully they might consider those
contributions as small enough to not be copyrightable, and avoid the
CLA hassle. I'll let you handle that with upstream.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v4] leveldb: new package

[Steve James]

On Thursday 08 Jan 2015 12:27:19 Thomas Petazzoni wrote:
--snip--

> Well, Buildroot is re-using kconfig directly from the Linux kernel
> sources. So we haven't defined the kconfig language, and we don't want
> to change it specifically for Buildroot.

As I thought. I'll get used to it.

--snip--

> If you don't use a full path and do:
> 
> 	$(INSTALL) -D $(@D)/foo $(STAGING_DIR)/usr/lib
> 
> and $(STAGING_DIR)/usr/lib doesn't already exist as a directory, then a
> file named $(STAGING_DIR)/usr/lib will be created, with the contents of
> $(@D)/foo. Cleary not what we want.

I would ordinarily create destination directories first, but I hadn't in this 
case, so good point.

> > > It would be good to add a comment above all these installation rules to
> > > indicate that the leveldb build system doesn't provide any "make
> > > install" rule.
> > 
> > Or better: I'll add the missing install recipe to the Makefile.
> 
> Indeed.
> 
> However, I looked quickly at the contribution process for leveldb and
> it's a bit annoying: you have to sign a CLA to assign the copyright of
> your contributions. Or hopefully they might consider those
> contributions as small enough to not be copyrightable, and avoid the
> CLA hassle. I'll let you handle that with upstream.

Yes I saw that too. How very annoying. I'm reluctant to put my name on a 
Google License Agreement just to tweak a Makefile. I see they even want my 
address and phone number. Hmm.

Whether they accept it upstream or not, I think patching the install recipe 
into the leveldb Makefile is a more correct solution and will be better for 
Buildroot.

Steve.