Patchwork [2/2] lsof: add busybox tweaks

login
register
mail settings
Submitter Gustavo Zacarias
Date April 26, 2012, 10:15 a.m.
Message ID <1335435324-18792-2-git-send-email-gustavo@zacarias.com.ar>
Download mbox | patch
Permalink /patch/155225/
State Accepted
Commit d7616cc9be8b1c7d91df9133dac24a59bb2e19c8
Headers show

Comments

Gustavo Zacarias - April 26, 2012, 10:15 a.m.
Since busybox 1.20+ includes a lsof applet make sure lsof gets built
after busybox so that we get the full-blown version if both are enabled.
Also hide the lsof package unless BUSYBOX_SHOW_OTHERS is true.

Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
---
 package/Config.in    |    2 ++
 package/lsof/lsof.mk |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)
Jean-Christophe PLAGNIOL-VILLARD - April 26, 2012, 2:08 p.m.
On 07:15 Thu 26 Apr     , Gustavo Zacarias wrote:
> Since busybox 1.20+ includes a lsof applet make sure lsof gets built
> after busybox so that we get the full-blown version if both are enabled.
> Also hide the lsof package unless BUSYBOX_SHOW_OTHERS is true.
> 
> Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
> ---
>  package/Config.in    |    2 ++
>  package/lsof/lsof.mk |    3 +++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/package/Config.in b/package/Config.in
> index 64fce62..7f9b234 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -24,7 +24,9 @@ source "package/dmalloc/Config.in"
>  source "package/kexec/Config.in"
>  source "package/latencytop/Config.in"
>  source "package/lmbench/Config.in"
> +if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>  source "package/lsof/Config.in"
put a depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS


in lsof/Config.in

Best Regards,
J.
Thomas Petazzoni - April 26, 2012, 2:45 p.m.
Le Thu, 26 Apr 2012 16:08:40 +0200,
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> a écrit :

> > +if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> >  source "package/lsof/Config.in"
> put a depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> 
> in lsof/Config.in

No, that's not what we do for all other packages. Gustavo's patch is
correct with regard to the current guidelines.

Now, whether we should change those guidelines to something else is a
different story. But if we change the guideline, it should be changed
for all packages and not just for lsof.

Regards,

Thomas
Jean-Christophe PLAGNIOL-VILLARD - April 27, 2012, 12:55 a.m.
On 16:45 Thu 26 Apr     , Thomas Petazzoni wrote:
> Le Thu, 26 Apr 2012 16:08:40 +0200,
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> a écrit :
> 
> > > +if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> > >  source "package/lsof/Config.in"
> > put a depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> > 
> > in lsof/Config.in
> 
> No, that's not what we do for all other packages. Gustavo's patch is
> correct with regard to the current guidelines.
> 
> Now, whether we should change those guidelines to something else is a
> different story. But if we change the guideline, it should be changed
> for all packages and not just for lsof.
package/kmod/Config.in is already like this

and it's more clean

Best Regards,
J.
Arnout Vandecappelle - April 27, 2012, 9:54 p.m.
On Friday 27 April 2012 02:55:58 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > No, that's not what we do for all other packages. Gustavo's patch is
> > correct with regard to the current guidelines.
> > 
> > Now, whether we should change those guidelines to something else is a
> > different story. But if we change the guideline, it should be changed
> > for all packages and not just for lsof.
> package/kmod/Config.in is already like this
> 
> and it's more clean

 I agree that it's better to do it in the package's Config.in.

 And changing the guideline doesn't automatically mean that all
packages have to change.  Only new packages (or patches) must follow 
the guideline.

 Regards,
 Arnout
Peter Korsgaard - April 29, 2012, 8:36 a.m.
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> > Now, whether we should change those guidelines to something else is a
 >> > different story. But if we change the guideline, it should be changed
 >> > for all packages and not just for lsof.
 >> package/kmod/Config.in is already like this

The main reason why it's done like this is that it's only the _TOOLS
suboption that should depend on the _SHOW_OTHERS option.

 >> and it's more clean

 Arnout>  I agree that it's better to do it in the package's Config.in.

 Arnout>  And changing the guideline doesn't automatically mean that all
 Arnout> packages have to change.  Only new packages (or patches) must follow 
 Arnout> the guideline.

True. But the first thing to decide on is if this _SHOW_OTHERS option is
really useful. It was afaik introduced long time ago to encourage people
to use the busybox applets instead of the full blown versions, but
perhaps we should just let people decide for themselves?
Peter Korsgaard - April 29, 2012, 8:56 a.m.
>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes:

 Gustavo> Since busybox 1.20+ includes a lsof applet make sure lsof gets
 Gustavo> built after busybox so that we get the full-blown version if
 Gustavo> both are enabled.  Also hide the lsof package unless
 Gustavo> BUSYBOX_SHOW_OTHERS is true.

Committed, thanks.
Gustavo Zacarias - April 29, 2012, 10:49 a.m.
On 2012-04-29 05:36, Peter Korsgaard wrote:

> True. But the first thing to decide on is if this _SHOW_OTHERS option 
> is
> really useful. It was afaik introduced long time ago to encourage 
> people
> to use the busybox applets instead of the full blown versions, but
> perhaps we should just let people decide for themselves?

I'd just kill it altogether, i'm not a fan of hidden options since 
they're prone to noise (people asking where it is) if they can be 
avoided.
And probably use Arnout's idea of appending it to the menu text or 
help.
Regards.

Patch

diff --git a/package/Config.in b/package/Config.in
index 64fce62..7f9b234 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -24,7 +24,9 @@  source "package/dmalloc/Config.in"
 source "package/kexec/Config.in"
 source "package/latencytop/Config.in"
 source "package/lmbench/Config.in"
+if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
 source "package/lsof/Config.in"
+endif
 source "package/ltp-testsuite/Config.in"
 source "package/lttng-babeltrace/Config.in"
 source "package/lttng-modules/Config.in"
diff --git a/package/lsof/lsof.mk b/package/lsof/lsof.mk
index c07fb2d..7bc22c0 100644
--- a/package/lsof/lsof.mk
+++ b/package/lsof/lsof.mk
@@ -8,6 +8,9 @@  LSOF_VERSION = 4.85
 LSOF_SOURCE = lsof_$(LSOF_VERSION).tar.bz2
 LSOF_SITE = ftp://lsof.itap.purdue.edu/pub/tools/unix/lsof/
 
+# Make certain full-blown lsof gets built after the busybox version (1.20+)
+LSOF_DEPENDENCIES += $(if $(BR2_PACKAGE_BUSYBOX),busybox)
+
 BR2_LSOF_CFLAGS =
 ifeq ($(BR2_LARGEFILE),)
 BR2_LSOF_CFLAGS += -U_FILE_OFFSET_BITS