[Buildroot] [PATCH] package/radlib: New package
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Tue Jan 19 16:47:34 UTC 2016
Hello,
Thanks for this contribution. See my review below for some comments.
On Tue, 19 Jan 2016 16:19:49 +0000, Kinsella, Ray wrote:
> radlib is a rapid application development library for unix
> multi-process applications. It uses SYS V IPC facilities and
> FIFOs to provide an RTOS-like, event-driven, distributed
> framework. Processes may be run as daemons or have a controlling
> terminal.
>
> Signed-off-by: Ray Kinsella <ray.kinsella at intel.com<mailto:ray.kinsella at intel.com>>
There is an issue here with SoB line, it should include this mailto:
thing.
> diff --git a/package/Config.in b/package/Config.in
> index b04c690..b971494 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1216,6 +1216,7 @@ endif
> source "package/protobuf-c/Config.in"
> source "package/qhull/Config.in"
> source "package/qlibc/Config.in"
> + source "package/radlib/Config.in"
Indentation problem.
> source "package/startup-notification/Config.in"
> source "package/tz/Config.in"
> source "package/tzdata/Config.in"
> diff --git a/package/radlib/0001-cross_compile_link_bug.patch b/package/radlib/0001-cross_compile_link_bug.patch
> new file mode 100644
> index 0000000..a532f93
> --- /dev/null
> +++ b/package/radlib/0001-cross_compile_link_bug.patch
All patches should have a description + Signed-off-by line.
> @@ -0,0 +1,163 @@
> +diff -Naur a/debug/Makefile.am b/debug/Makefile.am
> +--- a/debug/Makefile.am 2016-01-12 14:33:24.655252603 +0000
> ++++ b/debug/Makefile.am 2016-01-12 14:33:37.858601971 +0000
> +@@ -27,8 +27,8 @@
> + endif
> +
> + # define library directories
> +-raddebug_LDFLAGS = -L../src/.libs -L$(prefix)/lib -L/usr/lib
> +-INCLUDES += -I$(prefix)/include -I/usr/include
> ++raddebug_LDFLAGS = -L../src/.libs -L$(prefix)/lib
> ++INCLUDES += -I$(prefix)/include
This remains completely wrong. Contrary to what you do in your .mk
file, --prefix should *NOT* be $(STAGING_DIR)/usr or $(TARGET_DIR)/usr.
It should be just "/usr". And therefore those -I$(prefix)/include and
-L$(prefix)/lib are totally wrong.
> +-if CROSSCOMPILE
> +-raddebug_LDFLAGS += $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
> +-endif
> ++#if CROSSCOMPILE
> ++#raddebug_LDFLAGS += $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
> ++#endif
Why ?
> +diff -Naur a/debug/Makefile.in b/debug/Makefile.in
> +--- a/debug/Makefile.in 2016-01-12 14:33:24.655252603 +0000
> ++++ b/debug/Makefile.in 2016-01-12 14:34:05.062321796 +0000
> +@@ -43,7 +43,7 @@
> + @MYSQL_TRUE at am__append_4 = -L$(prefix)/lib64/mysql -L$(prefix)/lib/mysql -L/usr/lib64/mysql -L/usr/lib/mysql
> + @MYSQL_FALSE@@PGRESQL_TRUE at am__append_5 = -L$(prefix)/pgsql/lib
> + @MYSQL_FALSE@@PGRESQL_TRUE at am__append_6 = -I$(prefix)/pgsql/include
> +- at CROSSCOMPILE_TRUE@am__append_7 = $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
> ++#@CROSSCOMPILE_TRUE at am__append_7 = $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
Same.
> diff --git a/package/radlib/Config.in b/package/radlib/Config.in
> new file mode 100644
> index 0000000..2ca1617
> --- /dev/null
> +++ b/package/radlib/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_RADLIB
> + bool "radlib"
> + select BR2_PACKAGE_SQLITE
> + select BR2_PACKAGE_SQLITE_NO_SYNC
Why do you force this NO_SYNC option ?
> + help
> + radlib is a rapid application development library for unix
> + multi-process applications. It uses SYS V IPC facilities and
> + FIFOs to provide an RTOS-like, event-driven, distributed framework.
> + Processes may be run as daemons or have a controlling terminal.
> +
> + http://sourceforge.net/projects/radlib/
> +
> +if BR2_PACKAGE_RADLIB
> +
> +config BR2_PACKAGE_RADLIB_MYSQL$
Incorrect trailing $.
> + bool "Enable MYSQL support in Radlib"
> + select BR2_PACKAGE_MYSQL
> + help$
Ditto. Also, if you select BR2_PACKAGE_MYSQL, you must replicate all
the "depends on" of BR2_PACKAGE_MYSQL. I would therefore rather suggest
you to remove this option, and simply enable MySQL support in the .mk
file if BR2_PACKAGE_MYSQL=y.
> diff --git a/package/radlib/radlib.hash b/package/radlib/radlib.hash
> new file mode 100644
> index 0000000..ccbd532
> --- /dev/null
> +++ b/package/radlib/radlib.hash
> @@ -0,0 +1 @@
You need to indicate here in a comment where the hash is coming from.
> +sha256 82b98bb5e08a500dea1e4252843b9c772fa1fb67ac8ab89ed64abdd5e22eca66 radlib-2.12.0.tar.gz
> diff --git a/package/radlib/radlib.mk b/package/radlib/radlib.mk
> new file mode 100644
> index 0000000..3dc48f8
> --- /dev/null
> +++ b/package/radlib/radlib.mk
> @@ -0,0 +1,30 @@
> +################################################################################
> +#
> +# RADLib
Lowercase.
> +#
> +################################################################################
> +
> +RADLIB_VERSION = 2.12.0
> +RADLIB_SOURCE = radlib-$(RADLIB_VERSION).tar.gz
This variable is not needed here, as what you're using is the default
value.
> +RADLIB_SITE = http://downloads.sourceforge.net/radlib
> +RADLIB_INSTALL_STAGING = YES
> +RADLIB_LICENSE = BSD 2 Clause
BSD-2c
> +RADLIB_LICENSE_FILES = COPYING
> +RADLIB_DEPENDENCIES += sqlite
= is sufficient here.
> +
> +RADLIB_CONF_OPTS += --enable-sqlite --prefix=$(STAGING_DIR)/usr
If --enable-sqlite exists, then presumably it means that the sqlite
support is optional, no? If it is, then please make it optional.
Also, as said above --prefix=$(STAGING_DIR)/usr is completely wrong.
--prefix must be /usr, and it is already set to this value by the
autotools-package infrastructure, so you have nothing to do here.
"prefix" denotes where the software will be found while running on the
target. Certainly $(STAGING_DIR) doesn't exist once running on the
target!
So you must always use --prefix=/usr, and at installation time, pass
DESTDIR=$(TARGET_DIR) or DESTDIR=$(STAGING_DIR) to have things
installed not in /usr, but in $(TARGET_DIR)/usr or $(STAGING_DIR)/usr
respectively. All of this is done automatically for you by the
autotools-package infrastructure.
> +
> +ifeq ($(BR2_PACKAGE_RADLIB_MYSQL),y)
> +RADLIB_CONF_OPTS += --enable-mysql
This is wrong because it doesn't guarantee you that mysql has been
built before. You need to put "mysql" in RADLIB_DEPENDENCIES to have
this guarantee. Also, when something is not enabled, you should disable
it explicitly to avoid invalid auto-detection.
So, together with my suggestion of removing the
BR2_PACKAGE_RADLIB_MYSQL option, this would give:
ifeq ($(BR2_PACKAGE_MYSQL),y)
RADLIB_CONF_OPTS += --enable-mysql
RADLIB_DEPENDENCIES += mysql
else
RADLIB_CONF_OPTS += --disable-mysql
endif
This is the fairly canonical way of handling optional dependencies in
Buildroot packages. We use this construct all over the place.
> +endif
> +
> +define RADLIB_INSTALL_STAGING_CMDS
> + $(MAKE) exec_prefix=$(STAGING_DIR) install -C $(@D)/
> +endef
> +
> +define RADLIB_INSTALL_TARGET_CMDS
> + $(MAKE) DESTDIR=$(TARGET_DIR) install -C $(@D)/
> +endef
Those two variables definition should not be needed.
> +
> +$(eval $(autotools-package))
> +$(eval $(host-autotools-package))
Why do you call host-autotools-package here? For which reason do you
need a host variant of this package ?
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list