[Buildroot] [PATCHv2] Makefile: fix show-vars for good this time

[Buildroot] [PATCHv2] Makefile: fix show-vars for good this time

[Yann E. MORIN]

Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars
and 'show-vars') did not fully fix the show-vars case, which still
segfaults.

Overall, show-vars generates a JSON blurb. That is supposed to be
machine-readable, so we do not care that the variables are sorted, so
we get rid of it to (slightly) simplify the code.

Then, we currently iterate twice on the list of variables: the first one
to filter-out the 'internal' variables, and the second one to filter
only the variables matching the pattern. We can do away by iterating
only once, and applying both filters at once.

Since we now have an 'and' condition, we can take advantage of it: when
none of the items in $(and) are empty, $(and) evaluates to the last
item, while it evaluates to empty if any of the items is empty. So we
can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is
equivalent to: $(and a,b,c) ; this gains us one parentheses depth.

Finally, the cause for the segfault is an overly-long call to $(info).
Reducing that is not easy: we want to call clean-json on the whole of
the JSON blurb, so we can't emit the individual variables one by one, or
the trailing comma would not be trimmed away.

So, we go crazy: we just output each word from clean-json with $(info).

We can do that, because mk-json-str transforms all spaces in a string
to an escaped UTF-8 sequence, so we will never have spaces in values;
the keys are the variables, so they won't have spaces either; spaces in
the rest of the JSON blurb are totally optional, so we don't care how
many there are. We know there are spaces, because we explicitly
introduce some (after "expanded" or "raw", for example), so we should
never hit a too-big word for $(info) to print.

Thanks to Henri for the suggestion to push $(info) further inside the
macro.

Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Roosen Henri <Henri.Roosen@ginzinger.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

---
Changes v1 -> v2:
  - coalesce $(if) with $(and)
  - don't output a trailing $(space) when calling $(info)
---
 Makefile | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index a743e42f91..07b594ea8a 100644
--- a/Makefile
+++ b/Makefile
@@ -1073,17 +1073,24 @@ printvars:
 show-vars: VARS?=%
 show-vars:
 	@:
-	$(info $(call clean-json, { \
+	$(foreach i, \
+		$(call clean-json, { \
 			$(foreach V, \
-				$(sort $(foreach X, $(.VARIABLES), $(filter $(VARS),$(X)))), \
-				$(if $(filter-out environment% default automatic, $(origin $V)), \
+				$(.VARIABLES), \
+				$(and $(filter $(VARS),$(V)) \
+					, \
+					$(filter-out environment% default automatic, $(origin $V)) \
+					, \
 					"$V": { \
 						"expanded": $(call mk-json-str,$($V))$(comma) \
 						"raw": $(call mk-json-str,$(value $V)) \
 					}$(comma) \
 				) \
 			) \
-	} ))
+		} ) \
+		, \
+		$(info $(i)) \
+	)
 
 .PHONY: clean
 clean:
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv2] Makefile: fix show-vars for good this time

[Quentin Schulz]

Hi Yann,

On 8/1/22 22:42, Yann E. MORIN wrote:
> Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars
> and 'show-vars') did not fully fix the show-vars case, which still
> segfaults.
> 
> Overall, show-vars generates a JSON blurb. That is supposed to be
> machine-readable, so we do not care that the variables are sorted, so
> we get rid of it to (slightly) simplify the code.
> 
> Then, we currently iterate twice on the list of variables: the first one
> to filter-out the 'internal' variables, and the second one to filter
> only the variables matching the pattern. We can do away by iterating
> only once, and applying both filters at once.
> 
> Since we now have an 'and' condition, we can take advantage of it: when
> none of the items in $(and) are empty, $(and) evaluates to the last
> item, while it evaluates to empty if any of the items is empty. So we
> can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is
> equivalent to: $(and a,b,c) ; this gains us one parentheses depth.
> 
> Finally, the cause for the segfault is an overly-long call to $(info).
> Reducing that is not easy: we want to call clean-json on the whole of
> the JSON blurb, so we can't emit the individual variables one by one, or
> the trailing comma would not be trimmed away.
> 
> So, we go crazy: we just output each word from clean-json with $(info).
> 
> We can do that, because mk-json-str transforms all spaces in a string
> to an escaped UTF-8 sequence, so we will never have spaces in values;
> the keys are the variables, so they won't have spaces either; spaces in
> the rest of the JSON blurb are totally optional, so we don't care how
> many there are. We know there are spaces, because we explicitly
> introduce some (after "expanded" or "raw", for example), so we should
> never hit a too-big word for $(info) to print.
> 
> Thanks to Henri for the suggestion to push $(info) further inside the
> macro.
> 
> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Roosen Henri <Henri.Roosen@ginzinger.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 

Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Upon request, "time efficiency" regression test on old make:

 From within debian:buster container (make 4.2.1), from master 
2a592f0b2a400825a95e19113ce996bbc5c431d7:

# make raspberrypi4_defconfig
# time make show-vars > foo.json

real	0m19.001s
user	0m15.828s
sys	0m3.305s
# time make printvars VARS='%' > foo.txt

real	0m16.271s
user	0m13.056s
sys	0m3.360s
# ls -l foo.json foo.txt
-rw-r--r--. 1 qschulz qschulz 62448149 Aug  2 09:06 foo.json
-rw-r--r--. 1 qschulz qschulz 32813206 Aug  2 09:07 foo.txt

 From within debian:buster container (make 4.2.1), with this patch NOT 
applied and 5c54c3ef3d commit reverted:

# make raspberrypi4_defconfig
# time make show-vars > foo.json

real	0m24.325s
user	0m19.617s
sys	0m4.905s
# time make printvars VARS='%' > foo.txt

real	0m16.223s
user	0m12.895s
sys	0m3.469s
# ls -l foo.json foo.txt
-rw-r--r--. 1 qschulz qschulz 62448149 Aug  2 09:04 foo.json
-rw-r--r--. 1 qschulz qschulz 32813206 Aug  2 09:09 foo.txt

 From within debian:buster container (make 4.2.1), with this patch applied:

# make raspberrypi4_defconfig
# time make show-vars > foo.json

real	0m22.634s
user	0m17.542s
sys	0m5.202s
# time make printvars VARS='%' > foo.txt

real	0m16.829s
user	0m13.381s
sys	0m3.579s
# ls -l foo.json foo.txt
-rw-r--r--. 1 qschulz qschulz 62448149 Aug  2 09:02 foo.json
-rw-r--r--. 1 qschulz qschulz 32813206 Aug  2 09:14 foo.txt

I wouldn't trust too much the reports from the time tool because I had 
some runs where it was twice longer for some reason.

In short, it does not seem to impact too much time-wise.

Thanks,
Quentin
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv2] Makefile: fix show-vars for good this time

[Thomas Petazzoni via buildroot]

On Mon,  1 Aug 2022 22:42:27 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars
> and 'show-vars') did not fully fix the show-vars case, which still
> segfaults.
> 
> Overall, show-vars generates a JSON blurb. That is supposed to be
> machine-readable, so we do not care that the variables are sorted, so
> we get rid of it to (slightly) simplify the code.
> 
> Then, we currently iterate twice on the list of variables: the first one
> to filter-out the 'internal' variables, and the second one to filter
> only the variables matching the pattern. We can do away by iterating
> only once, and applying both filters at once.
> 
> Since we now have an 'and' condition, we can take advantage of it: when
> none of the items in $(and) are empty, $(and) evaluates to the last
> item, while it evaluates to empty if any of the items is empty. So we
> can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is
> equivalent to: $(and a,b,c) ; this gains us one parentheses depth.
> 
> Finally, the cause for the segfault is an overly-long call to $(info).
> Reducing that is not easy: we want to call clean-json on the whole of
> the JSON blurb, so we can't emit the individual variables one by one, or
> the trailing comma would not be trimmed away.
> 
> So, we go crazy: we just output each word from clean-json with $(info).
> 
> We can do that, because mk-json-str transforms all spaces in a string
> to an escaped UTF-8 sequence, so we will never have spaces in values;
> the keys are the variables, so they won't have spaces either; spaces in
> the rest of the JSON blurb are totally optional, so we don't care how
> many there are. We know there are spaces, because we explicitly
> introduce some (after "expanded" or "raw", for example), so we should
> never hit a too-big word for $(info) to print.
> 
> Thanks to Henri for the suggestion to push $(info) further inside the
> macro.
> 
> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Roosen Henri <Henri.Roosen@ginzinger.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> ---
> Changes v1 -> v2:
>   - coalesce $(if) with $(and)
>   - don't output a trailing $(space) when calling $(info)
> ---
>  Makefile | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv2] Makefile: fix show-vars for good this time

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars
 > and 'show-vars') did not fully fix the show-vars case, which still
 > segfaults.

 > Overall, show-vars generates a JSON blurb. That is supposed to be
 > machine-readable, so we do not care that the variables are sorted, so
 > we get rid of it to (slightly) simplify the code.

 > Then, we currently iterate twice on the list of variables: the first one
 > to filter-out the 'internal' variables, and the second one to filter
 > only the variables matching the pattern. We can do away by iterating
 > only once, and applying both filters at once.

 > Since we now have an 'and' condition, we can take advantage of it: when
 > none of the items in $(and) are empty, $(and) evaluates to the last
 > item, while it evaluates to empty if any of the items is empty. So we
 > can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is
 > equivalent to: $(and a,b,c) ; this gains us one parentheses depth.

 > Finally, the cause for the segfault is an overly-long call to $(info).
 > Reducing that is not easy: we want to call clean-json on the whole of
 > the JSON blurb, so we can't emit the individual variables one by one, or
 > the trailing comma would not be trimmed away.

 > So, we go crazy: we just output each word from clean-json with $(info).

 > We can do that, because mk-json-str transforms all spaces in a string
 > to an escaped UTF-8 sequence, so we will never have spaces in values;
 > the keys are the variables, so they won't have spaces either; spaces in
 > the rest of the JSON blurb are totally optional, so we don't care how
 > many there are. We know there are spaces, because we explicitly
 > introduce some (after "expanded" or "raw", for example), so we should
 > never hit a too-big word for $(info) to print.

 > Thanks to Henri for the suggestion to push $(info) further inside the
 > macro.

 > Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
 > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
 > Cc: Roosen Henri <Henri.Roosen@ginzinger.com>
 > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

 > ---
 > Changes v1 -> v2:
 >   - coalesce $(if) with $(and)
 >   - don't output a trailing $(space) when calling $(info)

Committed to 2022.05.x and 2022.02.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot