[2/3] package/strace: add missing libunwind optional dependency

Message ID 20180225183415.8362-2-romain.naour@gmail.com
State Accepted
Headers show
Series
  • [1/3] package/strace: bump to 4.21
Related show

Commit Message

Romain Naour Feb. 25, 2018, 6:34 p.m.
This improve the reproducible build.

Signed-off-by: Romain Naour <romain.naour@gmail.com>
---
 package/strace/strace.mk | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Thomas De Schampheleire Feb. 25, 2018, 8:28 p.m. | #1
Hi,

2018-02-25 19:34 GMT+01:00 Romain Naour <romain.naour@gmail.com>:
> This improve the reproducible build.
>
> Signed-off-by: Romain Naour <romain.naour@gmail.com>
> ---
>  package/strace/strace.mk | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/package/strace/strace.mk b/package/strace/strace.mk
> index e292c60f2a..9c1fab3871 100644
> --- a/package/strace/strace.mk
> +++ b/package/strace/strace.mk
> @@ -10,6 +10,13 @@ STRACE_SITE = https://github.com/strace/strace/releases/download/v$(STRACE_VERSI
>  STRACE_LICENSE = BSD-3-Clause
>  STRACE_LICENSE_FILES = COPYING
>
> +ifeq ($(BR2_PACKAGE_LIBUNWIND),y)
> +STRACE_DEPENDENCIES += libunwind
> +STRACE_CONF_OPTS += --with-libunwind
> +else
> +STRACE_CONF_OPTS += --without-libunwind
> +endif
> +

As far as I know, the main (only?) reason for a libunwind dependency
is to support 'strace -k' to get callstack info. Perhaps it would be
useful to update the Config.in file to hint about this, i.e. that
users should enable libunwind manually if they want 'strace -k'
support.

In our local tree, I had added a config option to strace for that, but
perhaps that is overkill?

/Thomas
Romain Naour Feb. 25, 2018, 8:50 p.m. | #2
Hi Thomas,

Le 25/02/2018 à 21:28, Thomas De Schampheleire a écrit :
> Hi,
> 
> 2018-02-25 19:34 GMT+01:00 Romain Naour <romain.naour@gmail.com>:
>> This improve the reproducible build.
>>
>> Signed-off-by: Romain Naour <romain.naour@gmail.com>
>> ---
>>  package/strace/strace.mk | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/package/strace/strace.mk b/package/strace/strace.mk
>> index e292c60f2a..9c1fab3871 100644
>> --- a/package/strace/strace.mk
>> +++ b/package/strace/strace.mk
>> @@ -10,6 +10,13 @@ STRACE_SITE = https://github.com/strace/strace/releases/download/v$(STRACE_VERSI
>>  STRACE_LICENSE = BSD-3-Clause
>>  STRACE_LICENSE_FILES = COPYING
>>
>> +ifeq ($(BR2_PACKAGE_LIBUNWIND),y)
>> +STRACE_DEPENDENCIES += libunwind
>> +STRACE_CONF_OPTS += --with-libunwind
>> +else
>> +STRACE_CONF_OPTS += --without-libunwind
>> +endif
>> +
> 
> As far as I know, the main (only?) reason for a libunwind dependency
> is to support 'strace -k' to get callstack info. Perhaps it would be
> useful to update the Config.in file to hint about this, i.e. that
> users should enable libunwind manually if they want 'strace -k'
> support.

Ok, why not.

> 
> In our local tree, I had added a config option to strace for that, but
> perhaps that is overkill?

Most packages optional dependencies are handled like this, otherwise we have to
maintain one config option for each optional dependencies.

Best regards,
Romain

> 
> /Thomas
>
Thomas Petazzoni Feb. 25, 2018, 9:34 p.m. | #3
Hello Thomas,

On Sun, 25 Feb 2018 21:28:19 +0100, Thomas De Schampheleire wrote:

> In our local tree, I had added a config option to strace for that, but
> perhaps that is overkill?

Are you saying that you have changes in your local tree on packages
available in upstream Buildroot, and you did not submit such
changes ? :-)

Thomas
Thomas De Schampheleire Feb. 25, 2018, 9:58 p.m. | #4
2018-02-25 22:34 GMT+01:00 Thomas Petazzoni <thomas.petazzoni@bootlin.com>:
> Hello Thomas,
>
> On Sun, 25 Feb 2018 21:28:19 +0100, Thomas De Schampheleire wrote:
>
>> In our local tree, I had added a config option to strace for that, but
>> perhaps that is overkill?
>
> Are you saying that you have changes in your local tree on packages
> available in upstream Buildroot, and you did not submit such
> changes ? :-)
>

Haha, yes indeed :-) But you may be glad to hear that I already made a
list of such changes that should be upstreamable, and plan to tackle
the upstreaming after merging 2018.02!
Peter Korsgaard March 31, 2018, 8:47 p.m. | #5
>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

Hi,

 >>> In our local tree, I had added a config option to strace for that, but
 >>> perhaps that is overkill?
 >> 
 >> Are you saying that you have changes in your local tree on packages
 >> available in upstream Buildroot, and you did not submit such
 >> changes ? :-)
 >> 

 > Haha, yes indeed :-) But you may be glad to hear that I already made a
 > list of such changes that should be upstreamable, and plan to tackle
 > the upstreaming after merging 2018.02!

Did you already get 2018.02 merged? ;)
Thomas De Schampheleire April 1, 2018, 8:04 a.m. | #6
On Sat, Mar 31, 2018, 22:47 Peter Korsgaard <peter@korsgaard.com> wrote:

> >>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com>
> writes:
>
> Hi,
>
>  >>> In our local tree, I had added a config option to strace for that, but
>  >>> perhaps that is overkill?
>  >>
>  >> Are you saying that you have changes in your local tree on packages
>  >> available in upstream Buildroot, and you did not submit such
>  >> changes ? :-)
>  >>
>
>  > Haha, yes indeed :-) But you may be glad to hear that I already made a
>  > list of such changes that should be upstreamable, and plan to tackle
>  > the upstreaming after merging 2018.02!
>
> Did you already get 2018.02 merged? ;)
>

The code is ready, but the drop was postponed due to other issues...

I'll get to it, don't worry :)
<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Mar 31, 2018, 22:47 Peter Korsgaard &lt;<a href="mailto:peter@korsgaard.com">peter@korsgaard.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">&gt;&gt;&gt;&gt;&gt; &quot;Thomas&quot; == Thomas De Schampheleire &lt;<a href="mailto:patrickdepinguin@gmail.com" target="_blank" rel="noreferrer">patrickdepinguin@gmail.com</a>&gt; writes:<br>
<br>
Hi,<br>
<br>
 &gt;&gt;&gt; In our local tree, I had added a config option to strace for that, but<br>
 &gt;&gt;&gt; perhaps that is overkill?<br>
 &gt;&gt;<br>
 &gt;&gt; Are you saying that you have changes in your local tree on packages<br>
 &gt;&gt; available in upstream Buildroot, and you did not submit such<br>
 &gt;&gt; changes ? :-)<br>
 &gt;&gt;<br>
<br>
 &gt; Haha, yes indeed :-) But you may be glad to hear that I already made a<br>
 &gt; list of such changes that should be upstreamable, and plan to tackle<br>
 &gt; the upstreaming after merging 2018.02!<br>
<br>
Did you already get 2018.02 merged? ;)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The code is ready, but the drop was postponed due to other issues...</div><div dir="auto"><br></div><div dir="auto">I&#39;ll get to it, don&#39;t worry :)</div></div>
Thomas Petazzoni April 6, 2018, 9:38 p.m. | #7
Hello,

On Sun, 25 Feb 2018 19:34:14 +0100, Romain Naour wrote:
> This improve the reproducible build.
> 
> Signed-off-by: Romain Naour <romain.naour@gmail.com>
> ---
>  package/strace/strace.mk | 7 +++++++
>  1 file changed, 7 insertions(+)

I've updated the Config.in help text to indicate that libunwind should
be enabled to get "strace -k" support, as suggested by Thomas DS. And
then I've applied. Thanks!

Thomas

Patch

diff --git a/package/strace/strace.mk b/package/strace/strace.mk
index e292c60f2a..9c1fab3871 100644
--- a/package/strace/strace.mk
+++ b/package/strace/strace.mk
@@ -10,6 +10,13 @@  STRACE_SITE = https://github.com/strace/strace/releases/download/v$(STRACE_VERSI
 STRACE_LICENSE = BSD-3-Clause
 STRACE_LICENSE_FILES = COPYING
 
+ifeq ($(BR2_PACKAGE_LIBUNWIND),y)
+STRACE_DEPENDENCIES += libunwind
+STRACE_CONF_OPTS += --with-libunwind
+else
+STRACE_CONF_OPTS += --without-libunwind
+endif
+
 # strace bundle some kernel headers to build libmpers, this mixes userspace
 # headers and kernel headers which break the build with musl.
 # The stddef.h from gcc is used instead of the one from musl.