[Buildroot] [PATCH v4] leveldb: new package

[Steve James <ste@junkomatic.net>]

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.