All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yann E. MORIN <yann.morin.1998@free.fr>
To: buildroot@busybox.net
Subject: [Buildroot] [PATCH] Fix issue with printvars executing giant shell command
Date: Mon, 10 Sep 2018 20:18:37 +0200	[thread overview]
Message-ID: <20180910181837.GG2674@scaer> (raw)
In-Reply-To: <1536601546.22056.12.camel@impinj.com>

Trent, All,

On 2018-09-10 17:45 +0000, Trent Piepho spake thusly:
> On Wed, 2018-08-29 at 22:49 +0200, Yann E. MORIN wrote:
> > On 2018-08-17 16:15 -0700, Trent Piepho spake thusly:
> > > 
> > > The solution here is to use $(strip $(foreach ...)), so the command
> > > expands to "@:", which make is smart enough to not even execute and
> > > wouldn't exceed any limits if it did.
> 
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -988,13 +988,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
> > >  # displayed.
> > >  .PHONY: printvars
> > >  printvars:
> > > -	@:$(foreach V, \
> > > +	@:$(strip $(foreach V, \
> > 
> > Why do we even need an actual command here? It works exactly the same
> > without a rule, and this indeed also gets rid of a call to a shell (and
> > consequently won't exceed the command line length limit).
> 
> Not quite exactly the same.  There's no longer a recipe at all, only
> the definition of a dependency for printvars, so the output will
> include:
> 
> make[1]: Nothing to be done for 'printvars'.

So what about:

diff --git a/Makefile b/Makefile
index 2c6af12989..57e776056c 100644
--- a/Makefile
+++ b/Makefile
@@ -988,7 +988,8 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:
+	$(foreach V, \
 		$(sort $(if $(VARS),$(filter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
 		$(if $(filter-out environment% default automatic, \
 				$(origin $V)), \

> Which isn't expected output for something that was parsing printvars.

Which should probably have already called it with 'make -s' because
that is what is also needed when printvars is called from an
out-of-tree build, by the way.

> I'll also mention that we saw this error running buildroot in certain
> docker containers, but not everywhere.  Whether or not make will pass a
> giant sequence of whitespace to a shell ends up not being so simple,
> and I couldn't find any precise rules that govern it.  So just because
> it works as desired in one test doesn't mean is must work that way
> everywhere.

Before my suggested hange:

    $ strace -ff -s 1048576 -o n.log -e trace=process -- make printvars >/dev/null
    $ grep execve n.log* |grep '                  '
    --> hit!

With the above change:

    $ strace -ff -s 1048576 -o n.log -e trace=process -- make printvars >/dev/null
    $ grep execve n.log* |grep '                  '
    --> no hit!

In a recipe, a command line that contains only spaces will not be executed
by make.

So with the above, you get the silence from a rule, and no long-shell
commands that break.

> However, it is clearly defined that strip will convert a sequence of
> whitespace to an empty string, so I figured better to relay on that.

I'm just trying to keep it simple.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

  reply	other threads:[~2018-09-10 18:18 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-17 23:15 [Buildroot] [PATCH] Fix issue with printvars executing giant shell command Trent Piepho
2018-08-29 20:49 ` Yann E. MORIN
2018-09-10 17:45   ` Trent Piepho
2018-09-10 18:18     ` Yann E. MORIN [this message]
2018-09-17 18:02       ` Trent Piepho
2018-09-17 19:54         ` Yann E. MORIN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180910181837.GG2674@scaer \
    --to=yann.morin.1998@free.fr \
    --cc=buildroot@busybox.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.