[1/1] package/uftrace: new package

From 57e136108b4ff900f3b7972f1f1c19a59ef9cf12 Mon Sep 17 00:00:00 2001
From: Giacomo Longo <gabibbo97@gmail.com>
Date: Mon, 26 Oct 2020 23:11:41 +0100
Subject: [PATCH 1/1] package/uftrace: new package

Hello, first time trying to package something for BuildRoot and
sending a patch via email.

The package is a tracing utility similar to ltrace and strace that
allows profiling of programs and their library calls.

I have performed a run of ./utils/check-package package/uftrace/* with
the following result:

> 78 lines processed
> 0 warnings generated

I have performed a run of ./utils/test-pkg -d ../test-pkg -c
../uftrace.config -p uftrace -a

> 45 builds, 36 skipped, 0 build failed, 0 legal-info failed

The only architectures supported upstream are aarch64, arm and x86_64/i386.
I could not build the program without glibc so it's marked as
requiring a glibc toolchain.

I am looking for some pointers on how to upstream my modification and
feedback on the patch in its current state.

Signed-off-by: Giacomo Longo <gabibbo97@gmail.com>
---
package/Config.in | 1 +
package/uftrace/Config.in | 41 ++++++++++++++++++++++++++++++++++++
package/uftrace/uftrace.hash | 3 +++
package/uftrace/uftrace.mk | 34 ++++++++++++++++++++++++++++++
4 files changed, 79 insertions(+)
create mode 100644 package/uftrace/Config.in
create mode 100644 package/uftrace/uftrace.hash
create mode 100644 package/uftrace/uftrace.mk

--
2.28.0

Hello Giacomo,

Thanks for your contribution! See below for a number of comments and
suggestions.

On Tue, 27 Oct 2020 00:14:14 +0100
Giacomo Longo <gabibbo97@gmail.com> wrote:

> From 57e136108b4ff900f3b7972f1f1c19a59ef9cf12 Mon Sep 17 00:00:00 2001
> From: Giacomo Longo <gabibbo97@gmail.com>
> Date: Mon, 26 Oct 2020 23:11:41 +0100
> Subject: [PATCH 1/1] package/uftrace: new package
> 
> Hello, first time trying to package something for BuildRoot and
> sending a patch via email.

Your patch should be sent using "git send-email". You can find at
https://git-send-email.io/ instructions on how to set up git send-email.

> The package is a tracing utility similar to ltrace and strace that
> allows profiling of programs and their library calls.
> 
> I have performed a run of ./utils/check-package package/uftrace/* with
> the following result:
> 
> > 78 lines processed
> > 0 warnings generated  
> 
> I have performed a run of ./utils/test-pkg -d ../test-pkg -c
> ../uftrace.config -p uftrace -a
> 
> > 45 builds, 36 skipped, 0 build failed, 0 legal-info failed  

Very good work!

> The only architectures supported upstream are aarch64, arm and x86_64/i386.
> I could not build the program without glibc so it's marked as
> requiring a glibc toolchain.

What specific issues have you encountered with other C libraries ?

> I am looking for some pointers on how to upstream my modification and
> feedback on the patch in its current state.

Read on for more comments :)

> Signed-off-by: Giacomo Longo <gabibbo97@gmail.com>
> ---
> package/Config.in | 1 +
> package/uftrace/Config.in | 41 ++++++++++++++++++++++++++++++++++++
> package/uftrace/uftrace.hash | 3 +++
> package/uftrace/uftrace.mk | 34 ++++++++++++++++++++++++++++++
> 4 files changed, 79 insertions(+)

Could you add an entry to the DEVELOPERS file for this package ?


> diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
> new file mode 100644
> index 0000000000..4912d1d8cc
> --- /dev/null
> +++ b/package/uftrace/Config.in
> @@ -0,0 +1,41 @@
> +config BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
> + bool
> + default y if BR2_aarch64
> + default y if BR2_arm
> + default y if BR2_i386
> + default y if BR2_x86_64
> +
> +config BR2_PACKAGE_UFTRACE
> + bool "uftrace"
> + depends on BR2_TOOLCHAIN_USES_GLIBC
> + depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
> + depends on !BR2_STATIC_LIBS
> + select BR2_PACKAGE_ELFUTILS

When you "select" something, you must replicate its depends on. So here
you must replicate:

        depends on BR2_USE_WCHAR
        depends on BR2_TOOLCHAIN_HAS_THREADS

> + select BR2_PACKAGE_UTIL_LINUX

You just need util-linux, and none of its sub-options ? This is quite
odd, very often packages need libuuid, or libmount, etc.

> + select BR2_INSTALL_LIBSTDCPP

You cannot select this: it must be a "depends on".

> + help
> + Tool to trace and analyze execution of a program.
> +
> + https://uftrace.github.io/slide
> +
> +comment "uftrace needs a glibc toolchain w/ C++, dynamic library"
> + depends on !BR2_TOOLCHAIN_USES_GLIBC || BR2_STATIC_LIBS ||
> !BR2_INSTALL_LIBSTDCPP

You need to move this comment either before the BR2_PACKAGE_UFTRACE
option, or at the end of file. Due to how kconfig works, if you have
this comment between the BR2_PACKAGE_UFTRACE option and its
sub-options, the sub-options will not properly be "indented" in
menuconfig.

Also, you need to add a:

	depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS

so that the comment doesn't appear on architectures where uftrace is
anyway not available.

> +if BR2_PACKAGE_UFTRACE
> +
> +config BR2_PACKAGE_UFTRACE_LUAJIT
> + bool "luajit scripting support"
> + default n

default n is the default, so it's not needed.

> + select BR2_PACKAGE_LUAJIT

When you select, you need to replicate the "depends on" of the options
you're selecting.

> + help
> + Enable luajit scripting support
> +
> +config BR2_PACKAGE_UFTRACE_TUI
> + bool "TUI support"
> + default n

Not needed.

> + depends on BR2_USE_WCHAR

ncurses doesn't depend on wchar, unless you need
BR2_PACKAGE_NCURSES_WCHAR of course.

> + select BR2_PACKAGE_NCURSES
> + help
> + Enable TUI support

Perhaps those two sub-options are not really needed, and you could
simply enable luajit and ncurses support in the .mk file if the
appropriate packages are enabled, like this:

ifeq ($(BR2_PACKAGE_LUAJIT),y)
... enable luajit support ...
else
... disable luajit support ...
endif

> diff --git a/package/uftrace/uftrace.mk b/package/uftrace/uftrace.mk
> new file mode 100644
> index 0000000000..4c60962151
> --- /dev/null
> +++ b/package/uftrace/uftrace.mk
> @@ -0,0 +1,34 @@
> +################################################################################
> +#
> +# uftrace
> +#
> +################################################################################
> +
> +UFTRACE_VERSION = v0.9.4
> +UFTRACE_SITE = $(call github,namhyung,uftrace,$(UFTRACE_VERSION))
> +UFTRACE_LICENSE = GPL-2.0+
> +UFTRACE_LICENSE_FILES = COPYING
> +UFTRACE_DEPENDENCIES = elfutils

You are selecting util-linux in the Config.in file, but you don't have
any build dependency on it. Since you have tested with test-pkg, it
seems like indeed util-linux is not a build dependency. Is it a runtime
dependency ? If so, for what tool ?

> +
> +ifeq ($(BR2_PACKAGE_UFTRACE_LUAJIT),y)
> +UFTRACE_DEPENDENCIES += luajit

There is no explicit ./configure option to enable/disable luajit
support ?

> +endif
> +
> +ifeq ($(BR2_PACKAGE_UFTRACE_TUI),y)
> +UFTRACE_DEPENDENCIES += ncurses

Same question here.

> +endif
> +
> +ifeq ($(BR2_aarch64),y)
> +UFTRACE_CONF_OPTS = --arch=aarch64
> +else ifeq ($(BR2_arm),y)
> +UFTRACE_CONF_OPTS = --arch=arm
> +else ifeq ($(BR2_i386),y)
> +UFTRACE_CONF_OPTS = --arch=x86
> +else ifeq ($(BR2_x86_64),y)
> +UFTRACE_CONF_OPTS = --arch=x86_64
> +endif
> +
> +UFTRACE_CONF_OPTS += --without-libpython
> +UFTRACE_CONF_OPTS += --without-capstone

We generally prefer to have the unconditional options like this before
the conditional ones, and written this way:

UFTRACE_CONF_OPTS = \
	--without-libpython \
	--without-capstone

Could you rework your patch to address those comments, and send a new
iteration, preferably with git send-email ?

Thanks!

Thomas

First of all, thanks Thomas for your kind words and helping me finding
my way around (this mail is my first git-send-patch).

I have tried to incorporate the feedback inside this second version of
the patch.

Here's a list of the changes I've made:

I added myself to the DEVELOPERS file.

I decided to remove the configuration options concerning TUI and
luajit, replacing them with an if inside the makefile. These are not
core functionalities and probably are not worth of an extra
configuration flag.

I removed the dependency on UTIL_LINUX that was not needed.

I have converted BR2_INSTALL_LIBSTDCPP from "selects" to "depends on".

The flags BR2_USE_WCHAR, BR2_TOOLCHAIN_HAS_THREADS coming from the
selection of BR2_PACKAGE_ELFUTILS have been included as
depends on.

Comment has been moved to the end of the file, and a dependency on the
correct architecture has been added.

The makefile has been shuffled around:

- Standard configuration options are now at the top
- Architecture configuration follows
- Optional feature flags are at the bottom

Concerning the uClibc and musl incompatibility, build fails on non-glibc with this error:

uftrace-v0.9.4/cmds/record.c:2078:19: error: ‘ADDR_NO_RANDOMIZE’ undeclared (first use in this function)
   if (personality(ADDR_NO_RANDOMIZE) < 0)

I have been able to find the symbol in musl at
https://git.musl-libc.org/cgit/musl/tree/include/sys/personality.h but
apparently it does not work.

The patch as it is passes ./utils/test-pkg with 45 builds, 36 skipped, 0
build failed, 0 legal-info failed and ./utils/check-package
package/uftrace/* with 62 lines processed and 0 warnings generated.

I have been able to test the package by assembling and running buildroot
on QEMU system arm emulation (target arch Cortex A7).

Thanks again for your understanding, have a nice day.

Hello Giaocomo,

On Tue, 27 Oct 2020 16:14:07 +0100
Giacomo Longo <gabibbo97@gmail.com> wrote:

> First of all, thanks Thomas for your kind words and helping me finding
> my way around (this mail is my first git-send-patch).
> 
> I have tried to incorporate the feedback inside this second version of
> the patch.

Thanks for your new iteration. I will reply directly on the patch
itself for additional comments.

Best regards,

Thomas