* [Buildroot] [PATCH] package/radlib: New package
@ 2016-01-19 16:19 Kinsella, Ray
2016-01-19 16:47 ` Thomas Petazzoni
0 siblings, 1 reply; 5+ messages in thread
From: Kinsella, Ray @ 2016-01-19 16:19 UTC (permalink / raw)
To: buildroot
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@intel.com>>
---
package/Config.in | 1 +
package/radlib/0001-cross_compile_link_bug.patch | 163 +++++++++++++++++++++++
package/radlib/Config.in | 22 +++
package/radlib/radlib.hash | 1 +
package/radlib/radlib.mk | 30 +++++
5 files changed, 217 insertions(+)
create mode 100644 package/radlib/0001-cross_compile_link_bug.patch
create mode 100644 package/radlib/Config.in
create mode 100644 package/radlib/radlib.hash
create mode 100644 package/radlib/radlib.mk
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"
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
@@ -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
+
+ if MYSQL
+ raddebug_LDFLAGS += -L$(prefix)/lib64/mysql -L$(prefix)/lib/mysql -L/usr/lib64/mysql -L/usr/lib/mysql
+@@ -39,6 +39,6 @@
+ endif
+ endif
+
+-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
+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
+ subdir = debug
+ DIST_COMMON = $(srcdir)/Makefile.am $(srcdir)/Makefile.in
+ ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
+@@ -198,7 +198,7 @@
+
+ # define include directories
+ INCLUDES = -I$(top_srcdir)/h -D_GNU_SOURCE -I$(prefix)/include \
+- -I/usr/include $(am__append_6)
++ $(am__append_6)
+
+ # define the sources
+ raddebug_SOURCES = \
+diff -Naur a/msgRouter/Makefile.am b/msgRouter/Makefile.am
+--- a/msgRouter/Makefile.am 2016-01-12 14:33:24.656252630 +0000
++++ b/msgRouter/Makefile.am 2016-01-12 14:33:37.859601998 +0000
+@@ -27,8 +27,8 @@
+ endif
+
+ # define library directories
+-radmrouted_LDFLAGS = -L../src/.libs -L$(prefix)/lib -L/usr/lib
+-INCLUDES += -I$(prefix)/include -I/usr/include
++radmrouted_LDFLAGS = -L../src/.libs -L$(prefix)/lib
++INCLUDES += -I$(prefix)/include
+
+ if MYSQL
+ radmrouted_LDFLAGS += -L$(prefix)/lib64/mysql -L$(prefix)/lib/mysql -L/usr/lib64/mysql -L/usr/lib/mysql
+@@ -39,6 +39,6 @@
+ endif
+ endif
+
+-if CROSSCOMPILE
+-radmrouted_LDFLAGS += $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
+-endif
++#if CROSSCOMPILE
++#radmrouted_LDFLAGS += $(prefix)/lib/crt1.o $(prefix)/lib/crti.o $(prefix)/lib/crtn.o
++#endif
+diff -Naur a/msgRouter/Makefile.in b/msgRouter/Makefile.in
+--- a/msgRouter/Makefile.in 2016-01-12 14:33:24.655252603 +0000
++++ b/msgRouter/Makefile.in 2016-01-12 14:34:16.740630810 +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)/lib -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
+ subdir = msgRouter
+ DIST_COMMON = $(srcdir)/Makefile.am $(srcdir)/Makefile.in
+ ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
+@@ -198,7 +198,7 @@
+
+ # define include directories
+ INCLUDES = -I$(top_srcdir)/h -D_GNU_SOURCE -I$(prefix)/include \
+- -I/usr/include $(am__append_6)
++ $(am__append_6)
+
+ # define the sources
+ radmrouted_SOURCES = \
+diff -Naur a/src/Makefile.am b/src/Makefile.am
+--- a/src/Makefile.am 2016-01-12 14:33:24.643252285 +0000
++++ b/src/Makefile.am 2016-01-12 14:35:06.261941170 +0000
+@@ -14,8 +14,7 @@
+
+ if MYSQL
+ MY_INCLUDES = \
+- -I$(prefix)/include/mysql \
+- -I/usr/include/mysql
++ -I$(prefix)/include/mysql
+ MY_SOURCES = \
+ $(top_srcdir)/database/mysql/my_database.c \
+ $(top_srcdir)/src/raddatabase.c
+@@ -24,8 +23,7 @@
+ endif
+ if PGRESQL
+ PG_INCLUDES = \
+- -I$(prefix)/include \
+- -I/usr/include
++ -I$(prefix)/include
+ PG_SOURCES = \
+ $(top_srcdir)/database/postgresql/pg_database.c \
+ $(top_srcdir)/src/raddatabase.c
+@@ -34,9 +32,6 @@
+ $(top_srcdir)/database/postgresql/_pg-types.h
+ endif
+ if SQLITE
+-SQ_INCLUDES = \
+- -I$(prefix)/include \
+- -I/usr/include
+ SQ_SOURCES = \
+ $(top_srcdir)/src/radsqlite.c
+ SQLITE_HDRS = \
+@@ -46,11 +41,9 @@
+ # define include directories
+ INCLUDES = \
+ -I$(top_srcdir)/h \
+- -I$(prefix)/include \
+ -D_GNU_SOURCE \
+ $(MY_INCLUDES) \
+- $(PG_INCLUDES) \
+- $(SQ_INCLUDES)
++ $(PG_INCLUDES)
+
+
+
+diff -Naur a/src/Makefile.in b/src/Makefile.in
+--- a/src/Makefile.in 2016-01-12 14:33:24.640252206 +0000
++++ b/src/Makefile.in 2016-01-12 14:34:38.655210681 +0000
+@@ -300,7 +300,6 @@
+ lib_LTLIBRARIES = librad.la
+ @MYSQL_TRUE at MY_INCLUDES = \
+ @MYSQL_TRUE@ -I$(prefix)/include/mysql \
+- at MYSQL_TRUE@ -I/usr/include/mysql
+
+ @MYSQL_TRUE at MY_SOURCES = \
+ @MYSQL_TRUE@ $(top_srcdir)/database/mysql/my_database.c \
+@@ -311,7 +310,6 @@
+
+ @PGRESQL_TRUE at PG_INCLUDES = \
+ @PGRESQL_TRUE@ -I$(prefix)/include \
+- at PGRESQL_TRUE@ -I/usr/include
+
+ @PGRESQL_TRUE at PG_SOURCES = \
+ @PGRESQL_TRUE@ $(top_srcdir)/database/postgresql/pg_database.c \
+@@ -323,7 +321,6 @@
+
+ @SQLITE_TRUE at SQ_INCLUDES = \
+ @SQLITE_TRUE@ -I$(prefix)/include \
+- at SQLITE_TRUE@ -I/usr/include
+
+ @SQLITE_TRUE at SQ_SOURCES = \
+ @SQLITE_TRUE@ $(top_srcdir)/src/radsqlite.c
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
+ 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$
+ bool "Enable MYSQL support in Radlib"
+ select BR2_PACKAGE_MYSQL
+ help$
+ radlib supports multiple database backends
+ selecting this option enables the mysql backend.
+
+endif
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 @@
+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
+#
+################################################################################
+
+RADLIB_VERSION = 2.12.0
+RADLIB_SOURCE = radlib-$(RADLIB_VERSION).tar.gz
+RADLIB_SITE = http://downloads.sourceforge.net/radlib
+RADLIB_INSTALL_STAGING = YES
+RADLIB_LICENSE = BSD 2 Clause
+RADLIB_LICENSE_FILES = COPYING
+RADLIB_DEPENDENCIES += sqlite
+
+RADLIB_CONF_OPTS += --enable-sqlite --prefix=$(STAGING_DIR)/usr
+
+ifeq ($(BR2_PACKAGE_RADLIB_MYSQL),y)
+RADLIB_CONF_OPTS += --enable-mysql
+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
+
+$(eval $(autotools-package))
+$(eval $(host-autotools-package))
--
2.4.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Buildroot] [PATCH] package/radlib: New package
2016-01-19 16:19 [Buildroot] [PATCH] package/radlib: New package Kinsella, Ray
@ 2016-01-19 16:47 ` Thomas Petazzoni
2016-01-20 11:57 ` Kinsella, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Petazzoni @ 2016-01-19 16:47 UTC (permalink / raw)
To: buildroot
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@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH] package/radlib: New package
2016-01-19 16:47 ` Thomas Petazzoni
@ 2016-01-20 11:57 ` Kinsella, Ray
2016-01-20 12:45 ` Thomas Petazzoni
0 siblings, 1 reply; 5+ messages in thread
From: Kinsella, Ray @ 2016-01-20 11:57 UTC (permalink / raw)
To: buildroot
Hi Thomas,
Please see my responses below.
On Tue, 2016-01-19 at 17:47 +0100, Thomas Petazzoni wrote:
> > Signed-off-by: Ray Kinsella <ray.kinsella@intel.com<mailto:
> > ray.kinsella at intel.com>>
>
> There is an issue here with SoB line, it should include this mailto:
> thing.
>
Apologies - Evolution is misbehaving.
> All patches should have a description + Signed-off-by line.
Ok - I will take care of it.
> > @@ -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.
Ok - but if I don't do this, it picks up the headers/libs for its
dependencies (libc, sqlite, mysql) from the host's /usr. I was using
the prefix to keep these paths right.
> > +-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 ?
Ok - I can ask why these wherever explicitly passed to linker in the
first instance. If I don't remove them - the linker fails complaining
of duplicate symbols. (the patch should just remove, rather than
comment these lines).
> > +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
> Same.
As above - but I will remove this altogether and let the reconf take
care of making the Makefile.in.
>
> > 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 ?
The sqlite db is staging area really - the data doesn't usually hang
around on the embedded device - it is generally uploaded to the cloud
etc. Might be more appropriate to trigger this wview instead of radlib.
> > + 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.
ok this makes sense - but is less explicit.
> > +
> > +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.
ok - in the same way as mysql then.
> 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.
ok noted - that I shouldn't explicitly passing these, but it still
doesn't fix my paths for dependencies.
> 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.
ok - will do.
> > +
> > +$(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 ?
Will remove host-autotools.
Thanks,
Ray K
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH] package/radlib: New package
2016-01-20 11:57 ` Kinsella, Ray
@ 2016-01-20 12:45 ` Thomas Petazzoni
2016-01-20 13:01 ` Kinsella, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Petazzoni @ 2016-01-20 12:45 UTC (permalink / raw)
To: buildroot
Hello,
On Wed, 20 Jan 2016 11:57:17 +0000, Kinsella, Ray wrote:
> > There is an issue here with SoB line, it should include this mailto:
> > thing.
>
> Apologies - Evolution is misbehaving.
Just don't use your e-mail client to send patches, use "git
send-email".
> > 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.
>
> Ok - but if I don't do this, it picks up the headers/libs for its
> dependencies (libc, sqlite, mysql) from the host's /usr. I was using
> the prefix to keep these paths right.
Just remove -I$(prefix)/include everywhere from the Makefile.am of this
package, it is bogus.
>
> > > +-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 ?
>
> Ok - I can ask why these wherever explicitly passed to linker in the
> first instance. If I don't remove them - the linker fails complaining
> of duplicate symbols. (the patch should just remove, rather than
> comment these lines).
Linking explicitly against crt1.o and crti.o seems wrong to me. gcc
will automatically take care of linking against crt1.o/crti.o when
needed.
> > > +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
> > Same.
>
> As above - but I will remove this altogether and let the reconf take
> care of making the Makefile.in.
Absolutely. Changing both Makefile.am and Makefile.in doesn't make much
sense.
> > > 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 ?
>
> The sqlite db is staging area really - the data doesn't usually hang
> around on the embedded device - it is generally uploaded to the cloud
> etc. Might be more appropriate to trigger this wview instead of radlib.
It is really up to the user to decide this.
Remember that on the same system, sqlite can also be used for other
purposes. So assuming that fsync() is not needed because *your* package
doesn't need it is a wrong assumption. Maybe other applications using
sqlite on the same system need to have fsync() enabled.
> > 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.
>
> ok this makes sense - but is less explicit.
Right, but it's "obvious" for the user: if you want MySQL support or
SQLite support in a package, it is kind of obvious that you need to
enable the MySQL / SQLite packages.
We normally only do sub-options for optional dependencies when it is
not obvious which other packages need to be enabled in order to benefit
for the optional dependency.
However, there are situations where this is not so clear-cut, so we
don't have a very strict rule about this.
> > > +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.
>
> ok - in the same way as mysql then.
Yes.
> ok noted - that I shouldn't explicitly passing these, but it still
> doesn't fix my paths for dependencies.
See above: remove all the -I$(prefix)/include and -L$(prefix)/lib from
the Makefile.am.
Are you in contact with upstream ?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH] package/radlib: New package
2016-01-20 12:45 ` Thomas Petazzoni
@ 2016-01-20 13:01 ` Kinsella, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Kinsella, Ray @ 2016-01-20 13:01 UTC (permalink / raw)
To: buildroot
Hi,
On Wed, 2016-01-20 at 13:45 +0100, Thomas Petazzoni wrote:
> Hello,
>
> On Wed, 20 Jan 2016 11:57:17 +0000, Kinsella, Ray wrote:
> Just don't use your e-mail client to send patches, use "git
> send-email".
ok - I will look at this :-)
>
> > > 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.
> >
> > Ok - but if I don't do this, it picks up the headers/libs for its
> > dependencies (libc, sqlite, mysql) from the host's /usr. I was
> > using
> > the prefix to keep these paths right.
>
> Just remove -I$(prefix)/include everywhere from the Makefile.am of
> this
> package, it is bogus.
ok - thanks for the pointer.
> >
> > > > +-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 ?
> >
> > Ok - I can ask why these wherever explicitly passed to linker in
> > the
> > first instance. If I don't remove them - the linker fails
> > complaining
> > of duplicate symbols. (the patch should just remove, rather than
> > comment these lines).
>
> Linking explicitly against crt1.o and crti.o seems wrong to me. gcc
> will automatically take care of linking against crt1.o/crti.o when
> needed.
Hence I commented them out - I will just remove them altogether via a
patch.
>
> > > > +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
> > > Same.
> >
> > As above - but I will remove this altogether and let the reconf
> > take
> > care of making the Makefile.in.
>
> Absolutely. Changing both Makefile.am and Makefile.in doesn't make
> much
> sense.
Agreed,
>
> > > > 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 ?
> >
> > The sqlite db is staging area really - the data doesn't usually
> > hang
> > around on the embedded device - it is generally uploaded to the
> > cloud
> > etc. Might be more appropriate to trigger this wview instead of
> > radlib.
>
> It is really up to the user to decide this.
Ok agreed.
> Remember that on the same system, sqlite can also be used for other
> purposes. So assuming that fsync() is not needed because *your*
> package
> doesn't need it is a wrong assumption. Maybe other applications using
> sqlite on the same system need to have fsync() enabled.
Understood - fair point.
> > > 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.
> >
> > ok this makes sense - but is less explicit.
>
> Right, but it's "obvious" for the user: if you want MySQL support or
> SQLite support in a package, it is kind of obvious that you need to
> enable the MySQL / SQLite packages.
>
> We normally only do sub-options for optional dependencies when it is
> not obvious which other packages need to be enabled in order to
> benefit
> for the optional dependency.
>
> However, there are situations where this is not so clear-cut, so we
> don't have a very strict rule about this.
ok - I will rework as indicated.
> > ok noted - that I shouldn't explicitly passing these, but it still
> > doesn't fix my paths for dependencies.
>
> See above: remove all the -I$(prefix)/include and -L$(prefix)/lib
> from
> the Makefile.am.
Agreed.
>
> Are you in contact with upstream ?
No - will send em a patch, once we are done here.
> Thanks,
>
> Thomas
Thanks for the feedback,
Ray K
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-20 13:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-19 16:19 [Buildroot] [PATCH] package/radlib: New package Kinsella, Ray
2016-01-19 16:47 ` Thomas Petazzoni
2016-01-20 11:57 ` Kinsella, Ray
2016-01-20 12:45 ` Thomas Petazzoni
2016-01-20 13:01 ` Kinsella, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox