Skip to contentSkip to conversation

leveldb: new package

PATCH Superseded Steve James 2 participants 5 messages Jan 5, 2015 – Jan 8, 2015

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Steve James

author
#0 lore raw

PATCH v4 1/1 leveldb: new package

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 <[email protected]>
Cc "Yann E. MORIN" <[email protected]>

---
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
package/Config.in +1 -0
index 8d91b04..25efec5 100644
@@ -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"
package/leveldb/001-ssize_t-undefined.patch new file +17 -0
new file mode 100644
index 0000000..9a958d8
@@ -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 <[email protected]>
+
+--- 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 {
+
package/leveldb/Config.in new file +13 -0
new file mode 100644
index 0000000..535e217
@@ -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)
package/leveldb/leveldb.mk new file +57 -0

57 lines not on this page · raw message

Signature · 1 line
1.9.1
#1 lore raw

PATCH v4 1/1 leveldb: new package

Dear Steve James,

Review

Quoted 2 lines · Steve James
(quoted diff) +2 -0
+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

Review

Quoted 5 lines
(quoted diff) +5 -0
+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:

package/leveldb/leveldb.mk +2 -11
index 1612c9d..8688599 100644
@@ -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)

Review

Quoted 9 lines
(quoted diff) +9 -0
+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

Review

Quoted 10 lines
(quoted diff) +10 -0
+	$(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.

Review

Quoted 9 lines
(quoted diff) +9 -0
+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

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#2 lore raw

PATCH v4 1/1 leveldb: new package

Hello Thomas, Thanks for the feedback...

Quoted 9 lines · On Wednesday 07 Jan 2015 09:12:18 Thomas Petazzoni wrote:

Dear Steve James,

Review

Quoted 2 lines · Steve James
(quoted diff) +2 -0
+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.

Quoted 11 lines
Quoted 7 lines
(quoted diff) +1 -1
+LEVELDB_FLAGS += -DNDEBUG -I. -I./include -pthread -DOS_LINUX
-DLEVELDB_PLATFORM_POSIX +LEVELDB_CFLAGS += $(LEVELDB_FLAGS)

$(TARGET_CFLAGS)

(quoted diff) +3 -0
+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...

Quoted 9 lines

Instead, I would prefer:

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

CFLAGS += ...

to

Quoted 7 lines

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.

Quoted 24 lines

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

package/leveldb/leveldb.mk +0 -5
index 1612c9d..8688599 100644
@@ -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)" -

package/leveldb/leveldb.mk +0 -5
-# 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)
Quoted 4 lines

$(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.

Quoted 15 lines

ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)

Review

Quoted 9 lines
(quoted diff) +9 -0
+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.

Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	$(HOSTLN) -sf libleveldb.so.1.18

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

Quoted 4 lines
Quoted 4 lines
(quoted diff) +4 -0
+	$(HOSTLN) -sf libleveldb.so.1.18 $(STAGING_DIR)/usr/lib/libleveldb.so
+endef
+endif
+
Quoted 8 lines
Quoted 6 lines
(quoted diff) +5 -0
+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.

Quoted 5 lines
Quoted 5 lines
(quoted diff) +5 -0
+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
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	$(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.

Quoted 3 lines

Thanks!

Thomas

Thanks,
Steve.

#3 lore raw

PATCH v4 1/1 leveldb: new package

Dear Steve James,

Quoted 9 lines · Steve James

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.

Quoted 3 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 5 lines
Quoted 3 lines

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

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#4 lore raw

PATCH v4 1/1 leveldb: new package

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

Quoted 3 lines

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--

Quoted 7 lines

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.

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

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.

Quoted 7 lines

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.