Skip to contentSkip to conversation

package/radlib: New package

PATCH New Kinsella 2 participants 5 messages Jan 19, 2016 – Jan 20, 2016

lore
Share & tools
lore

Kinsella

#0 lore raw

PATCH 1/1 package/radlib: New package

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:[email protected]>>
---
 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
package/Config.in +1 -0
index b04c690..b971494 100644
@@ -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"
package/radlib/0001-cross_compile_link_bug.patch new file +163 -0

163 lines not on this page · raw message

package/radlib/Config.in new file +22 -0
new file mode 100644
index 0000000..2ca1617
@@ -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
package/radlib/radlib.hash new file +1 -0
new file mode 100644
index 0000000..ccbd532
@@ -0,0 +1 @@
+sha256  82b98bb5e08a500dea1e4252843b9c772fa1fb67ac8ab89ed64abdd5e22eca66  radlib-2.12.0.tar.gz
package/radlib/radlib.mk new file +30 -0
new file mode 100644
index 0000000..3dc48f8
@@ -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

#1 lore raw

PATCH 1/1 package/radlib: New package

Hello,

Thanks for this contribution. See my review below for some comments.

Quoted 7 lines · Ray

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:[email protected]>>

There is an issue here with SoB line, it should include this mailto:
thing.

package/Config.in endif · line 1219

Quoted 9 lines
package/Config.in +1 -0
index b04c690..b971494 100644
@@ -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.

package/radlib/0001-cross_compile_link_bug.patch

Quoted 8 lines
package/Config.in +0 -0
        source "package/startup-notification/Config.in"
        source "package/tz/Config.in"
        source "package/tzdata/Config.in"
package/radlib/0001-cross_compile_link_bug.patch new file +0 -0
new file mode 100644
index 0000000..a532f93

All patches should have a description + Signed-off-by line.

package/radlib/0001-cross_compile_link_bug.patch line 11

Quoted 12 lines
package/radlib/0001-cross_compile_link_bug.patch new file +11 -0
@@ -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.

package/radlib/0001-cross_compile_link_bug.patch line 24

Quoted 6 lines
package/radlib/0001-cross_compile_link_bug.patch new file +6 -0
+-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 ?

package/radlib/0001-cross_compile_link_bug.patch line 33

Quoted 9 lines
package/radlib/0001-cross_compile_link_bug.patch new file +9 -0
+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.

package/radlib/Config.in line 4

Quoted 10 lines
package/radlib/Config.in new file +4 -0
new file mode 100644
index 0000000..2ca1617
@@ -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 ?

package/radlib/Config.in line 15

Quoted 11 lines
package/radlib/Config.in new file +11 -0
+       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 $.

package/radlib/Config.in line 18

Quoted 3 lines
package/radlib/Config.in new file +3 -0
+       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.

package/radlib/radlib.hash

Quoted 6 lines
package/radlib/radlib.hash new file +0 -0
new file mode 100644
index 0000000..ccbd532
@@ -0,0 +1 @@

You need to indicate here in a comment where the hash is coming from.

package/radlib/radlib.mk line 3

Quoted 10 lines
package/radlib/radlib.hash new file +1 -0
+sha256  82b98bb5e08a500dea1e4252843b9c772fa1fb67ac8ab89ed64abdd5e22eca66  radlib-2.12.0.tar.gz
package/radlib/radlib.mk new file +3 -0
new file mode 100644
index 0000000..3dc48f8
@@ -0,0 +1,30 @@
+################################################################################
+#
+# RADLib

Lowercase.

package/radlib/radlib.mk line 8

Quoted 5 lines
package/radlib/radlib.mk new file +5 -0
+#
+################################################################################
+
+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.

package/radlib/radlib.mk line 11

Quoted 3 lines
package/radlib/radlib.mk new file +3 -0
+RADLIB_SITE = http://downloads.sourceforge.net/radlib
+RADLIB_INSTALL_STAGING = YES
+RADLIB_LICENSE = BSD 2 Clause

BSD-2c

package/radlib/radlib.mk line 13

Quoted 2 lines
package/radlib/radlib.mk new file +2 -0
+RADLIB_LICENSE_FILES = COPYING
+RADLIB_DEPENDENCIES += sqlite

= is sufficient here.

package/radlib/radlib.mk line 15

Quoted 2 lines
package/radlib/radlib.mk new file +2 -0
+
+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.

package/radlib/radlib.mk line 18

Quoted 3 lines
package/radlib/radlib.mk new file +3 -0
+
+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.

package/radlib/radlib.mk line 27

Quoted 9 lines
package/radlib/radlib.mk new file +9 -0
+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.

package/radlib/radlib.mk line 30

Quoted 3 lines
package/radlib/radlib.mk new file +3 -0
+
+$(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

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

PATCH 1/1 package/radlib: New package

Hi Thomas,

Please see my responses below.

Quoted 6 lines · Thomas Petazzoni
Quoted 2 lines
Signed-off-by Ray Kinsella <[email protected]<mailto:

ray.kinsella at intel.com>>

There is an issue here with SoB line, it should include this mailto:
thing.

Apologies - Evolution is misbehaving.

Quoted 1 line

All patches should have a description + Signed-off-by line.

Ok - I will take care of it.

Quoted 10 lines
Quoted 10 lines
package/radlib/0001-cross_compile_link_bug.patch new file +9 -0
@@ -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
Quoted 8 lines

Review line 11

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

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

Quoted 4 lines
Quoted 3 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.

Quoted 13 lines

package/radlib/Config.in line 4

Quoted 11 lines
package/radlib/Config.in new file +4 -0
new file mode 100644
index 0000000..2ca1617
@@ -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.

Quoted 9 lines

package/radlib/Config.in line 18

Quoted 3 lines
package/radlib/Config.in new file +3 -0
+       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.

Quoted 5 lines

package/radlib/Config.in line 9

Quoted 2 lines
package/radlib/Config.in new file +2 -0
+
+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.

Quoted 9 lines

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!

Quoted 5 lines

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.

Quoted 10 lines

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

Quoted 2 lines

This is the fairly canonical way of handling optional dependencies in Buildroot packages. We use this construct all over the place.

ok - will do.

Quoted 6 lines

package/radlib/Config.in line 12

Quoted 3 lines
package/radlib/Config.in new file +3 -0
+
+$(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

#3 lore raw

PATCH 1/1 package/radlib: New package

Hello,

Quoted 4 lines · Ray
Quoted 2 lines

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

Quoted 9 lines
Quoted 5 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
+-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

Quoted 6 lines
Quoted 1 line

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.

Quoted 7 lines
Quoted 4 lines
Quoted 3 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.

Absolutely. Changing both Makefile.am and Makefile.in doesn't make much sense.

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
package/radlib/Config.in new file +0 -1
new file mode 100644
index 0000000..2ca1617
--- /dev/null
Quoted 8 lines

package/radlib/Config.in line 4

Quoted 6 lines

+++ b/package/radlib/Config.in

package/radlib/Config.in new file +4 -0
@@ -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 ?

Quoted 3 lines

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.

Quoted 7 lines
Quoted 5 lines

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.

Quoted 6 lines
Quoted 4 lines

package/radlib/Config.in line 5

Quoted 1 line
package/radlib/Config.in new file +1 -0
+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.

Quoted 2 lines

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

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

PATCH 1/1 package/radlib: New package

Hi,

Quoted 6 lines · Thomas Petazzoni

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

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

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.

Quoted 8 lines
Quoted 4 lines

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.

Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
+-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

Quoted 8 lines
Quoted 1 line

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

Quoted 3 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.

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 6 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.

Quoted 7 lines
Quoted 3 lines

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,

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines

package/radlib/Config.in line 4

Quoted 12 lines
package/radlib/Config.in new file +4 -0
new file mode 100644
index 0000000..2ca1617
@@ -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 ?

Quoted 8 lines
Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 14 lines
Quoted 9 lines
Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 2 lines

Are you in contact with upstream ?

No - will send em a patch, once we are done here.

Quoted 3 lines

Thanks,

Thomas

Thanks for the feedback,

Ray K