[PATCH v2] kbuild: use printf instead of echo -e extension

[PATCH v2] kbuild: use printf instead of echo -e extension

[Ahmad Fatoum]

We are using echo -e, so the \n in the string being echo'd are
interpreted. As -e is not POSIX and dash doesn't provide it,
we use a strange /usr/bin/env echo -e construct hoping that
whatever non-builtin echo is first in the search path supports -e.

As the new lines are just used to separate CPP directives, we can
just pass the directives as $(CPP) flags. This has the same result,
but is potable and avoids NixOS complaining when building barebox.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - drop echo -e eltogether after Sascha pointed out why it needs
    /usr/bin/env.

Tested with two fragments:
  - dts variable is defined for the DT it references
  - fragments are included (checked with #warning)
  - fragment change triggers rebuild
  - original DT change triggers rebuild
  - fragmentless case works ok
    
 scripts/Makefile.lib | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 4deaa5dfa73c..befd47ed6600 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -372,8 +372,8 @@ $(obj)/%.dtb.z: $(obj)/%.dtb FORCE
 dts-frags = $(subst $(quote),,$(CONFIG_EXTERNAL_DTS_FRAGMENTS))
 quiet_cmd_dtc = DTC     $@
 # For compatibility between make 4.2 and 4.3
-cmd_dtc = /usr/bin/env echo -e '$(pound)define $(subst -,_,$(*F))_dts 1\n'$(foreach f,$< $(2),'$(pound)include "$(f)"\n') | \
-	$(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) - ; \
+cmd_dtc = echo | $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) \
+	-D'$(subst -,_,$(*F))_dts=1' $(foreach f,$< $(2),-include '$(f)') - ; \
 	$(objtree)/scripts/dtc/dtc -O dtb -o $@ -b 0 \
 		-i $(srctree)/arch/$(SRCARCH)/dts $(DTC_FLAGS) \
 		-i $(srctree)/dts/src/$(SRCARCH) \
-- 
2.39.2

Re: [PATCH v2] kbuild: use printf instead of echo -e extension

[Sascha Hauer]

Hi Ahmad,

The subject seems outdated, you are not using printf at all here.

On Thu, Apr 13, 2023 at 10:45:47AM +0200, Ahmad Fatoum wrote:
> We are using echo -e, so the \n in the string being echo'd are
> interpreted. As -e is not POSIX and dash doesn't provide it,
> we use a strange /usr/bin/env echo -e construct hoping that
> whatever non-builtin echo is first in the search path supports -e.
> 
> As the new lines are just used to separate CPP directives, we can
> just pass the directives as $(CPP) flags. This has the same result,
> but is potable and avoids NixOS complaining when building barebox.

s/potable/portable/

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>   - drop echo -e eltogether after Sascha pointed out why it needs
>     /usr/bin/env.
> 
> Tested with two fragments:
>   - dts variable is defined for the DT it references
>   - fragments are included (checked with #warning)
>   - fragment change triggers rebuild
>   - original DT change triggers rebuild
>   - fragmentless case works ok
>     
>  scripts/Makefile.lib | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 4deaa5dfa73c..befd47ed6600 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -372,8 +372,8 @@ $(obj)/%.dtb.z: $(obj)/%.dtb FORCE
>  dts-frags = $(subst $(quote),,$(CONFIG_EXTERNAL_DTS_FRAGMENTS))
>  quiet_cmd_dtc = DTC     $@
>  # For compatibility between make 4.2 and 4.3
> -cmd_dtc = /usr/bin/env echo -e '$(pound)define $(subst -,_,$(*F))_dts 1\n'$(foreach f,$< $(2),'$(pound)include "$(f)"\n') | \
> -	$(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) - ; \
> +cmd_dtc = echo | $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) \
> +	-D'$(subst -,_,$(*F))_dts=1' $(foreach f,$< $(2),-include '$(f)') - ; \

Good idea to get rid of the echo -e dependency entirely, I like it :)

You could replace the '-' for stdin with /dev/null and then drop the
echo |. Maybe this makes it a bit easier to read.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] kbuild: use printf instead of echo -e extension

[Ahmad Fatoum]

On 13.04.23 11:20, Sascha Hauer wrote:
> Hi Ahmad,
> 
> The subject seems outdated, you are not using printf at all here.

Oh right. I tried printf first, but foreach concatenates with
spaces in-between and printf ignores everything not referenced
by the format string...

> On Thu, Apr 13, 2023 at 10:45:47AM +0200, Ahmad Fatoum wrote:
>> We are using echo -e, so the \n in the string being echo'd are
>> interpreted. As -e is not POSIX and dash doesn't provide it,
>> we use a strange /usr/bin/env echo -e construct hoping that
>> whatever non-builtin echo is first in the search path supports -e.
>>
>> As the new lines are just used to separate CPP directives, we can
>> just pass the directives as $(CPP) flags. This has the same result,
>> but is potable and avoids NixOS complaining when building barebox.
> 
> s/potable/portable/

Will fix.

> Good idea to get rid of the echo -e dependency entirely, I like it :)
> 
> You could replace the '-' for stdin with /dev/null and then drop the
> echo |. Maybe this makes it a bit easier to read.

I had thought about that, but deemed

  In file included from <command-line>:3:
  /home/afa/fragment1.dts:6:2: warning: #warning hey [-Wcpp]

to be less confusing than it mentioning /dev/null. Now that I actually
tried it out though, the warning also references command-line, so
you will change for v3.

Thanks,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |