[1/1] Add TCF Agent package

Message ID 20171114145442.16734-1-norbert.lange@andritz.com
State Changes Requested
Headers show
Series
  • [1/1] Add TCF Agent package
Related show

Commit Message

Norbert Lange Nov. 14, 2017, 2:54 p.m.
Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
---
 package/Config.in                                  |  1 +
 ...add-CMake-install-target-for-agent-binary.patch | 35 ++++++++++++++++
 package/tcfagent/Config.in                         | 16 ++++++++
 package/tcfagent/S55tcfagent                       | 47 ++++++++++++++++++++++
 package/tcfagent/tcfagent.hash                     |  4 ++
 package/tcfagent/tcfagent.mk                       | 38 +++++++++++++++++
 package/tcfagent/tcfagent.service                  | 10 +++++
 7 files changed, 151 insertions(+)
 create mode 100644 package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
 create mode 100644 package/tcfagent/Config.in
 create mode 100755 package/tcfagent/S55tcfagent
 create mode 100644 package/tcfagent/tcfagent.hash
 create mode 100644 package/tcfagent/tcfagent.mk
 create mode 100644 package/tcfagent/tcfagent.service

Comments

Thomas Petazzoni Nov. 29, 2017, 9:45 p.m. | #1
Hello,

On Tue, 14 Nov 2017 15:54:42 +0100, Norbert Lange wrote:
> Signed-off-by: Norbert Lange <norbert.lange@andritz.com>

Thanks a lot for this contribution! This looks pretty good, there are
just a few things that should be improved. See below for my comments.

First a pedantic comment: the commit title should be "tcf-agent: new
package".

> ---
>  package/Config.in                                  |  1 +
>  ...add-CMake-install-target-for-agent-binary.patch | 35 ++++++++++++++++
>  package/tcfagent/Config.in                         | 16 ++++++++
>  package/tcfagent/S55tcfagent                       | 47 ++++++++++++++++++++++
>  package/tcfagent/tcfagent.hash                     |  4 ++
>  package/tcfagent/tcfagent.mk                       | 38 +++++++++++++++++
>  package/tcfagent/tcfagent.service                  | 10 +++++

Please add yourself to the DEVELOPERS file. This way, we will know who
to contact when there are issues with this package :-)

> diff --git a/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
> new file mode 100644
> index 0000000000..bb9c334d91
> --- /dev/null
> +++ b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
> @@ -0,0 +1,35 @@
> +From 9140c630085833acfe565f27195a876c6656f068 Mon Sep 17 00:00:00 2001
> +From: Norbert Lange <norbert.lange@andritz.com>
> +Date: Mon, 30 Oct 2017 16:22:40 +0100
> +Subject: [PATCH] add CMake install target for agent binary
> +
> +allows use in automated buildsystems like Buildroot

Buildroot could technically do without it, but it would be annoying
indeed. Perhaps a better phrasing would be: "It is common for CMake
packages to make sure that 'make install' 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".

Also, please add your Signed-off-by line to this patch, and submit it
upstream for inclusion.

> + agent/CMakeLists.txt | 5 +++++
> + 1 file changed, 5 insertions(+)
> +
> +diff --git a/agent/CMakeLists.txt b/agent/CMakeLists.txt
> +index aef15b96..7868987a 100644
> +--- a/agent/CMakeLists.txt
> ++++ b/agent/CMakeLists.txt
> +@@ -1,6 +1,7 @@
> + # -*- cmake -*-
> + 
> + cmake_minimum_required(VERSION 2.8)
> ++include(GNUInstallDirs)

I'm not super familiar with CMake, but why is this needed?

> diff --git a/package/tcfagent/Config.in b/package/tcfagent/Config.in
> new file mode 100644
> index 0000000000..1374fdf88d
> --- /dev/null
> +++ b/package/tcfagent/Config.in
> @@ -0,0 +1,16 @@
> +config BR2_PACKAGE_TCFAGENT
> +	bool "tcfagent"
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # snmp++

You're not using snmp++, so the comment here doesn't make much sense.

> +	select BR2_PACKAGE_UTIL_LINUX_LIBUUID

You should also select BR2_PACKAGE_UTIL_LINUX then.

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

Please wrap lines at 72 characters (where the first tab counts for 8
characters).

> +comment "tcfagent needs a toolchain w/ threads, C++, dynamic library"
> +	depends on !BR2_TOOLCHAIN_HAS_THREADS

So, the message says that you need threads, C++ and dynamic library...
but the dependency is only about thread. Which one is correct ? :-)

> diff --git a/package/tcfagent/S55tcfagent b/package/tcfagent/S55tcfagent
> new file mode 100755
> index 0000000000..b022d5eb92
> --- /dev/null
> +++ b/package/tcfagent/S55tcfagent
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +#
> +# Start tcf-agent....

Not very useful comment.

> +#
> +
> +DAEMON_PATH=/usr/sbin/tcf-agent
> +DAEMON_NAME=tcf-agent
> +DAEMON_ARGS="-d -L- -l0"
> +
> +PIDFILE=/var/run/$DAEMON_NAME.pid
> +[ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME

Positive logic here would be better:

[ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME

> +start() {
> +      printf "Starting $DAEMON_NAME: "
> +      start-stop-daemon -S -q -o \
> +      -x $DAEMON_PATH -- $DAEMON_ARGS &&
> +        PPID=$(pidof $(basename $DAEMON_PATH)) &&
> +        echo $PPID > $PIDFILE

What you're doing here for the pidfile is quite weird. Why don't you
let start-stop-daemon create the PID file in the first place ? Is it
because tcf-agent daemonizes itself ? If so, I see two options:

 - Ask tcf-agent to not daemonize itself (sometimes there's a -f or
   --foreground option)

 - Ask tcf-agent to create the PID file if it supports that.

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

Why the HUP signal ?

> +        -x $DAEMON_PATH &&
> +        rm -f $PIDFILE

We generally don't remove the PID file.

> +  [ $? = 0 ] && echo "OK" || echo "FAIL"
> +}
> +
> +case "$1" in
> +    start)
> +  start
> +  ;;
> +    stop)
> +  stop
> +  ;;
> +    restart|reload)
> +  stop
> +  start
> +  ;;
> +  *)
> +  echo "Usage: $0 {start|stop|restart}"
> +  exit 1
> +esac
> +
> +exit $?

This last "exit" is not needed.

> diff --git a/package/tcfagent/tcfagent.hash b/package/tcfagent/tcfagent.hash
> new file mode 100644
> index 0000000000..ffed5dcb19
> --- /dev/null
> +++ b/package/tcfagent/tcfagent.hash
> @@ -0,0 +1,4 @@
> +# Locally computed:
> +sha256  47d34c0778aa8b9e2c26132c9bb03d643bfb8e44d03ce862d06f2f93edcb63ae  org.eclipse.tcf.agent-1.3.0.tar.gz
> +sha256  34188fd2daeadf6574071f5004f9a7a55b3ac73efe9db203a01559ee1013b2db  org.eclipse.tcf.agent-1.4_neon.tar.gz
> +sha256  4b6c757e2bed92a0a791d0687425d462c974abe4c79f80e27e362fdaa59107f5  org.eclipse.tcf.agent-1.5_oxygen.tar.gz

Please keep the hash only for the current version being used.

However, you could add the hash for the license file.

> diff --git a/package/tcfagent/tcfagent.mk b/package/tcfagent/tcfagent.mk
> new file mode 100644
> index 0000000000..30bd366137
> --- /dev/null
> +++ b/package/tcfagent/tcfagent.mk
> @@ -0,0 +1,38 @@
> +################################################################################
> +#
> +# TCFAGENT
> +#
> +################################################################################
> +
> +TCFAGENT_VERSION = 1.5_oxygen
> +# the tar.xz link was broken the time this file got authored
> +TCFAGENT_SOURCE = org.eclipse.tcf.agent-$(TCFAGENT_VERSION).tar.gz
> +TCFAGENT_SITE = http://git.eclipse.org/c/tcf/org.eclipse.tcf.agent.git/snapshot
> +# see https://wiki.spdx.org/view/Legal_Team/License_List/Licenses_Under_Consideration

Thanks for adding this explanation, definitely useful!

> +TCFAGENT_LICENSE = BSD-3-Clause
> +TCFAGENT_LICENSE_FILES = agent/edl-v10.html
> +
> +TCFAGENT_DEPENDENCIES = util-linux
> +TCFAGENT_SUBDIR = agent
> +TCFAGENT_CONF_OPTS = -DBUILD_SHARED_LIBS=Off

Why do you disable shared library build ?

> +
> +define TCFAGENT_RENAME_AGENT
> +	mv $(TARGET_DIR)/usr/bin/agent $(TARGET_DIR)/usr/sbin/tcf-agent
> +endef
> +
> +TCFAGENT_POST_INSTALL_TARGET_HOOKS += TCFAGENT_RENAME_AGENT

This is a bit annoying because you have already overwritten the "agent"
file if there was one in $(TARGET_DIR)/usr/bin/agent. So perhaps it
should be patched in the upstream CMake build logic instead ?

> +
> +define TCFAGENT_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -D -m 644 package/tcfagent/tcfagent.service \
> +		$(TARGET_DIR)/usr/lib/systemd/system/tcfagent.service
> +	mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants
> +	ln -fs ../../../../usr/lib/systemd/system/tcfagent.service \
> +		$(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/tcfagent.service
> +endef
> +
> +define TCFAGENT_INSTALL_INIT_SYSV
> +	$(INSTALL) -D -m 755 package/tcfagent/S55tcfagent \
> +		$(TARGET_DIR)/etc/init.d/S55tcfagent
> +endef
> +
> +$(eval $(cmake-package))
> diff --git a/package/tcfagent/tcfagent.service b/package/tcfagent/tcfagent.service
> new file mode 100644
> index 0000000000..2f57fca3c8
> --- /dev/null
> +++ b/package/tcfagent/tcfagent.service
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=Target Communication Framework Agent
> +After=network.target
> +
> +[Service]
> +Type=forking
> +ExecStart=@SBINDIR@/tcf-agent -L- -l0

What is replacing @SBINDIR@ by the correct value? Note: I'm not super
familiar with systemd, so maybe it's an obvious functionality of
systemd. But I don't think I've already seen that in systemd units.

Could you look at the different issues I pointed out and submit an
updated version of your patch ?

Thanks a lot!

Thomas
Norbert Lange Nov. 30, 2017, 11:08 a.m. | #2
Hi,

-   [ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
This scheme is used to be able to execute that script with 'sh -e'
[ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME would
fail if there is no such file (which is no error)

Please tell me if I should still use the later line.

-   sysv init file
I am using HUP because TERM does not end the process, and their own
init script (agent/tcf/main/tcf-agent.init) uses HUP.

Fetching the PID this way is also taken from that file, the daemon
does do a fork. I tried running in foreground but that had some other
issues
which I dont recall right now (will test this some more). Would be
cleanest to patch in creating a pid file I think.

-   systemd unit file
Sorry, I really should've tested this, copied some boilerplate code
with placeholders.

-   GNUInstallDirs
allows common variable overrides for installation dirs, which is often
important for Windows/Mac to allow derivation from the unix-sheme

-   DBUILD_SHARED_LIBS=Off
Without this, a seperate library will installed, which has no use
other than the tcf-agent binary.
This library is an example implementation, its unlikely you would use
it unmodified, if it gets stable at some time I would create a
separate package libtcfagent
including the headers needed for development.
(Its also what you get when you use the plain Makefile)

-   drop CMake....
I am considering dropping the use of CMake (there s alot deduced
automatically, by changing the name you got to redo all of that
manually)
and using the plain Makefile, this seems like less hassle in the end,
but that means some additional stuff will be installed.
It there any common way to do this install step in an temp dir and
then (re)moving files before copying this into the final target fs?

The other points are clear.

Norbert

2017-11-29 22:45 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello,
>
> On Tue, 14 Nov 2017 15:54:42 +0100, Norbert Lange wrote:
>> Signed-off-by: Norbert Lange <norbert.lange@andritz.com>
>
> Thanks a lot for this contribution! This looks pretty good, there are
> just a few things that should be improved. See below for my comments.
>
> First a pedantic comment: the commit title should be "tcf-agent: new
> package".
>
>> ---
>>  package/Config.in                                  |  1 +
>>  ...add-CMake-install-target-for-agent-binary.patch | 35 ++++++++++++++++
>>  package/tcfagent/Config.in                         | 16 ++++++++
>>  package/tcfagent/S55tcfagent                       | 47 ++++++++++++++++++++++
>>  package/tcfagent/tcfagent.hash                     |  4 ++
>>  package/tcfagent/tcfagent.mk                       | 38 +++++++++++++++++
>>  package/tcfagent/tcfagent.service                  | 10 +++++
>
> Please add yourself to the DEVELOPERS file. This way, we will know who
> to contact when there are issues with this package :-)
>
>> diff --git a/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
>> new file mode 100644
>> index 0000000000..bb9c334d91
>> --- /dev/null
>> +++ b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
>> @@ -0,0 +1,35 @@
>> +From 9140c630085833acfe565f27195a876c6656f068 Mon Sep 17 00:00:00 2001
>> +From: Norbert Lange <norbert.lange@andritz.com>
>> +Date: Mon, 30 Oct 2017 16:22:40 +0100
>> +Subject: [PATCH] add CMake install target for agent binary
>> +
>> +allows use in automated buildsystems like Buildroot
>
> Buildroot could technically do without it, but it would be annoying
> indeed. Perhaps a better phrasing would be: "It is common for CMake
> packages to make sure that 'make install' 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".
>
> Also, please add your Signed-off-by line to this patch, and submit it
> upstream for inclusion.
>
>> + agent/CMakeLists.txt | 5 +++++
>> + 1 file changed, 5 insertions(+)
>> +
>> +diff --git a/agent/CMakeLists.txt b/agent/CMakeLists.txt
>> +index aef15b96..7868987a 100644
>> +--- a/agent/CMakeLists.txt
>> ++++ b/agent/CMakeLists.txt
>> +@@ -1,6 +1,7 @@
>> + # -*- cmake -*-
>> +
>> + cmake_minimum_required(VERSION 2.8)
>> ++include(GNUInstallDirs)
>
> I'm not super familiar with CMake, but why is this needed?
>
>> diff --git a/package/tcfagent/Config.in b/package/tcfagent/Config.in
>> new file mode 100644
>> index 0000000000..1374fdf88d
>> --- /dev/null
>> +++ b/package/tcfagent/Config.in
>> @@ -0,0 +1,16 @@
>> +config BR2_PACKAGE_TCFAGENT
>> +     bool "tcfagent"
>> +     depends on BR2_TOOLCHAIN_HAS_THREADS # snmp++
>
> You're not using snmp++, so the comment here doesn't make much sense.
>
>> +     select BR2_PACKAGE_UTIL_LINUX_LIBUUID
>
> You should also select BR2_PACKAGE_UTIL_LINUX then.
>
>> +     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.
>
> Please wrap lines at 72 characters (where the first tab counts for 8
> characters).
>
>> +comment "tcfagent needs a toolchain w/ threads, C++, dynamic library"
>> +     depends on !BR2_TOOLCHAIN_HAS_THREADS
>
> So, the message says that you need threads, C++ and dynamic library...
> but the dependency is only about thread. Which one is correct ? :-)
>
>> diff --git a/package/tcfagent/S55tcfagent b/package/tcfagent/S55tcfagent
>> new file mode 100755
>> index 0000000000..b022d5eb92
>> --- /dev/null
>> +++ b/package/tcfagent/S55tcfagent
>> @@ -0,0 +1,47 @@
>> +#!/bin/sh
>> +#
>> +# Start tcf-agent....
>
> Not very useful comment.
>
>> +#
>> +
>> +DAEMON_PATH=/usr/sbin/tcf-agent
>> +DAEMON_NAME=tcf-agent
>> +DAEMON_ARGS="-d -L- -l0"
>> +
>> +PIDFILE=/var/run/$DAEMON_NAME.pid
>> +[ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
>
> Positive logic here would be better:
>
> [ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME
>
>> +start() {
>> +      printf "Starting $DAEMON_NAME: "
>> +      start-stop-daemon -S -q -o \
>> +      -x $DAEMON_PATH -- $DAEMON_ARGS &&
>> +        PPID=$(pidof $(basename $DAEMON_PATH)) &&
>> +        echo $PPID > $PIDFILE
>
> What you're doing here for the pidfile is quite weird. Why don't you
> let start-stop-daemon create the PID file in the first place ? Is it
> because tcf-agent daemonizes itself ? If so, I see two options:
>
>  - Ask tcf-agent to not daemonize itself (sometimes there's a -f or
>    --foreground option)
>
>  - Ask tcf-agent to create the PID file if it supports that.
>
>> +      [ $? = 0 ] && echo "OK" || echo "FAIL"
>> +}
>> +
>> +stop() {
>> +  printf "Stopping $DAEMON_NAME: "
>> +  start-stop-daemon -K -o -s HUP -q -p $PIDFILE \
>
> Why the HUP signal ?
>
>> +        -x $DAEMON_PATH &&
>> +        rm -f $PIDFILE
>
> We generally don't remove the PID file.
>
>> +  [ $? = 0 ] && echo "OK" || echo "FAIL"
>> +}
>> +
>> +case "$1" in
>> +    start)
>> +  start
>> +  ;;
>> +    stop)
>> +  stop
>> +  ;;
>> +    restart|reload)
>> +  stop
>> +  start
>> +  ;;
>> +  *)
>> +  echo "Usage: $0 {start|stop|restart}"
>> +  exit 1
>> +esac
>> +
>> +exit $?
>
> This last "exit" is not needed.
>
>> diff --git a/package/tcfagent/tcfagent.hash b/package/tcfagent/tcfagent.hash
>> new file mode 100644
>> index 0000000000..ffed5dcb19
>> --- /dev/null
>> +++ b/package/tcfagent/tcfagent.hash
>> @@ -0,0 +1,4 @@
>> +# Locally computed:
>> +sha256  47d34c0778aa8b9e2c26132c9bb03d643bfb8e44d03ce862d06f2f93edcb63ae  org.eclipse.tcf.agent-1.3.0.tar.gz
>> +sha256  34188fd2daeadf6574071f5004f9a7a55b3ac73efe9db203a01559ee1013b2db  org.eclipse.tcf.agent-1.4_neon.tar.gz
>> +sha256  4b6c757e2bed92a0a791d0687425d462c974abe4c79f80e27e362fdaa59107f5  org.eclipse.tcf.agent-1.5_oxygen.tar.gz
>
> Please keep the hash only for the current version being used.
>
> However, you could add the hash for the license file.
>
>> diff --git a/package/tcfagent/tcfagent.mk b/package/tcfagent/tcfagent.mk
>> new file mode 100644
>> index 0000000000..30bd366137
>> --- /dev/null
>> +++ b/package/tcfagent/tcfagent.mk
>> @@ -0,0 +1,38 @@
>> +################################################################################
>> +#
>> +# TCFAGENT
>> +#
>> +################################################################################
>> +
>> +TCFAGENT_VERSION = 1.5_oxygen
>> +# the tar.xz link was broken the time this file got authored
>> +TCFAGENT_SOURCE = org.eclipse.tcf.agent-$(TCFAGENT_VERSION).tar.gz
>> +TCFAGENT_SITE = http://git.eclipse.org/c/tcf/org.eclipse.tcf.agent.git/snapshot
>> +# see https://wiki.spdx.org/view/Legal_Team/License_List/Licenses_Under_Consideration
>
> Thanks for adding this explanation, definitely useful!
>
>> +TCFAGENT_LICENSE = BSD-3-Clause
>> +TCFAGENT_LICENSE_FILES = agent/edl-v10.html
>> +
>> +TCFAGENT_DEPENDENCIES = util-linux
>> +TCFAGENT_SUBDIR = agent
>> +TCFAGENT_CONF_OPTS = -DBUILD_SHARED_LIBS=Off
>
> Why do you disable shared library build ?
>
>> +
>> +define TCFAGENT_RENAME_AGENT
>> +     mv $(TARGET_DIR)/usr/bin/agent $(TARGET_DIR)/usr/sbin/tcf-agent
>> +endef
>> +
>> +TCFAGENT_POST_INSTALL_TARGET_HOOKS += TCFAGENT_RENAME_AGENT
>
> This is a bit annoying because you have already overwritten the "agent"
> file if there was one in $(TARGET_DIR)/usr/bin/agent. So perhaps it
> should be patched in the upstream CMake build logic instead ?
>
>> +
>> +define TCFAGENT_INSTALL_INIT_SYSTEMD
>> +     $(INSTALL) -D -m 644 package/tcfagent/tcfagent.service \
>> +             $(TARGET_DIR)/usr/lib/systemd/system/tcfagent.service
>> +     mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants
>> +     ln -fs ../../../../usr/lib/systemd/system/tcfagent.service \
>> +             $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/tcfagent.service
>> +endef
>> +
>> +define TCFAGENT_INSTALL_INIT_SYSV
>> +     $(INSTALL) -D -m 755 package/tcfagent/S55tcfagent \
>> +             $(TARGET_DIR)/etc/init.d/S55tcfagent
>> +endef
>> +
>> +$(eval $(cmake-package))
>> diff --git a/package/tcfagent/tcfagent.service b/package/tcfagent/tcfagent.service
>> new file mode 100644
>> index 0000000000..2f57fca3c8
>> --- /dev/null
>> +++ b/package/tcfagent/tcfagent.service
>> @@ -0,0 +1,10 @@
>> +[Unit]
>> +Description=Target Communication Framework Agent
>> +After=network.target
>> +
>> +[Service]
>> +Type=forking
>> +ExecStart=@SBINDIR@/tcf-agent -L- -l0
>
> What is replacing @SBINDIR@ by the correct value? Note: I'm not super
> familiar with systemd, so maybe it's an obvious functionality of
> systemd. But I don't think I've already seen that in systemd units.
>
> Could you look at the different issues I pointed out and submit an
> updated version of your patch ?
>
> Thanks a lot!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Thomas Petazzoni Nov. 30, 2017, 12:50 p.m. | #3
Hello,

It would be nice if you could avoid top-posting. Replying below each
part of the e-mail you're answering is much more readable, and is the
common practice on the mailing lists of open-source projects.

On Thu, 30 Nov 2017 12:08:17 +0100, Norbert Lange wrote:
> Hi,
> 
> -   [ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
> This scheme is used to be able to execute that script with 'sh -e'
> [ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME would
> fail if there is no such file (which is no error)
> 
> Please tell me if I should still use the later line.

Interesting reason. Please keep your original proposal then. The
ejabberd package is already using this construct. However, all other
packages are using the && construct.

Unless Yann has a better suggestion for this ?

> -   sysv init file
> I am using HUP because TERM does not end the process, and their own
> init script (agent/tcf/main/tcf-agent.init) uses HUP.

OK. A comment in the shell script about why you're sending the HUP
signal would be nice.

> Fetching the PID this way is also taken from that file, the daemon
> does do a fork. I tried running in foreground but that had some other
> issues which I dont recall right now (will test this some more).
> Would be cleanest to patch in creating a pid file I think.

Yes, if you could test some more running the daemon in the background,
it would be nice.

> -   systemd unit file
> Sorry, I really should've tested this, copied some boilerplate code
> with placeholders.

No problem :)

> -   GNUInstallDirs
> allows common variable overrides for installation dirs, which is often
> important for Windows/Mac to allow derivation from the unix-sheme
> 
> -   DBUILD_SHARED_LIBS=Off
> Without this, a seperate library will installed, which has no use
> other than the tcf-agent binary.
> This library is an example implementation, its unlikely you would use
> it unmodified, if it gets stable at some time I would create a
> separate package libtcfagent
> including the headers needed for development.
> (Its also what you get when you use the plain Makefile)

Please add a comment in the .mk file about this, like:

# tcf-agent builds a shared library, but it really isn't useful for
# anything but tcf-agent itself, so don't bother building it as a
# shared library

> -   drop CMake....
> I am considering dropping the use of CMake (there s alot deduced
> automatically, by changing the name you got to redo all of that
> manually)
> and using the plain Makefile, this seems like less hassle in the end,
> but that means some additional stuff will be installed.

Hum, I don't think I suggested dropping CMake. I'm all in favor of
using standardized build systems such as CMake. What about just
patching the CMakeLists.txt to build a binary named tcf-agent instead
of agent? With a proper explanation, this should be acceptable by the
upstream tcf-agent developers.

> It there any common way to do this install step in an temp dir and
> then (re)moving files before copying this into the final target fs?

Nope, we don't have such a possibility currently I'm afraid.

Best regards,

Thomas
Norbert Lange Nov. 30, 2017, 1:53 p.m. | #4
2017-11-30 13:50 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello,
>
> It would be nice if you could avoid top-posting. Replying below each
> part of the e-mail you're answering is much more readable, and is the
> common practice on the mailing lists of open-source projects.

Ok noted.

>> -   drop CMake....
>> I am considering dropping the use of CMake (there s alot deduced
>> automatically, by changing the name you got to redo all of that
>> manually)
>> and using the plain Makefile, this seems like less hassle in the end,
>> but that means some additional stuff will be installed.
>
> Hum, I don't think I suggested dropping CMake. I'm all in favor of
> using standardized build systems such as CMake. What about just
> patching the CMakeLists.txt to build a binary named tcf-agent instead
> of agent? With a proper explanation, this should be acceptable by the
> upstream tcf-agent developers.

You did not suggest it, I am thinking about the best way to build it.
The package comes both with CMake and GNUMake files, both with their issues,
GNUMake is more complete as it installs headers and init script.

I cant easily rename the executable target, as there is already a target
named "tcf-agent" for the library. I dont want to work around that
limitation, as in my experience, as soon as you start circumventing
the CMake automechanics you never stop.
"tcf-agent" is the commonly referred executable name btw.

>
>> It there any common way to do this install step in an temp dir and
>> then (re)moving files before copying this into the final target fs?
>
> Nope, we don't have such a possibility currently I'm afraid.

Ok, gonna need some tinkering time, especially after looking more
closely into the (C)Make files. I found some goodies like

  if(NOT TCF_MACHINE)
    set(TCF_MACHINE "x86_64")
  endif()

which means currently other architectures will have some funny
disassembler and debugging-services...
Possible values are "a64  arm  i386  i686  powerpc  ppc64  x86_64", gonna
map this from the Buildroot variables somehow.

Also, if the package *optionally* links to eg. libssl if available,
then I would add this only in
TCFAGENT_DEPENDENCIES += openssl ?

Norbert
Yann E. MORIN Nov. 30, 2017, 9:24 p.m. | #5
Norbert, Thomas, All,

On 2017-11-30 13:50 +0100, Thomas Petazzoni spake thusly:
> On Thu, 30 Nov 2017 12:08:17 +0100, Norbert Lange wrote:
> > -   [ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
> > This scheme is used to be able to execute that script with 'sh -e'
> > [ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME would
> > fail if there is no such file (which is no error)
> > 
> > Please tell me if I should still use the later line.
> 
> Interesting reason. Please keep your original proposal then. The
> ejabberd package is already using this construct. However, all other
> packages are using the && construct.
> 
> Unless Yann has a better suggestion for this ?

I think we should write down those rules about startup scripts and their
configuration files.

But back on topic for Norbert's use-case. I usually do like he did, but
I find it hackish nonetheless. I really prefer positive logic anyway,
like so:

    if [ -r /etc/default/foo.config ]; then
        . /etc/default/foo.config
    fi

Yes, that's three lines instead of one, but who cares, really?

However, Norbert's excuse only stands for script that are 'set -e' (i.e.
they exit as soon as a command exits with a non-zero return code.

But this is not the case of the startup script. So if we would really
want to make it a single line, then positive logic would still to be
preferred:

    [ -r /etc/default/foo.config ] && . /etc/default/foo.config

Regards,
Yann E. MORIN.
Norbert Lange Dec. 1, 2017, 8:40 a.m. | #6
2017-11-30 22:24 GMT+01:00 Yann E. MORIN <yann.morin.1998@free.fr>:
> Norbert, Thomas, All,
>
> On 2017-11-30 13:50 +0100, Thomas Petazzoni spake thusly:
>> On Thu, 30 Nov 2017 12:08:17 +0100, Norbert Lange wrote:
>> > -   [ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
>> > This scheme is used to be able to execute that script with 'sh -e'
>> > [ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME would
>> > fail if there is no such file (which is no error)
>> >
>> > Please tell me if I should still use the later line.
>>
>> Interesting reason. Please keep your original proposal then. The
>> ejabberd package is already using this construct. However, all other
>> packages are using the && construct.
>>
>> Unless Yann has a better suggestion for this ?
>
> I think we should write down those rules about startup scripts and their
> configuration files.

Yes, maybe some helper scripts like https://packages.debian.org/sid/lsb-base
would be a good resource for templates and cut down most of the
repetition aswell.
eg.

#!/bin/sh

DAEMON_PATH=/usr/sbin/tcf-agent
DAEMON_NAME=tcf-agent
DAEMON_ARGS="-d -L- -l0"

. /lib/lsb/init-functions

# compute default name for pidfile
DAEMON_USE_PIDFILE=default

# mostly the same for many, if not copy then and adopt
. $DEFAULT_START_STOP

>
> But back on topic for Norbert's use-case. I usually do like he did, but
> I find it hackish nonetheless. I really prefer positive logic anyway,
> like so:
>
>     if [ -r /etc/default/foo.config ]; then
>         . /etc/default/foo.config
>     fi
>
> Yes, that's three lines instead of one, but who cares, really?
>
> However, Norbert's excuse only stands for script that are 'set -e' (i.e.
> they exit as soon as a command exits with a non-zero return code.

There is no excuse (I have no problem using your solution),
but old experiences, preferences and habits like running scripts
with set -e and the "shortcut logic" required - try until one of the
statements is true.
The if will disable exit-on-error, so that's different to both lines.

Btw. I was under the impression that buildroot tries to be as small as possible,
and therefore uses its own init scripts instead the ones from upstream packages.

>
> But this is not the case of the startup script. So if we would really
> want to make it a single line, then positive logic would still to be
> preferred:
>
>     [ -r /etc/default/foo.config ] && . /etc/default/foo.config

Ok.
Norbert
>
> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
Yann E. MORIN Dec. 1, 2017, 4:43 p.m. | #7
Norbet, All,

On 2017-12-01 09:40 +0100, Norbert Lange spake thusly:
> 2017-11-30 22:24 GMT+01:00 Yann E. MORIN <yann.morin.1998@free.fr>:
> > On 2017-11-30 13:50 +0100, Thomas Petazzoni spake thusly:
> >> On Thu, 30 Nov 2017 12:08:17 +0100, Norbert Lange wrote:
> >> > -   [ ! -r /etc/default/$DAEMON_NAME ] || . /etc/default/$DAEMON_NAME
> >> > This scheme is used to be able to execute that script with 'sh -e'
> >> > [ -r /etc/default/$DAEMON_NAME ] && . /etc/default/$DAEMON_NAME would
> >> > fail if there is no such file (which is no error)
> >> >
> >> > Please tell me if I should still use the later line.
> >>
> >> Interesting reason. Please keep your original proposal then. The
> >> ejabberd package is already using this construct. However, all other
> >> packages are using the && construct.
> >>
> >> Unless Yann has a better suggestion for this ?
> >
> > I think we should write down those rules about startup scripts and their
> > configuration files.
> 
> Yes, maybe some helper scripts like https://packages.debian.org/sid/lsb-base
> would be a good resource for templates and cut down most of the
> repetition aswell.

I think a simple template in the manual would be enough.

> > But back on topic for Norbert's use-case. I usually do like he did, but
> > I find it hackish nonetheless. I really prefer positive logic anyway,
> > like so:
> >
> >     if [ -r /etc/default/foo.config ]; then
> >         . /etc/default/foo.config
> >     fi
> >
> > Yes, that's three lines instead of one, but who cares, really?
> >
> > However, Norbert's excuse only stands for script that are 'set -e' (i.e.
> > they exit as soon as a command exits with a non-zero return code.
> 
> There is no excuse (I have no problem using your solution),

Sorry, I did not meant 'excuse' as something that was an bad reason, but
in a more neutral sense...

> but old experiences, preferences and habits like running scripts
> with set -e and the "shortcut logic" required - try until one of the
> statements is true.
> The if will disable exit-on-error, so that's different to both lines.
> 
> Btw. I was under the impression that buildroot tries to be as small as possible,
> and therefore uses its own init scripts instead the ones from upstream packages.

There is no causality; only correlation. We usually provide our own
startup scripts not because of size, but because the upstream ones make
strong assumptions about running on a desktop-like or server-like
system, and do not work with Busybox init for example.

And startup script are relatively small, when compared to the rest of
the system. Also, they comress very easily. So it is usually not worth
optimising them for size that much.

(and besides, using three lines instead of one is only in fact using
only just a few more bytes, which is what counts in the end).

Regards,
Yann E. MORIN.

> > But this is not the case of the startup script. So if we would really
> > want to make it a single line, then positive logic would still to be
> > preferred:
> >
> >     [ -r /etc/default/foo.config ] && . /etc/default/foo.config
> 
> Ok.
> Norbert
> >
> > Regards,
> > Yann E. MORIN.
> >
> > --
> > .-----------------.--------------------.------------------.--------------------.
> > |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> > | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> > | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> > '------------------------------^-------^------------------^--------------------'

Patch

diff --git a/package/Config.in b/package/Config.in
index fe5ccc434e..ab152677e5 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -124,6 +124,7 @@  menu "Debugging, profiling and benchmark"
 	source "package/stress-ng/Config.in"
 	source "package/sysdig/Config.in"
 	source "package/sysprof/Config.in"
+	source "package/tcfagent/Config.in"
 	source "package/tinymembench/Config.in"
 	source "package/trace-cmd/Config.in"
 	source "package/trinity/Config.in"
diff --git a/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
new file mode 100644
index 0000000000..bb9c334d91
--- /dev/null
+++ b/package/tcfagent/0001-add-CMake-install-target-for-agent-binary.patch
@@ -0,0 +1,35 @@ 
+From 9140c630085833acfe565f27195a876c6656f068 Mon Sep 17 00:00:00 2001
+From: Norbert Lange <norbert.lange@andritz.com>
+Date: Mon, 30 Oct 2017 16:22:40 +0100
+Subject: [PATCH] add CMake install target for agent binary
+
+allows use in automated buildsystems like Buildroot
+---
+ agent/CMakeLists.txt | 5 +++++
+ 1 file changed, 5 insertions(+)
+
+diff --git a/agent/CMakeLists.txt b/agent/CMakeLists.txt
+index aef15b96..7868987a 100644
+--- a/agent/CMakeLists.txt
++++ b/agent/CMakeLists.txt
+@@ -1,6 +1,7 @@
+ # -*- cmake -*-
+ 
+ cmake_minimum_required(VERSION 2.8)
++include(GNUInstallDirs)
+ 
+ set(CMAKE_COLOR_MAKEFILE OFF)
+ 
+@@ -43,3 +44,9 @@ message(STATUS "machine:" ${TCF_MACHINE})
+ 
+ add_executable(agent tcf/main/main.c)
+ target_link_libraries(agent ${TCF_LIB_NAME})
++
++install(TARGETS agent ${TCF_LIB_NAME}
++  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
++  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
++  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
++)
+-- 
+2.14.2
+
diff --git a/package/tcfagent/Config.in b/package/tcfagent/Config.in
new file mode 100644
index 0000000000..1374fdf88d
--- /dev/null
+++ b/package/tcfagent/Config.in
@@ -0,0 +1,16 @@ 
+config BR2_PACKAGE_TCFAGENT
+	bool "tcfagent"
+	depends on BR2_TOOLCHAIN_HAS_THREADS # snmp++
+	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.
+
+comment "tcfagent needs a toolchain w/ threads, C++, dynamic library"
+	depends on !BR2_TOOLCHAIN_HAS_THREADS
diff --git a/package/tcfagent/S55tcfagent b/package/tcfagent/S55tcfagent
new file mode 100755
index 0000000000..b022d5eb92
--- /dev/null
+++ b/package/tcfagent/S55tcfagent
@@ -0,0 +1,47 @@ 
+#!/bin/sh
+#
+# Start tcf-agent....
+#
+
+DAEMON_PATH=/usr/sbin/tcf-agent
+DAEMON_NAME=tcf-agent
+DAEMON_ARGS="-d -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 -q -o \
+      -x $DAEMON_PATH -- $DAEMON_ARGS &&
+        PPID=$(pidof $(basename $DAEMON_PATH)) &&
+        echo $PPID > $PIDFILE
+
+      [ $? = 0 ] && echo "OK" || echo "FAIL"
+}
+
+stop() {
+  printf "Stopping $DAEMON_NAME: "
+  start-stop-daemon -K -o -s HUP -q -p $PIDFILE \
+        -x $DAEMON_PATH &&
+        rm -f $PIDFILE
+  [ $? = 0 ] && echo "OK" || echo "FAIL"
+}
+
+case "$1" in
+    start)
+  start
+  ;;
+    stop)
+  stop
+  ;;
+    restart|reload)
+  stop
+  start
+  ;;
+  *)
+  echo "Usage: $0 {start|stop|restart}"
+  exit 1
+esac
+
+exit $?
diff --git a/package/tcfagent/tcfagent.hash b/package/tcfagent/tcfagent.hash
new file mode 100644
index 0000000000..ffed5dcb19
--- /dev/null
+++ b/package/tcfagent/tcfagent.hash
@@ -0,0 +1,4 @@ 
+# Locally computed:
+sha256  47d34c0778aa8b9e2c26132c9bb03d643bfb8e44d03ce862d06f2f93edcb63ae  org.eclipse.tcf.agent-1.3.0.tar.gz
+sha256  34188fd2daeadf6574071f5004f9a7a55b3ac73efe9db203a01559ee1013b2db  org.eclipse.tcf.agent-1.4_neon.tar.gz
+sha256  4b6c757e2bed92a0a791d0687425d462c974abe4c79f80e27e362fdaa59107f5  org.eclipse.tcf.agent-1.5_oxygen.tar.gz
diff --git a/package/tcfagent/tcfagent.mk b/package/tcfagent/tcfagent.mk
new file mode 100644
index 0000000000..30bd366137
--- /dev/null
+++ b/package/tcfagent/tcfagent.mk
@@ -0,0 +1,38 @@ 
+################################################################################
+#
+# TCFAGENT
+#
+################################################################################
+
+TCFAGENT_VERSION = 1.5_oxygen
+# the tar.xz link was broken the time this file got authored
+TCFAGENT_SOURCE = org.eclipse.tcf.agent-$(TCFAGENT_VERSION).tar.gz
+TCFAGENT_SITE = http://git.eclipse.org/c/tcf/org.eclipse.tcf.agent.git/snapshot
+# see https://wiki.spdx.org/view/Legal_Team/License_List/Licenses_Under_Consideration
+TCFAGENT_LICENSE = BSD-3-Clause
+TCFAGENT_LICENSE_FILES = agent/edl-v10.html
+
+TCFAGENT_DEPENDENCIES = util-linux
+TCFAGENT_SUBDIR = agent
+TCFAGENT_CONF_OPTS = -DBUILD_SHARED_LIBS=Off
+
+define TCFAGENT_RENAME_AGENT
+	mv $(TARGET_DIR)/usr/bin/agent $(TARGET_DIR)/usr/sbin/tcf-agent
+endef
+
+TCFAGENT_POST_INSTALL_TARGET_HOOKS += TCFAGENT_RENAME_AGENT
+
+define TCFAGENT_INSTALL_INIT_SYSTEMD
+	$(INSTALL) -D -m 644 package/tcfagent/tcfagent.service \
+		$(TARGET_DIR)/usr/lib/systemd/system/tcfagent.service
+	mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants
+	ln -fs ../../../../usr/lib/systemd/system/tcfagent.service \
+		$(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/tcfagent.service
+endef
+
+define TCFAGENT_INSTALL_INIT_SYSV
+	$(INSTALL) -D -m 755 package/tcfagent/S55tcfagent \
+		$(TARGET_DIR)/etc/init.d/S55tcfagent
+endef
+
+$(eval $(cmake-package))
diff --git a/package/tcfagent/tcfagent.service b/package/tcfagent/tcfagent.service
new file mode 100644
index 0000000000..2f57fca3c8
--- /dev/null
+++ b/package/tcfagent/tcfagent.service
@@ -0,0 +1,10 @@ 
+[Unit]
+Description=Target Communication Framework Agent
+After=network.target
+
+[Service]
+Type=forking
+ExecStart=@SBINDIR@/tcf-agent -L- -l0
+
+[Install]
+WantedBy=multi-user.target