diff mbox series

[1/1] package/uftrace: new package

Message ID 20210403191609.1168606-1-asafka7@gmail.com
State Changes Requested
Headers show
Series [1/1] package/uftrace: new package | expand

Commit Message

Asaf Kahlon April 3, 2021, 7:16 p.m. UTC 
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

Comments

Yann E. MORIN April 4, 2021, 9:03 a.m. UTC | #1 
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@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni April 4, 2021, 10:07 a.m. UTC | #2 
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
Asaf Kahlon April 8, 2021, 6:45 p.m. UTC | #3 
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
Asaf Kahlon April 8, 2021, 6:52 p.m. UTC | #4 
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@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.
diff mbox series

Patch

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))