tcfagent: new package

From: Norbert Lange <[email protected]>

Add tcfpackage which contains a service "tcf-agent"

Signed-off-by Norbert Lange <[email protected]>

---
Changes v2 -> v3:
- fix: init file still started the tool daemonized
- use same email for all signatures

Changes v1 -> v2:
- added myself to DEVELOPERS
- fixes to the init service, now uses common sheme to generate PID files
- fixed systemd unit
- renamed target executable directly in CMakeList
- allow cmake build without existing C++ compiler
- add patches to make sources compile with musl
- allow compilation with other architectures than x86_64
- some more small details suggested by Thomas Petazzoni

Hello Norbert,

Quoted 10 lines · Norbert Lange

From: Norbert Lange <[email protected]>

Add tcfpackage which contains a service "tcf-agent"

Signed-off-by Norbert Lange <[email protected]>

---
Changes v2 -> v3:
- fix: init file still started the tool daemonized
- use same email for all signatures

Quoted 11 lines

Changes v1 -> v2:
- added myself to DEVELOPERS
- fixes to the init service, now uses common sheme to generate PID files
- fixed systemd unit
- renamed target executable directly in CMakeList
- allow cmake build without existing C++ compiler
- add patches to make sources compile with musl
- allow compilation with other architectures than x86_64
- some more small details suggested by Thomas Petazzoni

It would be nice if you can create an account on
patchwork and mark the former version as Superseded when submitting a new version.

Thanks,

Best regards
Marcus Folkesson

Can do, but maybe scrap that part of your documentation:
"You can also add the --in-reply-to <message-id> option when submitting a patch to the mailing list. The id of the mail to reply to can be found under the "Message Id" tag on patchwork. The advantage of in-reply-to is that patchwork will automatically mark the previous version of the patch as superseded."

From: Norbert Lange <[email protected]>

Add tcfpackage which contains a service "tcf-agent"

Signed-off-by Norbert Lange <[email protected]>

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch new file +2 -0

new file mode 100644
index 0000000000..954c0d5fe3
@@ -0,0 +1,48 @@
+From 4c5fb079bc06ef04d09955eebe0d93d5ebfd4cdf Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +12 -0

+
+musl was not covered so far, and this library does not define a
+macro for detection.
+unless glibc is detected, a canonicalize_file_name implementation
+will be provided.
+
+Signed-off-by: Norbert Lange <[email protected]>
+---
+ agent/tcf/framework/mdep.c | 2 +-
+ agent/tcf/framework/mdep.h | 2 +-
+ 2 files changed, 2 insertions(+), 2 deletions(-)
+

package/tcfagent/Config.in new file +15 -0

new file mode 100644
index 0000000000..81affce6c1
@@ -0,0 +1,25 @@
+config BR2_PACKAGE_TCFAGENT
+	bool "tcfagent"
+	depends on BR2_TOOLCHAIN_HAS_THREADS
+	depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be \
+			|| BR2_i386 || BR2_x86_64 \
+			|| BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
+	select BR2_PACKAGE_UTIL_LINUX
+	select BR2_PACKAGE_UTIL_LINUX_LIBUUID
+	help
+	  Target Communication Framework Agent is an example application
+	  using the Target Communication Framework Library.
+
+	  Target Communication Framework is universal, extensible, simple,
+	  lightweight, vendor agnostic framework for tools and targets to
+	  communicate for purpose of debugging, profiling, code patching

package/tcfagent/S55tcfagent new file +3 -0

new file mode 100755
index 0000000000..2243b5a4c7
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+DAEMON_PATH=/usr/sbin/tcf-agent

package/tcfagent/S55tcfagent new file +8 -0

+# supported arch strings: a64 arm i386 i686 powerpc ppc64 x86_64
+_TCFAGENT_BRARCH = $(patsubst "%",%,$(BR2_ARCH))
+ifneq ($(filter arm armeb,$(_TCFAGENT_BRARCH)),)
+ 	TCFAGENT_CONF_OPTS += -DTCF_MACHINE=arm
+else ifneq ($(filter aarch64 aarch64_be,$(_TCFAGENT_BRARCH)),)
+ 	TCFAGENT_CONF_OPTS += -DTCF_MACHINE=a64
+else ifneq ($(filter i386 i486 i586,$(_TCFAGENT_BRARCH)),)
+	TCFAGENT_CONF_OPTS += -DTCF_MACHINE=i386

Hello,

Quoted 5 lines · Norbert Lange

From: Norbert Lange <[email protected]>

Add tcfpackage which contains a service "tcf-agent"

Signed-off-by Norbert Lange <[email protected]>

This is getting really good. I was about to apply, but spotted a few things that I really would like to be improved before applying. See below.

DEVELOPERS N: No? Rubinstein <[email protected]> · line 1241

Quoted 2 lines

DEVELOPERS +2 -0

+N:   Norbert Lange <[email protected]>
+F:   package/tcfagent

Add a final / here, to be consistent with all other entries in that file.

Quoted 12 lines

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch new file +6 -0

new file mode 100644
index 0000000000..954c0d5fe3
@@ -0,0 +1,48 @@
+From 4c5fb079bc06ef04d09955eebe0d93d5ebfd4cdf Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Mon, 4 Dec 2017 10:56:45 +0100
+Subject: [PATCH] agent: add install target to the CMakeLists
+
+It is common for CMake packages to make sure that 'make install'

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch line 12

Quoted 6 lines

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch new file +6 -0

+works properly, and that's what most users expect.
+
+More specifically, build systems such as Buildroot also expect
+'make install' to do the right thing for CMake-based packages
+
+Signed-off-by: Norbert Lange <[email protected]>

Did you submit this patch upstream ?

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch line 4

Quoted 10 lines

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch new file +4 -0

new file mode 100644
index 0000000000..16b748926f
@@ -0,0 +1,66 @@
+From 6407e3961dba749c8e3a0ea4ae8991154520364f Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Fri, 1 Dec 2017 13:15:50 +0100
+Subject: [PATCH 1/2] linux: remove explicit uses of __ptrace_request

Please generate patches with "git format-patch -N". Indeed the PATCH 1/2 doesn't make much sense here, since it's patch 0002 :)

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch line 8

Quoted 4 lines

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch new file +4 -0

+
+This type is not to be used directly, and with musl it wont build
+
+Signed-off-by: Norbert Lange <[email protected]>

Did you submit this upstream?

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch line 5

Quoted 11 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +5 -0

new file mode 100644
index 0000000000..86dc2f37a1
@@ -0,0 +1,46 @@
+From 58cc4092495acdc0b4be8e23c3964d47989a59dd Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Fri, 1 Dec 2017 13:34:08 +0100
+Subject: [PATCH 2/2] linux: provide canonicalize_file_name for all c libs
+ except glibc

Please use git format-patch -N.

Quoted 20 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +20 -0

+
+musl was not covered so far, and this library does not define a
+macro for detection.
+unless glibc is detected, a canonicalize_file_name implementation
+will be provided.
+
+Signed-off-by: Norbert Lange <[email protected]>
+---
+ agent/tcf/framework/mdep.c | 2 +-
+ agent/tcf/framework/mdep.h | 2 +-
+ 2 files changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/agent/tcf/framework/mdep.c b/agent/tcf/framework/mdep.c
+index cf5771e5..530a8017 100644
+--- a/agent/tcf/framework/mdep.c
++++ b/agent/tcf/framework/mdep.c
+@@ -1086,7 +1086,7 @@ char * canonicalize_file_name(const char * path) {
+     return strdup(res);
+ }
+

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch line 27

Quoted 2 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +2 -0

+-#elif defined(__UCLIBC__)
++#elif defined(__UCLIBC__) || !defined(__GLIBC_)

This is really not the proper way to do that. Could you instead replace all this mess by a CheckFunctionExists(canonicalize_file_name) check in the CMakeLists.txt, and then use the result of this test in the code to decide whether it should provide its own implementation of canonicalize_file_name or not.

With such a change, the patch can be submitted upstream.

Quoted 23 lines

package/tcfagent/Config.in new file +15 -0

new file mode 100644
index 0000000000..81affce6c1
@@ -0,0 +1,25 @@
+config BR2_PACKAGE_TCFAGENT
+     bool "tcfagent"
+     depends on BR2_TOOLCHAIN_HAS_THREADS
+     depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be \
+                     || BR2_i386 || BR2_x86_64 \
+                     || BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
+     select BR2_PACKAGE_UTIL_LINUX
+     select BR2_PACKAGE_UTIL_LINUX_LIBUUID
+     help
+       Target Communication Framework Agent is an example application
+       using the Target Communication Framework Library.
+
+       Target Communication Framework is universal, extensible, simple,
+       lightweight, vendor agnostic framework for tools and targets to
+       communicate for purpose of debugging, profiling, code patching

package/tcfagent/Config.in line 25

Quoted 10 lines

package/tcfagent/Config.in new file +10 -0

+       and other device software development needs.
+
+       tcf-agent is a daemon, which provides TCF services that can be
+       used by local and remote clients.
+
+comment "tcfagent needs a toolchain w/ threads"
+     depends on !BR2_TOOLCHAIN_HAS_THREADS
+     depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be \
+             || BR2_i386 || BR2_x86_64 \
+             || BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le

For the architecture, I'd like to have something a bit better to:

1. Avoid duplicating the list of architectures between the main option and the Config.in comment

2. Avoid the matching on the architecture in the .mk file.

So, something like this

config BR2_PACKAGE_TCFAGENT_ARCH

        string
        default "arm" if BR2_arm || BR2_armeb
        default "a64" if BR2_aarch64 || BR2_aarch64be
        default "i386" if ...
        ...

config BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

        bool
        default y if BR2_PACKAGE_TCFAGENT_ARCH != ""

config BR2_PACKAGE_TCFAGENT
bool "tcfagent"
depends on BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

comment "tcfagent needs ..."
depends on BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

Quoted 12 lines

package/tcfagent/S55tcfagent new file +3 -0

new file mode 100755
index 0000000000..2243b5a4c7
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+DAEMON_PATH=/usr/sbin/tcf-agent

package/tcfagent/S55tcfagent line 13

Quoted 10 lines

package/tcfagent/S55tcfagent new file +10 -0

+DAEMON_NAME=tcf-agent
+DAEMON_ARGS="-L- -l0"
+
+PIDFILE=/var/run/$DAEMON_NAME.pid
+[ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME
+
+start() {
+      printf "Starting $DAEMON_NAME: "
+      start-stop-daemon -S -o -q -p $PIDFILE -m -b \
+      -x $DAEMON_PATH -- $DAEMON_ARGS

Continuation line should be indented compared to the first line.

package/tcfagent/S55tcfagent line 15

Quoted 2 lines

package/tcfagent/S55tcfagent new file +2 -0

+
+      [ $? = 0 ] && echo "OK" || echo "FAIL"

You indent with 8 spaces here, it should be one tab.

package/tcfagent/S55tcfagent line 23

Quoted 8 lines

package/tcfagent/S55tcfagent new file +8 -0

+}
+
+stop() {
+  printf "Stopping $DAEMON_NAME: "
+  start-stop-daemon -K -o -q -p $PIDFILE \
+        -x $DAEMON_PATH
+
+  [ $? = 0 ] && echo "OK" || echo "FAIL"

You intend with two spaces here, not consitent with what is done above.

package/tcfagent/S55tcfagent line 35

Quoted 12 lines

package/tcfagent/S55tcfagent new file +12 -0

+}
+
+case "$1" in
+    start)
+  start
+  ;;
+    stop)
+  stop
+  ;;
+    restart|reload)
+  stop
+  start

Indentation is not good here.

Quoted 8 lines

package/tcfagent/S55tcfagent new file +8 -0

+# supported arch strings: a64 arm i386 i686 powerpc ppc64 x86_64
+_TCFAGENT_BRARCH = $(patsubst "%",%,$(BR2_ARCH))
+ifneq ($(filter arm armeb,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=arm
+else ifneq ($(filter aarch64 aarch64_be,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=a64
+else ifneq ($(filter i386 i486 i586,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=i386

package/tcfagent/S55tcfagent

Quoted 10 lines

package/tcfagent/S55tcfagent new file +10 -0

+else ifneq ($(filter i686 x86_64 powerpc,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=$(_TCFAGENT_BRARCH)
+else ifneq ($(filter powerpc64 powerpc64le,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=ppc64
+# only supported in trunk as of 1.5_oxygen
+# else ifneq ($(filter microblaze microblazeel,$(_TCFAGENT_BRARCH)),)
+#    TCFAGENT_CONF_OPTS += -DTCF_MACHINE=microblaze
+else
+$(warning "Unsupported architecture $(_TCFAGENT_BRARCH)")
+endif

The warning is useless (the package cannot be selected on unsupported architectures) and actually harmful because it will be printed on configurations using unsupported architectures: all .mk files are parsed, even when the package is not enabled, and the $(warning ...) call is evaluated at the time of the .mk file parsing.

Replace all this by:

TCFAGENT_ARCH = $(call qstrip,$(BR2_PACKAGE_TCFAGENT_ARCH))

package/tcfagent/S55tcfagent

Quoted 2 lines

package/tcfagent/S55tcfagent new file +2 -0

+
+_TCFAGENT_BRARCH =

Unneeded variable.

Could you rework your submission according to this review, and submit an updated version ?

Thanks a lot!

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Target architecture

Dependency symbol: BR2_powerpc, BR2_mips, ? (see arch/Config.in) Comment string: no comment to be added

2017-12-13 14:49 GMT+01:00 Thomas Petazzoni
<[email protected]>:

Quoted 31 lines

Hello,

Quoted 5 lines · Norbert Lange

From: Norbert Lange <[email protected]>

Add tcfpackage which contains a service "tcf-agent"

Signed-off-by Norbert Lange <[email protected]>

This is getting really good. I was about to apply, but spotted a few things that I really would like to be improved before applying. See below.

DEVELOPERS N: No? Rubinstein <[email protected]> · line 1241

Quoted 2 lines

DEVELOPERS +2 -0

+N:   Norbert Lange <[email protected]>
+F:   package/tcfagent

Add a final / here, to be consistent with all other entries in that file.

Quoted 12 lines

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch new file +6 -0

new file mode 100644
index 0000000000..954c0d5fe3
@@ -0,0 +1,48 @@
+From 4c5fb079bc06ef04d09955eebe0d93d5ebfd4cdf Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Mon, 4 Dec 2017 10:56:45 +0100
+Subject: [PATCH] agent: add install target to the CMakeLists
+
+It is common for CMake packages to make sure that 'make install'

Quoted 8 lines

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch line 12

Quoted 6 lines

package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch new file +6 -0

+works properly, and that's what most users expect.
+
+More specifically, build systems such as Buildroot also expect
+'make install' to do the right thing for CMake-based packages
+
+Signed-off-by: Norbert Lange <[email protected]>

Did you submit this patch upstream ?

Only a failed attempt so far, gonna try again

Quoted 58 lines

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch line 4

Quoted 10 lines

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch new file +4 -0

new file mode 100644
index 0000000000..16b748926f
@@ -0,0 +1,66 @@
+From 6407e3961dba749c8e3a0ea4ae8991154520364f Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Fri, 1 Dec 2017 13:15:50 +0100
+Subject: [PATCH 1/2] linux: remove explicit uses of __ptrace_request

Please generate patches with "git format-patch -N". Indeed the PATCH 1/2 doesn't make much sense here, since it's patch 0002 :)

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch line 8

Quoted 4 lines

package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch new file +4 -0

+
+This type is not to be used directly, and with musl it wont build
+
+Signed-off-by: Norbert Lange <[email protected]>

Did you submit this upstream?

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch line 5

Quoted 11 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +5 -0

new file mode 100644
index 0000000000..86dc2f37a1
@@ -0,0 +1,46 @@
+From 58cc4092495acdc0b4be8e23c3964d47989a59dd Mon Sep 17 00:00:00 2001
+From: Norbert Lange <[email protected]>
+Date: Fri, 1 Dec 2017 13:34:08 +0100
+Subject: [PATCH 2/2] linux: provide canonicalize_file_name for all c libs
+ except glibc

Please use git format-patch -N.

Quoted 20 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +20 -0

+
+musl was not covered so far, and this library does not define a
+macro for detection.
+unless glibc is detected, a canonicalize_file_name implementation
+will be provided.
+
+Signed-off-by: Norbert Lange <[email protected]>
+---
+ agent/tcf/framework/mdep.c | 2 +-
+ agent/tcf/framework/mdep.h | 2 +-
+ 2 files changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/agent/tcf/framework/mdep.c b/agent/tcf/framework/mdep.c
+index cf5771e5..530a8017 100644
+--- a/agent/tcf/framework/mdep.c
++++ b/agent/tcf/framework/mdep.c
+@@ -1086,7 +1086,7 @@ char * canonicalize_file_name(const char * path) {
+     return strdup(res);
+ }
+

Quoted 8 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch line 27

Quoted 2 lines

package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch new file +2 -0

+-#elif defined(__UCLIBC__)
++#elif defined(__UCLIBC__) || !defined(__GLIBC_)

This is really not the proper way to do that. Could you instead replace all this mess by a CheckFunctionExists(canonicalize_file_name) check in the CMakeLists.txt, and then use the result of this test in the code to decide whether it should provide its own implementation of canonicalize_file_name or not.

Yes this is a mess, but its not MY mess. I believe this to be the correct way for a patch in buildroot. Changing the multiple buildsystems is not something I want to do, there seems to be a consistency to not put any complex logic outside the source-files.

Quoted 2 lines

With such a change, the patch can be submitted upstream.

I will (and did try) to commit the patches upstream, but I don`t believe this to be an issue and a big overhaul of multiple buildsystems would take longer and has a bigger chance of being rejected.

Quoted 54 lines

Quoted 23 lines

package/tcfagent/Config.in new file +15 -0

new file mode 100644
index 0000000000..81affce6c1
@@ -0,0 +1,25 @@
+config BR2_PACKAGE_TCFAGENT
+     bool "tcfagent"
+     depends on BR2_TOOLCHAIN_HAS_THREADS
+     depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be \
+                     || BR2_i386 || BR2_x86_64 \
+                     || BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le
+     select BR2_PACKAGE_UTIL_LINUX
+     select BR2_PACKAGE_UTIL_LINUX_LIBUUID
+     help
+       Target Communication Framework Agent is an example application
+       using the Target Communication Framework Library.
+
+       Target Communication Framework is universal, extensible, simple,
+       lightweight, vendor agnostic framework for tools and targets to
+       communicate for purpose of debugging, profiling, code patching

package/tcfagent/Config.in line 25

Quoted 10 lines

package/tcfagent/Config.in new file +10 -0

+       and other device software development needs.
+
+       tcf-agent is a daemon, which provides TCF services that can be
+       used by local and remote clients.
+
+comment "tcfagent needs a toolchain w/ threads"
+     depends on !BR2_TOOLCHAIN_HAS_THREADS
+     depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_aarch64_be \
+             || BR2_i386 || BR2_x86_64 \
+             || BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le

For the architecture, I'd like to have something a bit better to:

1. Avoid duplicating the list of architectures between the main option and the Config.in comment

2. Avoid the matching on the architecture in the .mk file.

So, something like this

config BR2_PACKAGE_TCFAGENT_ARCH

        string
        default "arm" if BR2_arm || BR2_armeb
        default "a64" if BR2_aarch64 || BR2_aarch64be
        default "i386" if ...
        ...

config BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

        bool
        default y if BR2_PACKAGE_TCFAGENT_ARCH != ""

Quoted 6 lines

config BR2_PACKAGE_TCFAGENT
bool "tcfagent"
depends on BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

comment "tcfagent needs ..."
depends on BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS

Ok, Config is not something I am deeply familiar with.

Quoted 85 lines

Quoted 12 lines

package/tcfagent/S55tcfagent new file +3 -0

new file mode 100755
index 0000000000..2243b5a4c7
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+DAEMON_PATH=/usr/sbin/tcf-agent

package/tcfagent/S55tcfagent line 13

Quoted 10 lines

package/tcfagent/S55tcfagent new file +10 -0

+DAEMON_NAME=tcf-agent
+DAEMON_ARGS="-L- -l0"
+
+PIDFILE=/var/run/$DAEMON_NAME.pid
+[ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME
+
+start() {
+      printf "Starting $DAEMON_NAME: "
+      start-stop-daemon -S -o -q -p $PIDFILE -m -b \
+      -x $DAEMON_PATH -- $DAEMON_ARGS

Continuation line should be indented compared to the first line.

package/tcfagent/S55tcfagent line 15

Quoted 2 lines

package/tcfagent/S55tcfagent new file +2 -0

+
+      [ $? = 0 ] && echo "OK" || echo "FAIL"

You indent with 8 spaces here, it should be one tab.

package/tcfagent/S55tcfagent line 23

Quoted 8 lines

package/tcfagent/S55tcfagent new file +8 -0

+}
+
+stop() {
+  printf "Stopping $DAEMON_NAME: "
+  start-stop-daemon -K -o -q -p $PIDFILE \
+        -x $DAEMON_PATH
+
+  [ $? = 0 ] && echo "OK" || echo "FAIL"

You intend with two spaces here, not consitent with what is done above.

package/tcfagent/S55tcfagent line 35

Quoted 12 lines

package/tcfagent/S55tcfagent new file +12 -0

+}
+
+case "$1" in
+    start)
+  start
+  ;;
+    stop)
+  stop
+  ;;
+    restart|reload)
+  stop
+  start

Indentation is not good here.

Quoted 8 lines

package/tcfagent/S55tcfagent new file +8 -0

+# supported arch strings: a64 arm i386 i686 powerpc ppc64 x86_64
+_TCFAGENT_BRARCH = $(patsubst "%",%,$(BR2_ARCH))
+ifneq ($(filter arm armeb,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=arm
+else ifneq ($(filter aarch64 aarch64_be,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=a64
+else ifneq ($(filter i386 i486 i586,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=i386

package/tcfagent/S55tcfagent

Quoted 10 lines

package/tcfagent/S55tcfagent new file +10 -0

+else ifneq ($(filter i686 x86_64 powerpc,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=$(_TCFAGENT_BRARCH)
+else ifneq ($(filter powerpc64 powerpc64le,$(_TCFAGENT_BRARCH)),)
+     TCFAGENT_CONF_OPTS += -DTCF_MACHINE=ppc64
+# only supported in trunk as of 1.5_oxygen
+# else ifneq ($(filter microblaze microblazeel,$(_TCFAGENT_BRARCH)),)
+#    TCFAGENT_CONF_OPTS += -DTCF_MACHINE=microblaze
+else
+$(warning "Unsupported architecture $(_TCFAGENT_BRARCH)")
+endif

The warning is useless (the package cannot be selected on unsupported architectures) and actually harmful because it will be printed on configurations using unsupported architectures: all .mk files are parsed, even when the package is not enabled, and the $(warning ...) call is evaluated at the time of the .mk file parsing.

Replace all this by:

TCFAGENT_ARCH = $(call qstrip,$(BR2_PACKAGE_TCFAGENT_ARCH))

Quoted 7 lines

package/tcfagent/S55tcfagent

Quoted 2 lines

package/tcfagent/S55tcfagent new file +2 -0

+
+_TCFAGENT_BRARCH =

Unneeded variable.

Could you rework your submission according to this review, and submit an updated version ?

Sure, might take a while.

Quoted 8 lines

Thanks a lot!

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Norbert LAnge

before I do the bureaucratic work and more testing, there is some mixed information from you and the docs.

To quote the manual:

Quoted 4 lines

Target architecture

Dependency symbol: BR2_powerpc, BR2_mips, ? (see arch/Config.in) Comment string: no comment to be added

config BR2_PACKAGE_TCFAGENT
bool "tcfagent"
depends on BR2_TOOLCHAIN_HAS_THREADS
depends on BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS
select BR2_PACKAGE_UTIL_LINUX
select BR2_PACKAGE_UTIL_LINUX_LIBUUID
help
Target Communication Framework Agent is an example application using the Target Communication Framework Library.

Target Communication Framework is universal, extensible, simple, lightweight, vendor agnostic framework for tools and targets to communicate for purpose of debugging, profiling, code patching and other device software development needs.

tcf-agent is a daemon, which provides TCF services that can be used by local and remote clients.

config BR2_PACKAGE_TCFAGENT_ARCH
string

default "arm"        if BR2_arm || BR2_armeb
default "a64"        if BR2_aarch64 || BR2_aarch64be
default "i686"       if BR2_i386 && BR2_ARCH="i686"
default "i386"       if BR2_i386
default "x86_64"     if BR2_x86_64
default "powerpc"    if BR2_powerpc
default "ppc64"      if BR2_powerpc64 || BR2_powerpc64le
#   Supported with 1.6, enable when released
# default "microblaze" if BR2_microblaze || BR2_microblazeel

config BR2_PACKAGE_TCFAGENT_ARCH_SUPPORTS
bool
default y if BR2_PACKAGE_TCFAGENT_ARCH != ""

comment "tcfagent needs a toolchain w/ threads"

depends on !BR2_TOOLCHAIN_HAS_THREADS