* [Buildroot] [PATCH 1/1] package/uftrace: new package
@ 2020-10-26 23:14 Giacomo Longo
2020-10-27 9:36 ` Thomas Petazzoni
0 siblings, 1 reply; 9+ messages in thread
From: Giacomo Longo @ 2020-10-26 23:14 UTC (permalink / raw)
To: buildroot
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2020-10-26 23:14 Giacomo Longo
@ 2020-10-27 9:36 ` Thomas Petazzoni
2020-10-27 15:14 ` Giacomo Longo
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Petazzoni @ 2020-10-27 9:36 UTC (permalink / raw)
To: buildroot
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
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2020-10-27 9:36 ` Thomas Petazzoni
@ 2020-10-27 15:14 ` Giacomo Longo
2020-11-03 20:39 ` Thomas Petazzoni
0 siblings, 1 reply; 9+ messages in thread
From: Giacomo Longo @ 2020-10-27 15:14 UTC (permalink / raw)
To: buildroot
Signed-off-by: Giacomo Longo <gabibbo97@gmail.com>
---
DEVELOPERS | 3 +++
package/Config.in | 1 +
package/uftrace/Config.in | 24 ++++++++++++++++++++++++
package/uftrace/uftrace.hash | 3 +++
package/uftrace/uftrace.mk | 35 +++++++++++++++++++++++++++++++++++
5 files changed, 66 insertions(+)
create mode 100644 package/uftrace/Config.in
create mode 100644 package/uftrace/uftrace.hash
create mode 100644 package/uftrace/uftrace.mk
diff --git a/DEVELOPERS b/DEVELOPERS
index ceb9cc9160..1c63e693b3 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -980,6 +980,9 @@ N: Gerome Burlats <gerome.burlats@smile.fr>
F: board/qemu/
F: configs/qemu_*
+N: Giacomo Longo <gabibbo97@gmail.com>
+F: package/uftrace/
+
N: Gilles Talis <gilles.talis@gmail.com>
F: board/freescale/imx8mmevk/
F: configs/freescale_imx8mmevk_defconfig
diff --git a/package/Config.in b/package/Config.in
index ee05467479..b9c51dc3f1 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -140,6 +140,7 @@ menu "Debugging, profiling and benchmark"
source "package/trace-cmd/Config.in"
source "package/trinity/Config.in"
source "package/uclibc-ng-test/Config.in"
+ source "package/uftrace/Config.in"
source "package/valgrind/Config.in"
source "package/vmtouch/Config.in"
source "package/whetstone/Config.in"
diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
new file mode 100644
index 0000000000..4349a5778e
--- /dev/null
+++ b/package/uftrace/Config.in
@@ -0,0 +1,24 @@
+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_INSTALL_LIBSTDCPP
+ depends on !BR2_STATIC_LIBS
+ depends on BR2_USE_WCHAR # elfutils
+ depends on BR2_TOOLCHAIN_HAS_THREADS # elfutils
+ select BR2_PACKAGE_ELFUTILS
+ 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, threads, wchar"
+ depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
+ depends on !BR2_TOOLCHAIN_USES_GLIBC || !BR2_INSTALL_LIBSTDCPP || BR2_STATIC_LIBS || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR
diff --git a/package/uftrace/uftrace.hash b/package/uftrace/uftrace.hash
new file mode 100644
index 0000000000..8f38e7431d
--- /dev/null
+++ b/package/uftrace/uftrace.hash
@@ -0,0 +1,3 @@
+# Locally computed:
+sha512 f73ad4461051b9c61668161e077897d118ac556d234ff204e32bf14ecdc2c0df148da30ea5d5054641e79ea20b29261d6f637908f5047f5669207ef244865358 uftrace-v0.9.4.tar.gz
+sha512 aee80b1f9f7f4a8a00dcf6e6ce6c41988dcaedc4de19d9d04460cbfb05d99829ffe8f9d038468eabbfba4d65b38e8dbef5ecf5eb8a1b891d9839cda6c48ee957 COPYING
diff --git a/package/uftrace/uftrace.mk b/package/uftrace/uftrace.mk
new file mode 100644
index 0000000000..bce98888de
--- /dev/null
+++ b/package/uftrace/uftrace.mk
@@ -0,0 +1,35 @@
+################################################################################
+#
+# 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
+
+UFTRACE_CONF_OPTS = \
+ --without-capstone \
+ --without-libpython
+
+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=i386
+else ifeq ($(BR2_x86_64),y)
+UFTRACE_CONF_OPTS += --arch=x86_64
+endif
+
+ifeq ($(BR2_PACKAGE_LUAJIT),n)
+UFTRACE_CONF_OPTS += --without-libluajit
+endif
+
+ifeq ($(BR2_PACKAGE_NCURSES_WCHAR),n)
+UFTRACE_CONF_OPTS += --without-libncurses
+endif
+
+$(eval $(autotools-package))
--
2.28.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2020-10-27 15:14 ` Giacomo Longo
@ 2020-11-03 20:39 ` Thomas Petazzoni
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Petazzoni @ 2020-11-03 20:39 UTC (permalink / raw)
To: buildroot
Hello,
On Tue, 27 Oct 2020 16:14:08 +0100
Giacomo Longo <gabibbo97@gmail.com> wrote:
> diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
> new file mode 100644
> index 0000000000..4349a5778e
> --- /dev/null
> +++ b/package/uftrace/Config.in
> @@ -0,0 +1,24 @@
> +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
In fact the reason why you can only use glibc is two-fold:
* You select elfutils, and elfutils does not build on musl.
* uftrace uses ADDR_NO_RANDOMIZE, which isn't supported in uClibc
So it would be good to write something like this:
# elfutils not available for musl, uClibc-ng does not
# provide ADDR_NO_RANDOMIZE, so only glibc is supported
depends on BR2_TOOLCHAIN_USES_GLIBC
> +comment "uftrace needs a glibc toolchain w/ C++, dynamic library, threads, wchar"
> + depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
> + depends on !BR2_TOOLCHAIN_USES_GLIBC || !BR2_INSTALL_LIBSTDCPP || BR2_STATIC_LIBS || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR
Could you cut this line at ~80 characters, using a backslash ? See
other packages.
> diff --git a/package/uftrace/uftrace.hash b/package/uftrace/uftrace.hash
> new file mode 100644
> index 0000000000..8f38e7431d
> --- /dev/null
> +++ b/package/uftrace/uftrace.hash
> @@ -0,0 +1,3 @@
> +# Locally computed:
> +sha512 f73ad4461051b9c61668161e077897d118ac556d234ff204e32bf14ecdc2c0df148da30ea5d5054641e79ea20b29261d6f637908f5047f5669207ef244865358 uftrace-v0.9.4.tar.gz
> +sha512 aee80b1f9f7f4a8a00dcf6e6ce6c41988dcaedc4de19d9d04460cbfb05d99829ffe8f9d038468eabbfba4d65b38e8dbef5ecf5eb8a1b891d9839cda6c48ee957 COPYING
Could you use sha256 hashes instead ?
> +UFTRACE_VERSION = v0.9.4
> +UFTRACE_SITE = $(call github,namhyung,uftrace,$(UFTRACE_VERSION))
Please don't encode the "v" in the version field. So please do:
UFTRACE_VERSION = 0.9.4
UFTRACE_SITE = $(call github,namhyung,uftrace,v$(UFTRACE_VERSION))
Indeed, this will allow UFTRACE_VERSION to be used to check if new
uftrace releases are available, using release-monitoring.org.
> +UFTRACE_LICENSE = GPL-2.0+
> +UFTRACE_LICENSE_FILES = COPYING
> +UFTRACE_DEPENDENCIES = elfutils
> +
> +UFTRACE_CONF_OPTS = \
> + --without-capstone \
> + --without-libpython
> +
> +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=i386
> +else ifeq ($(BR2_x86_64),y)
> +UFTRACE_CONF_OPTS += --arch=x86_64
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LUAJIT),n)
BR2_PACKAGE_LUAJIT is never going to be "n", so this condition will
never be true. Also, if luajit *is* there, you want luajit to be
compiled before uftrace. So you want something like this:
# No --with-<option> available
ifeq ($(BR2_PACKAGE_LUAJIT),y)
UFTRACE_DEPENDENCIES += luajit
else
UFTRACE_CONF_OPTS += --without-libluajit
endif
> +ifeq ($(BR2_PACKAGE_NCURSES_WCHAR),n)
> +UFTRACE_CONF_OPTS += --without-libncurses
> +endif
Same comment as above.
> +$(eval $(autotools-package))
This is not an autotools-based package: it uses a hand-written
configure script, not one that is generated by autoconf. For example,
this explains why it doesn't support --with-<foo>, but only
--without-<foo>.
So, please use the generic-package infrastructure in this sort of
cases. Using autotools-package should really only be done for packages
that do use autoconf.
Could you adjust your package according to this ?
Thanks a lot!
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
@ 2021-04-03 19:16 Asaf Kahlon
2021-04-04 9:03 ` Yann E. MORIN
0 siblings, 1 reply; 9+ messages in thread
From: Asaf Kahlon @ 2021-04-03 19:16 UTC (permalink / raw)
To: buildroot
The uftrace tool is to trace and analyze execution of a
program written in C/C++. It was heavily inspired by the
ftrace framework of the Linux kernel (especially function
graph tracer) and supports userspace programs.
It supports various kind of commands and filters to help
analysis of the program execution and performance.
Signed-off-by: Asaf Kahlon <asafka7@gmail.com>
---
DEVELOPERS | 1 +
package/Config.in | 1 +
package/busybox/busybox. | 0
package/uftrace/Config.in | 12 ++++++++++++
package/uftrace/uftrace.hash | 3 +++
package/uftrace/uftrace.mk | 26 ++++++++++++++++++++++++++
6 files changed, 43 insertions(+)
create mode 100644 package/busybox/busybox.
create mode 100644 package/uftrace/Config.in
create mode 100644 package/uftrace/uftrace.hash
create mode 100644 package/uftrace/uftrace.mk
diff --git a/DEVELOPERS b/DEVELOPERS
index c6d4f1919f..33607f8b30 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -240,6 +240,7 @@ F: package/python*
F: package/snmpclitools/
F: package/spdlog/
F: package/uftp/
+F: package/uftrace/
F: package/uvw/
F: package/zeromq/
diff --git a/package/Config.in b/package/Config.in
index 1269bc7b51..cd5cd17576 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -144,6 +144,7 @@ menu "Debugging, profiling and benchmark"
source "package/trace-cmd/Config.in"
source "package/trinity/Config.in"
source "package/uclibc-ng-test/Config.in"
+ source "package/uftrace/Config.in"
source "package/valgrind/Config.in"
source "package/vmtouch/Config.in"
source "package/whetstone/Config.in"
diff --git a/package/busybox/busybox. b/package/busybox/busybox.
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
new file mode 100644
index 0000000000..ca3041195e
--- /dev/null
+++ b/package/uftrace/Config.in
@@ -0,0 +1,12 @@
+config BR2_PACKAGE_UFTRACE
+ bool "uftrace"
+ depends on BR2_arm || BR2_aarch64 || BR2_x86_64
+ help
+ The uftrace tool is to trace and analyze execution of a
+ program written in C/C++. It was heavily inspired by the
+ ftrace framework of the Linux kernel (especially function
+ graph tracer) and supports userspace programs.
+ It supports various kind of commands and filters to help
+ analysis of the program execution and performance.
+
+ https://github.com/namhyung/uftrace
diff --git a/package/uftrace/uftrace.hash b/package/uftrace/uftrace.hash
new file mode 100644
index 0000000000..be0464d8e9
--- /dev/null
+++ b/package/uftrace/uftrace.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256 418d30c959d3b6d0dcafd55e588a5d414a9984b30f2522a5af004a268824a5a2 v0.9.4.tar.gz
+sha256 8177f97513213526df2cf6184d8ff986c675afb514d4e68a404010521b880643 COPYING
diff --git a/package/uftrace/uftrace.mk b/package/uftrace/uftrace.mk
new file mode 100644
index 0000000000..b696ac4c6e
--- /dev/null
+++ b/package/uftrace/uftrace.mk
@@ -0,0 +1,26 @@
+################################################################################
+#
+# uftrace
+#
+################################################################################
+
+UFTRACE_VERSION = 0.9.4
+UFTRACE_SITE = https://github.com/namhyung/uftrace/archive/refs/tags
+UFTRACE_SOURCE = v0.9.4.tar.gz
+UFTRACE_LICENSE = GPL-2.0
+UFTRACE_LICENSE_FILES = COPYING
+
+ifeq ($(BR2_PACKAGE_ELFUTILS),y)
+UFTRACE_DEPENDENCIES += elfutils
+endif
+
+define UFTRACE_BUILD_CMDS
+ $(TARGET_CONFIGURE_OPTS) ARCH=$(BR2_ARCH) $(MAKE) -C $(@D) $(UFTRACE_MAKE_OPTS)
+endef
+
+define UFTRACE_INSTALL_TARGET_CMDS
+ $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) \
+ $(UFTRACE_MAKE_OPTS) install
+endef
+
+$(eval $(generic-package))
--
2.27.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2021-04-03 19:16 [Buildroot] [PATCH 1/1] package/uftrace: new package Asaf Kahlon
@ 2021-04-04 9:03 ` Yann E. MORIN
2021-04-04 10:07 ` Thomas Petazzoni
2021-04-08 18:52 ` Asaf Kahlon
0 siblings, 2 replies; 9+ messages in thread
From: Yann E. MORIN @ 2021-04-04 9:03 UTC (permalink / raw)
To: buildroot
Asaf, All,
On 2021-04-03 22:16 +0300, Asaf Kahlon spake thusly:
> The uftrace tool is to trace and analyze execution of a
> program written in C/C++. It was heavily inspired by the
> ftrace framework of the Linux kernel (especially function
> graph tracer) and supports userspace programs.
> It supports various kind of commands and filters to help
> analysis of the program execution and performance.
Rather than duplicate the package description in the commit log, I
think it makes more sense that it contains explanations about the
packaging in Buildroot. See below.
> Signed-off-by: Asaf Kahlon <asafka7@gmail.com>
> ---
> DEVELOPERS | 1 +
> package/Config.in | 1 +
> package/busybox/busybox. | 0
Uh? ;-)
[--SNIP--]
> diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
> new file mode 100644
> index 0000000000..ca3041195e
> --- /dev/null
> +++ b/package/uftrace/Config.in
> @@ -0,0 +1,12 @@
> +config BR2_PACKAGE_UFTRACE
> + bool "uftrace"
> + depends on BR2_arm || BR2_aarch64 || BR2_x86_64
AFAICS, it also support i386...
Even though the architecture dependencies are pretty trivial today, I
think it would be good to introduce a _ARCH_SUPPORTS symbol:
config BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
bool
default y if BR2_arm
default y if BR2_aarch64
default y if BR2_i386
default y if BR2_x86_64
config BR2_PACKAGE_UFTRACE
bool "uftrace"
depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
[--SNIP--]
> diff --git a/package/uftrace/uftrace.mk b/package/uftrace/uftrace.mk
> new file mode 100644
> index 0000000000..b696ac4c6e
> --- /dev/null
> +++ b/package/uftrace/uftrace.mk
> @@ -0,0 +1,26 @@
> +################################################################################
> +#
> +# uftrace
> +#
> +################################################################################
> +
> +UFTRACE_VERSION = 0.9.4
> +UFTRACE_SITE = https://github.com/namhyung/uftrace/archive/refs/tags
Please use the github macro; see e.g. boot/shim/shim.mk and the manual:
https://buildroot.org/downloads/manual/manual.html#github-download-url
> +UFTRACE_SOURCE = v0.9.4.tar.gz
No need to specify _SOURCE when using the github macro.
> +UFTRACE_LICENSE = GPL-2.0
> +UFTRACE_LICENSE_FILES = COPYING
> +
> +ifeq ($(BR2_PACKAGE_ELFUTILS),y)
> +UFTRACE_DEPENDENCIES += elfutils
> +endif
> +
> +define UFTRACE_BUILD_CMDS
> + $(TARGET_CONFIGURE_OPTS) ARCH=$(BR2_ARCH) $(MAKE) -C $(@D) $(UFTRACE_MAKE_OPTS)
You are not setting UFTRACE_MAKE_OPTS anywhere that I can see...
> +endef
>
> +define UFTRACE_INSTALL_TARGET_CMDS
> + $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) \
> + $(UFTRACE_MAKE_OPTS) install
> +endef
> +
> +$(eval $(generic-package))
There is a ./configure script (but it is not autotools, so this truly is
a generic-package indeed): any reason why you are not using it? A little
note about it in the commit log would be nice.
I was wondering if having a host variant would not be interesting too.
Indeed, uftrace has a record mode, where it has a 'record' mode where it
stores all the traces into a file, and replay/report/graph modes where
it reads from that file to do off-sire analysis. So one might be
interested in running in record mode in the target, and doing the
analysis on the development machine...
Thoughts?
Regards,
Yann E. MORIN.
> --
> 2.27.0
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2021-04-04 9:03 ` Yann E. MORIN
@ 2021-04-04 10:07 ` Thomas Petazzoni
2021-04-08 18:45 ` Asaf Kahlon
2021-04-08 18:52 ` Asaf Kahlon
1 sibling, 1 reply; 9+ messages in thread
From: Thomas Petazzoni @ 2021-04-04 10:07 UTC (permalink / raw)
To: buildroot
Hello,
(In addition to Yann comments)
On Sun, 4 Apr 2021 11:03:38 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > +UFTRACE_LICENSE = GPL-2.0
> > +UFTRACE_LICENSE_FILES = COPYING
> > +
> > +ifeq ($(BR2_PACKAGE_ELFUTILS),y)
> > +UFTRACE_DEPENDENCIES += elfutils
> > +endif
This optional dependency is a bit odd. How does the uftrace build
system detects the availability of elfutils? You're not passing any
specific option when elfutils is available or not available.
Best regards,
Thomas
--
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2021-04-04 10:07 ` Thomas Petazzoni
@ 2021-04-08 18:45 ` Asaf Kahlon
0 siblings, 0 replies; 9+ messages in thread
From: Asaf Kahlon @ 2021-04-08 18:45 UTC (permalink / raw)
To: buildroot
Hello Thomas,
On Sun, Apr 4, 2021 at 1:07 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> (In addition to Yann comments)
>
> On Sun, 4 Apr 2021 11:03:38 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > > +UFTRACE_LICENSE = GPL-2.0
> > > +UFTRACE_LICENSE_FILES = COPYING
> > > +
> > > +ifeq ($(BR2_PACKAGE_ELFUTILS),y)
> > > +UFTRACE_DEPENDENCIES += elfutils
> > > +endif
>
> This optional dependency is a bit odd. How does the uftrace build
> system detects the availability of elfutils? You're not passing any
> specific option when elfutils is available or not available.
uftrace does it strangely - it has a bunch of C files that check if a
possible extension exists.
Those test files are compiled anyway, or at least uftrace tries to
compile them and ignore the error in case of a failure.
After that, it checks if an executable with a specific name was
created. If yes, it adds the dependency.
So there's no flag that enables this dependency, it just has to be
installed and uftrace will try to add it.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Regards,
Asaf
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/uftrace: new package
2021-04-04 9:03 ` Yann E. MORIN
2021-04-04 10:07 ` Thomas Petazzoni
@ 2021-04-08 18:52 ` Asaf Kahlon
1 sibling, 0 replies; 9+ messages in thread
From: Asaf Kahlon @ 2021-04-08 18:52 UTC (permalink / raw)
To: buildroot
Hello Yann,
On Sun, Apr 4, 2021 at 12:03 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Asaf, All,
>
> On 2021-04-03 22:16 +0300, Asaf Kahlon spake thusly:
> > The uftrace tool is to trace and analyze execution of a
> > program written in C/C++. It was heavily inspired by the
> > ftrace framework of the Linux kernel (especially function
> > graph tracer) and supports userspace programs.
> > It supports various kind of commands and filters to help
> > analysis of the program execution and performance.
>
> Rather than duplicate the package description in the commit log, I
> think it makes more sense that it contains explanations about the
> packaging in Buildroot. See below.
I'll amend the message, thanks.
>
> > Signed-off-by: Asaf Kahlon <asafka7@gmail.com>
> > ---
> > DEVELOPERS | 1 +
> > package/Config.in | 1 +
> > package/busybox/busybox. | 0
>
> Uh? ;-)
Oops :) Will be fixed.
>
> [--SNIP--]
> > diff --git a/package/uftrace/Config.in b/package/uftrace/Config.in
> > new file mode 100644
> > index 0000000000..ca3041195e
> > --- /dev/null
> > +++ b/package/uftrace/Config.in
> > @@ -0,0 +1,12 @@
> > +config BR2_PACKAGE_UFTRACE
> > + bool "uftrace"
> > + depends on BR2_arm || BR2_aarch64 || BR2_x86_64
>
> AFAICS, it also support i386...
>
> Even though the architecture dependencies are pretty trivial today, I
> think it would be good to introduce a _ARCH_SUPPORTS symbol:
>
> config BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
> bool
> default y if BR2_arm
> default y if BR2_aarch64
> default y if BR2_i386
> default y if BR2_x86_64
>
> config BR2_PACKAGE_UFTRACE
> bool "uftrace"
> depends on BR2_PACKAGE_UFTRACE_ARCH_SUPPORTS
>
Done on v2.
> [--SNIP--]
> > diff --git a/package/uftrace/uftrace.mk b/package/uftrace/uftrace.mk
> > new file mode 100644
> > index 0000000000..b696ac4c6e
> > --- /dev/null
> > +++ b/package/uftrace/uftrace.mk
> > @@ -0,0 +1,26 @@
> > +################################################################################
> > +#
> > +# uftrace
> > +#
> > +################################################################################
> > +
> > +UFTRACE_VERSION = 0.9.4
> > +UFTRACE_SITE = https://github.com/namhyung/uftrace/archive/refs/tags
>
> Please use the github macro; see e.g. boot/shim/shim.mk and the manual:
> https://buildroot.org/downloads/manual/manual.html#github-download-url
>
Done in v2.
> > +UFTRACE_SOURCE = v0.9.4.tar.gz
>
> No need to specify _SOURCE when using the github macro.
>
> > +UFTRACE_LICENSE = GPL-2.0
> > +UFTRACE_LICENSE_FILES = COPYING
> > +
> > +ifeq ($(BR2_PACKAGE_ELFUTILS),y)
> > +UFTRACE_DEPENDENCIES += elfutils
> > +endif
> > +
> > +define UFTRACE_BUILD_CMDS
> > + $(TARGET_CONFIGURE_OPTS) ARCH=$(BR2_ARCH) $(MAKE) -C $(@D) $(UFTRACE_MAKE_OPTS)
>
> You are not setting UFTRACE_MAKE_OPTS anywhere that I can see...
You're right, removed.
>
> > +endef
> >
> > +define UFTRACE_INSTALL_TARGET_CMDS
> > + $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) \
> > + $(UFTRACE_MAKE_OPTS) install
> > +endef
> > +
> > +$(eval $(generic-package))
>
> There is a ./configure script (but it is not autotools, so this truly is
> a generic-package indeed): any reason why you are not using it? A little
> note about it in the commit log would be nice.
Well, the ./configure script is called from the Makefile, and actually
I don't see any reason to run it separately.
But I agree a proper commit message should be added in order to make
the compilation process clean.
>
> I was wondering if having a host variant would not be interesting too.
> Indeed, uftrace has a record mode, where it has a 'record' mode where it
> stores all the traces into a file, and replay/report/graph modes where
> it reads from that file to do off-sire analysis. So one might be
> interested in running in record mode in the target, and doing the
> analysis on the development machine...
That's an interesting idea. I admit I didn't try this use case.
I'll send a v2 soon, and I'll invest some time later to check the
option to add a host.
Anyway, thanks for the review!
>
> Thoughts?
>
> Regards,
> Yann E. MORIN.
>
> > --
> > 2.27.0
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
> | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
> '------------------------------^-------^------------------^--------------------'
Regards,
Asaf.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-08 18:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-03 19:16 [Buildroot] [PATCH 1/1] package/uftrace: new package Asaf Kahlon
2021-04-04 9:03 ` Yann E. MORIN
2021-04-04 10:07 ` Thomas Petazzoni
2021-04-08 18:45 ` Asaf Kahlon
2021-04-08 18:52 ` Asaf Kahlon
-- strict thread matches above, loose matches on Subject: below --
2020-10-26 23:14 Giacomo Longo
2020-10-27 9:36 ` Thomas Petazzoni
2020-10-27 15:14 ` Giacomo Longo
2020-11-03 20:39 ` Thomas Petazzoni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox