[Buildroot] [PATCH v3] tcfagent: new package
Norbert Lange
nolange79 at gmail.com
Tue Jan 2 16:34:19 UTC 2018
Hello,
before I do the bureaucratic work and more testing,
there is some mixed information from you and the docs.
To quote the manual:
> Target architecture
>
> Dependency symbol: BR2_powerpc, BR2_mips, … (see arch/Config.in)
> Comment string: no comment to be added
I am now unsure about the comment sections, and whether there
should be one or two of them.
Full Config.in is following.
Norbert
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
2017-12-15 14:11 GMT+01:00 Norbert Lange <nolange79 at gmail.com>:
> 2017-12-13 14:49 GMT+01:00 Thomas Petazzoni
> <thomas.petazzoni at free-electrons.com>:
>> Hello,
>>
>> On Mon, 4 Dec 2017 13:09:12 +0100, Norbert Lange wrote:
>>> From: Norbert Lange <norbert.lange at andritz.com>
>>>
>>> Add tcfpackage which contains a service "tcf-agent"
>>>
>>> Signed-off-by: Norbert Lange <nolange79 at gmail.com>
>>
>> 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.
>>
>>> +N: Norbert Lange <nolange79 at gmail.com>
>>> +F: package/tcfagent
>>
>> Add a final / here, to be consistent with all other entries in that
>> file.
>>
>>> diff --git a/package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch b/package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch
>>> new file mode 100644
>>> index 0000000000..954c0d5fe3
>>> --- /dev/null
>>> +++ b/package/tcfagent/0001-agent-add-install-target-to-the-CMakeLists.patch
>>> @@ -0,0 +1,48 @@
>>> +From 4c5fb079bc06ef04d09955eebe0d93d5ebfd4cdf Mon Sep 17 00:00:00 2001
>>> +From: Norbert Lange <nolange79 at gmail.com>
>>> +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'
>>> +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 <nolange79 at gmail.com>
>>
>> Did you submit this patch upstream ?
>
> Only a failed attempt so far, gonna try again
>
>
>>> diff --git a/package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch b/package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch
>>> new file mode 100644
>>> index 0000000000..16b748926f
>>> --- /dev/null
>>> +++ b/package/tcfagent/0002-linux-remove-explicit-uses-of-__ptrace_request.patch
>>> @@ -0,0 +1,66 @@
>>> +From 6407e3961dba749c8e3a0ea4ae8991154520364f Mon Sep 17 00:00:00 2001
>>> +From: Norbert Lange <nolange79 at gmail.com>
>>> +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 :)
>>
>>
>>
>>> +
>>> +This type is not to be used directly, and with musl it wont build
>>> +
>>> +Signed-off-by: Norbert Lange <nolange79 at gmail.com>
>>
>> Did you submit this upstream?
>>
>>
>>> diff --git a/package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch b/package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch
>>> new file mode 100644
>>> index 0000000000..86dc2f37a1
>>> --- /dev/null
>>> +++ b/package/tcfagent/0003-linux-provide-canonicalize_file_name-for-all-c-libs-.patch
>>> @@ -0,0 +1,46 @@
>>> +From 58cc4092495acdc0b4be8e23c3964d47989a59dd Mon Sep 17 00:00:00 2001
>>> +From: Norbert Lange <nolange79 at gmail.com>
>>> +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.
>>
>>> +
>>> +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 <nolange79 at gmail.com>
>>> +---
>>> + 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);
>>> + }
>>> +
>>> +-#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.
>
>>
>> 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.
>
>>
>>
>>> diff --git a/package/tcfagent/Config.in b/package/tcfagent/Config.in
>>> new file mode 100644
>>> index 0000000000..81affce6c1
>>> --- /dev/null
>>> +++ b/package/tcfagent/Config.in
>>> @@ -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
>>> + 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
>
> Ok, Config is not something I am deeply familiar with.
>
>>
>>
>>
>>> diff --git a/package/tcfagent/S55tcfagent b/package/tcfagent/S55tcfagent
>>> new file mode 100755
>>> index 0000000000..2243b5a4c7
>>> --- /dev/null
>>> +++ b/package/tcfagent/S55tcfagent
>>> @@ -0,0 +1,40 @@
>>> +#!/bin/sh
>>> +
>>> +DAEMON_PATH=/usr/sbin/tcf-agent
>>> +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.
>>
>>> +
>>> + [ $? = 0 ] && echo "OK" || echo "FAIL"
>>
>> You indent with 8 spaces here, it should be one tab.
>>
>>> +}
>>> +
>>> +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.
>>
>>> +}
>>> +
>>> +case "$1" in
>>> + start)
>>> + start
>>> + ;;
>>> + stop)
>>> + stop
>>> + ;;
>>> + restart|reload)
>>> + stop
>>> + start
>>
>> Indentation is not good here.
>>
>>> +# 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
>>> +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))
>>
>>> +
>>> +_TCFAGENT_BRARCH =
>>
>> Unneeded variable.
>>
>> Could you rework your submission according to this review, and submit
>> an updated version ?
>
> Sure, might take a while.
>
>>
>> Thanks a lot!
>>
>> Thomas
>> --
>> Thomas Petazzoni, CTO, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Norbert LAnge
More information about the buildroot
mailing list