[PATCH] Makefile: use shell to calculate map_size

[PATCH] Makefile: use shell to calculate map_size

[Leon M. Busch-George]

From: "Leon M. Busch-George" <leon@georgemail.eu>

The error message "bc: command not found" is easily missed since the
build continues.
bc is not a part of coreutils or base-devel. POSIX sh can also do the
calculation.

Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 7a3209bd9e..41dc4baad2 100644
--- a/Makefile
+++ b/Makefile
@@ -1275,9 +1275,9 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
 binary_size_check: u-boot-nodtb.bin FORCE
 	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \
 	map_size=$(shell cat u-boot.map | \
-		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
+		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "echo $$((0x" toupper(end) " - 0x" toupper(start) "))"}' \
 		| sed 's/0X//g' \
-		| bc); \
+		| sh); \
 	if [ "" != "$$map_size" ]; then \
 		if test $$map_size -ne $$file_size; then \
 			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
-- 
2.44.0

Re: [PATCH] Makefile: use shell to calculate map_size

[Dragan Simic]

Hello Leon,

On 2024-03-02 14:17, Leon M. Busch-George wrote:
> From: "Leon M. Busch-George" <leon@georgemail.eu>
> 
> The error message "bc: command not found" is easily missed since the
> build continues.
> bc is not a part of coreutils or base-devel. POSIX sh can also do the
> calculation.
> 
> Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>

Please, see a nitpick below.  Otherwise, the patch is looking
good to me.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7a3209bd9e..41dc4baad2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1275,9 +1275,9 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
>  binary_size_check: u-boot-nodtb.bin FORCE
>  	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \
>  	map_size=$(shell cat u-boot.map | \
> -		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end =
> $$1} END {if (start != "" && end != "") print "ibase=16; "
> toupper(end) " - " toupper(start)}' \
> +		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end =
> $$1} END {if (start != "" && end != "") print "echo $$((0x"
> toupper(end) " - 0x" toupper(start) "))"}' \
>  		| sed 's/0X//g' \
> -		| bc); \
> +		| sh); \

Maybe "sh -s" could be used instead, just for some additional
strictness.

>  	if [ "" != "$$map_size" ]; then \
>  		if test $$map_size -ne $$file_size; then \
>  			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \

Re: [PATCH] Makefile: use shell to calculate map_size

[Leon Busch-George]

Oops!
That should have went to the list as well...

On Mon, 4 Mar 2024 15:40:07 +0100
Leon Busch-George <leon@georgemail.eu> wrote:

> Hi Dragan :-)
> 
> Thanks for your reply!
> 
> On Sat, 02 Mar 2024 22:13:08 +0100
> Dragan Simic <dsimic@manjaro.org> wrote:
> 
> > > +		awk '/_image_copy_start/ {start = $$1}
> > > /_image_binary_end/ {end = $$1} END {if (start != "" && end != "")
> > > print "echo $$((0x" toupper(end) " - 0x" toupper(start) "))"}' \
> > >  		| sed 's/0X//g' \
> > > -		| bc); \
> > > +		| sh); \
> > 
> > Maybe "sh -s" could be used instead, just for some additional
> > strictness.
> 
> -s is the default already but I see no reason against adding it.
> Allow me to offer another idea to improve strictness (I'll send a v2):
> 
>     awk '.. print end " " start ..' | sh -c 'read end start; echo
> $((end - start))'
> 
> That gets rid off sed and the interface between awk and sh is much
> cleaner (only the two numbers on one line rather than shell code).
> Sadly, the sed 's/0X//g' was introduced without an explanation in
> 3ce7a4fefa and but, looking at it more, I'm farly confident it was
> only for bc.
> 
> kind regards,
> Leon

[PATCH v2] Makefile: use shell to calculate map_size

[Leon M. Busch-George]

From: "Leon M. Busch-George" <leon@georgemail.eu>

The error message "bc: command not found" is easily missed since the
build continues.
bc is not a part of coreutils or base-devel. POSIX sh can also do the
calculation.

Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
---
 Makefile | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index a2bc9d5903..e8e794368e 100644
--- a/Makefile
+++ b/Makefile
@@ -1275,10 +1275,15 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
 binary_size_check: u-boot-nodtb.bin FORCE
 	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \
 	map_size=$(shell cat u-boot.map | \
-		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
-		| sed 's/0X//g' \
-		| bc); \
-	if [ "" != "$$map_size" ]; then \
+		awk ' \
+			/_image_copy_start/ { start = $$1 } \
+			/_image_binary_end/ { end = $$1 } \
+			END { \
+				if (start != "" && end != "") \
+					print end " " start; \
+			}' \
+		| sh -c 'read end start; [ -n "$$end" ] && echo $$((end - start))'); \
+	if [ -n "$$map_size" ]; then \
 		if test $$map_size -ne $$file_size; then \
 			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
 			echo "  but u-boot-nodtb.bin shows $$file_size" >&2 ; \
-- 
2.44.0

Re: [PATCH v2] Makefile: use shell to calculate map_size

[Leon Busch-George]

A colleague made me aware that the '[ -n "$$end" ]' is not necessary since 'read' already returns an exit code.

v3 inc

On Mon,  4 Mar 2024 21:38:56 +0100
"Leon M. Busch-George" <leon@georgemail.de> wrote:

> From: "Leon M. Busch-George" <leon@georgemail.eu>
> 
> The error message "bc: command not found" is easily missed since the
> build continues.
> bc is not a part of coreutils or base-devel. POSIX sh can also do the
> calculation.
> 
> Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
> ---
>  Makefile | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index a2bc9d5903..e8e794368e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1275,10 +1275,15 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
>  binary_size_check: u-boot-nodtb.bin FORCE
>  	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print
> $$1}') ; \ map_size=$(shell cat u-boot.map | \
> -		awk '/_image_copy_start/ {start = $$1}
> /_image_binary_end/ {end = $$1} END {if (start != "" && end != "")
> print "ibase=16; " toupper(end) " - " toupper(start)}' \
> -		| sed 's/0X//g' \
> -		| bc); \
> -	if [ "" != "$$map_size" ]; then \
> +		awk ' \
> +			/_image_copy_start/ { start = $$1 } \
> +			/_image_binary_end/ { end = $$1 } \
> +			END { \
> +				if (start != "" && end != "") \
> +					print end " " start; \
> +			}' \
> +		| sh -c 'read end start; [ -n "$$end" ] && echo
> $$((end - start))'); \
> +	if [ -n "$$map_size" ]; then \
>  		if test $$map_size -ne $$file_size; then \
>  			echo "u-boot.map shows a binary size of
> $$map_size" >&2 ; \ echo "  but u-boot-nodtb.bin shows $$file_size"
> >&2 ; \

[PATCH v3] Makefile: use shell to calculate map_size

[Leon M. Busch-George]

From: "Leon M. Busch-George" <leon@georgemail.eu>

The error message "bc: command not found" is easily missed since the
build continues.
bc is not a part of coreutils or base-devel. POSIX sh can also do the
calculation.

Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
---
 Makefile | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index a2bc9d5903..0aef07ef3b 100644
--- a/Makefile
+++ b/Makefile
@@ -1275,10 +1275,15 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
 binary_size_check: u-boot-nodtb.bin FORCE
 	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \
 	map_size=$(shell cat u-boot.map | \
-		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
-		| sed 's/0X//g' \
-		| bc); \
-	if [ "" != "$$map_size" ]; then \
+		awk ' \
+			/_image_copy_start/ { start = $$1 } \
+			/_image_binary_end/ { end = $$1 } \
+			END { \
+				if (start != "" && end != "") \
+					print end " " start; \
+			}' \
+		| sh -c 'read end start && echo $$((end - start))'); \
+	if [ -n "$$map_size" ]; then \
 		if test $$map_size -ne $$file_size; then \
 			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
 			echo "  but u-boot-nodtb.bin shows $$file_size" >&2 ; \
-- 
2.44.0

Re: [PATCH v2] Makefile: use shell to calculate map_size

[Dragan Simic]

Hello Leon,

On 2024-03-04 21:44, Dragan Simic wrote:
> On 2024-03-04 21:38, Leon M. Busch-George wrote:
>> From: "Leon M. Busch-George" <leon@georgemail.eu>
>> 
>> The error message "bc: command not found" is easily missed since the
>> build continues.
>> bc is not a part of coreutils or base-devel. POSIX sh can also do the
>> calculation.
>> 
>> Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>

The v2 is looking good to me.  I also did some testing of the awk code
by hand, outside of the actual Makefile, and it worked as expected.

See also one small nitpick below, and please feel free to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

>> ---
>>  Makefile | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Makefile b/Makefile
>> index a2bc9d5903..e8e794368e 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1275,10 +1275,15 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
>>  binary_size_check: u-boot-nodtb.bin FORCE
>>  	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \

Perhaps a couple of spaces could be added around "print $$1" in the
line above, to nicely round off the code cleanups.

>>  	map_size=$(shell cat u-boot.map | \
>> -		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end =
>> $$1} END {if (start != "" && end != "") print "ibase=16; "
>> toupper(end) " - " toupper(start)}' \
>> -		| sed 's/0X//g' \
>> -		| bc); \
>> -	if [ "" != "$$map_size" ]; then \
>> +		awk ' \
>> +			/_image_copy_start/ { start = $$1 } \
>> +			/_image_binary_end/ { end = $$1 } \
>> +			END { \
>> +				if (start != "" && end != "") \
>> +					print end " " start; \
>> +			}' \
>> +		| sh -c 'read end start; [ -n "$$end" ] && echo $$((end - 
>> start))'); \
>> +	if [ -n "$$map_size" ]; then \
>>  		if test $$map_size -ne $$file_size; then \
>>  			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
>>  			echo "  but u-boot-nodtb.bin shows $$file_size" >&2 ; \

Re: [PATCH v3] Makefile: use shell to calculate map_size

[Dragan Simic]

On 2024-03-05 18:15, Dragan Simic wrote:
> On 2024-03-05 17:46, Leon M. Busch-George wrote:
>> From: "Leon M. Busch-George" <leon@georgemail.eu>
>> 
>> The error message "bc: command not found" is easily missed since the
>> build continues.
>> bc is not a part of coreutils or base-devel. POSIX sh can also do the
>> calculation.
>> 
>> Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>

Sorry, I totally missed the v3.  It's still looking good to me, and my
Reviewed-by tag still applies, together with the small nitpick.

>> ---
>>  Makefile | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Makefile b/Makefile
>> index a2bc9d5903..0aef07ef3b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1275,10 +1275,15 @@ OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
>>  binary_size_check: u-boot-nodtb.bin FORCE
>>  	@file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}') ; \
>>  	map_size=$(shell cat u-boot.map | \
>> -		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end =
>> $$1} END {if (start != "" && end != "") print "ibase=16; "
>> toupper(end) " - " toupper(start)}' \
>> -		| sed 's/0X//g' \
>> -		| bc); \
>> -	if [ "" != "$$map_size" ]; then \
>> +		awk ' \
>> +			/_image_copy_start/ { start = $$1 } \
>> +			/_image_binary_end/ { end = $$1 } \
>> +			END { \
>> +				if (start != "" && end != "") \
>> +					print end " " start; \
>> +			}' \
>> +		| sh -c 'read end start && echo $$((end - start))'); \
>> +	if [ -n "$$map_size" ]; then \
>>  		if test $$map_size -ne $$file_size; then \
>>  			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
>>  			echo "  but u-boot-nodtb.bin shows $$file_size" >&2 ; \