[Buildroot] [PATCH 1/1] flatcc: new package

[Arnout Vandecappelle]

  Hi Steve,

  Thanks for your contribution. I have a few comments, will you fix them and repost?


On 04/16/16 03:23, Steve deRosier wrote:
> This adds flatcc as a new package, pulling v0.3.0 from github. flatcc has
> both a host tool - the compiler, and libraries for the target.
>
> Signed-off-by: Steve deRosier <steve.derosier at lairdtech.com>
> ---
>   package/Config.in        |  1 +
>   package/flatcc/Config.in |  8 ++++++++
>   package/flatcc/flatcc.mk | 41 +++++++++++++++++++++++++++++++++++++++++

  You should also add a hash file.

>   3 files changed, 50 insertions(+)
>   create mode 100644 package/flatcc/Config.in
>   create mode 100644 package/flatcc/flatcc.mk
>
> diff --git a/package/Config.in b/package/Config.in
> index bcfe13b..9e3d953 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1201,6 +1201,7 @@ menu "Other"
>   	source "package/ding-libs/Config.in"
>   	source "package/eigen/Config.in"
>   	source "package/elfutils/Config.in"
> +	source "package/flatcc/Config.in"
>   	source "package/fftw/Config.in"
>   	source "package/flann/Config.in"
>   	source "package/gflags/Config.in"
> diff --git a/package/flatcc/Config.in b/package/flatcc/Config.in
> new file mode 100644
> index 0000000..4f84f06
> --- /dev/null
> +++ b/package/flatcc/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_FLATCC
> +	bool "flatcc"

  I doesn't seem to build on big-endian, so add
	depends on BR2_ENDIAN == "LITTLE"

> +	help
> +	  flatcc is C language implementation of Google Flatbuffers. It consists
> +	  of both a library for the target as well as a flatbuffer compiler tool
> +	  for the host.

  Help text should be wrapped at 72 columns, where the tab counts as 8, so 62 
significant characters.

> +
> +	  https://github.com/dvidelabs/flatcc
> diff --git a/package/flatcc/flatcc.mk b/package/flatcc/flatcc.mk
> new file mode 100644
> index 0000000..28fb89b
> --- /dev/null
> +++ b/package/flatcc/flatcc.mk
> @@ -0,0 +1,41 @@
> +################################################################################
> +#
> +# FLATCC
> +#
> +################################################################################
> +FLATCC_VERSION = v0.3.0

  There's a v0.3.2 now.

> +FLATCC_SITE =$(call github,dvidelabs,flatcc,$(FLATCC_VERSION))
                 ^ missing space

> +FLATCC_LICENSE = Apache-2.0
> +FLATCC_LICENSE_FILES = LICENSE
> +FLATCC_INSTALL_STAGING = YES
> +FLATCC_DEPENDENCIES += host-flatcc

  The + is redundant.

> +
> +FLATCC_CONF_OPTS += -DFLATCC_TEST=OFF -DFLATCC_PORTABLE=ON

  Why set portable to ON? AFAICS in the CMakeLists.txt, it should be detected 
properly based on the cross-gcc version... But maybe that's something new in v0.3.2.

> +HOST_FLATCC_CONF_OPTS += -DFLATCC_TEST=OFF
> +
> +define HOST_FLATCC_INSTALL_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/bin/flatcc $(HOST_DIR)/usr/bin/flatcc

  I was going to say: this shouldn't be needed - but in fact it is, it looks 
like they have a weird CMakeLists.txt that doesn't even have an 'install' 
target... Se yes, this is needed.

> +endef


> +
> +ifeq ($(BR2_STATIC_LIBS),y)
> +	FLATCC_INSTALL_TARGET = NO

  We don't usually do this. However, in this case it does make sense because 
it's an elegant way to not execute the INSTALL_TARGET_COMMANDS that install the 
non-existent .so files.

  That said, it should not be indented. We only indent commands (inside a define 
block), not variable assignments.


> +define FLATCC_INSTALL_STAGING_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.a $(STAGING_DIR)/usr/lib/libflatcc.a
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.a $(STAGING_DIR)/usr/lib/libflatccrt.a
> +	cp -r $(@D)/include/flatcc $(STAGING_DIR)/usr/include/.
> +endef
> +else
> +define FLATCC_INSTALL_STAGING_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(STAGING_DIR)/usr/lib/libflatcc.so
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(STAGING_DIR)/usr/lib/libflatccrt.so
> +	cp -r $(@D)/include/flatcc $(STAGING_DIR)/usr/include/.
> +endef
> +endif

  Here you forgot about the STATIC_SHARED case. The proper way to do it would be:

ifeq ($(BR2_SHARED_LIBS),)
define FLATCC INSTALL_STAGING_STATIC
	... libflatcc.a and libflatccrt.a
endef
endif
ifeq ($(BR2_STATIC_LIBS),)
define FLATCC_INSTALL_STAGING_SHARED
	... libflatcc.so and libflatccrt.so
endef
endif

define FLATCC_INSTALL_STAGING_COMMANDS
	cp -r $(@D)/include/flatcc $(STAGING_DIR)/usr/include/.
	$(FLATCC_INSTALL_STAGING_STATIC)
	$(FLATCC_INSTALL_STAGING_SHARED)
endef

Alternatively, the condition for static could be:
ifeq ($(BR2_STATIC_LIBS)$(BR2_STATIC_SHARED_LIBS),y)



  Regards,
  Arnout

> +
> +define FLATCC_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(TARGET_DIR)/usr/lib/libflatcc.so
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(TARGET_DIR)/usr/lib/libflatccrt.so
> +endef
> +
> +$(eval $(cmake-package))
> +$(eval $(host-cmake-package))
>


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF