| Message ID | 20171114145442.16734-1-norbert.lange@andritz.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] Add TCF Agent package | expand |
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
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
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
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
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.
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. | > '------------------------------^-------^------------------^--------------------'
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. | > > '------------------------------^-------^------------------^--------------------'
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
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