diff mbox

[LEDE-DEV] Use printf instead of echo.

Message ID 1471939333-956-1-git-send-email-ash.benz@bk.ru
State Changes Requested
Headers show

Commit Message

Ash Benz Aug. 23, 2016, 8:02 a.m. UTC
On some systems the shell built-in echo is used while on others /bin/echo and those two are not
 identical; this causes incomplete .cflags generation on some systems.

Signed-off-by: Ash Benz <ash.benz@bk.ru>
---
 package/network/services/hostapd/patches/200-multicall.patch | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Felix Fietkau Aug. 23, 2016, 9:21 a.m. UTC | #1
On 2016-08-23 10:02, Ash Benz wrote:
>  On some systems the shell built-in echo is used while on others /bin/echo and those two are not
>  identical; this causes incomplete .cflags generation on some systems.
> 
> Signed-off-by: Ash Benz <ash.benz@bk.ru>
> ---
>  package/network/services/hostapd/patches/200-multicall.patch | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/package/network/services/hostapd/patches/200-multicall.patch b/package/network/services/hostapd/patches/200-multicall.patch
> index 8b260c2..ddfec6e 100644
> --- a/package/network/services/hostapd/patches/200-multicall.patch
> +++ b/package/network/services/hostapd/patches/200-multicall.patch
> @@ -54,10 +54,10 @@
>   endif
>   
>  +dump_cflags:
> -+	@echo -n $(CFLAGS) " "
> ++	@printf "%s " $(CFLAGS)
>  +
>  +dump_ldflags:
> -+	@echo -n $(LDFLAGS) $(LIBS) $(EXTRALIBS) " "
> ++	@printf "%s " $(LDFLAGS) $(LIBS) $(EXTRALIBS)
>  +
>   nt_password_hash: $(NOBJS)
>   	$(Q)$(CC) $(LDFLAGS) -o nt_password_hash $(NOBJS) $(LIBS_n)
> @@ -142,10 +142,10 @@
>   	@$(E) "  sed" $<
>   
>  +dump_cflags:
> -+	@echo -n $(CFLAGS) " "
> ++	@printf "%s " $(CFLAGS)
>  +
>  +dump_ldflags:
> -+	@echo -n $(LDFLAGS) $(LIBS) $(EXTRALIBS) " "
> ++	@printf "%s " $(LDFLAGS) $(LIBS) $(EXTRALIBS)
While your version appears to work, I think it would be more correct to
put everything in one argument for %s.
Please use something like:
	@printf "%s " "$(LDFLAGS) $(LIBS) $(EXTRALIBS)"

- Felix
Rafał Miłecki Aug. 23, 2016, 9:46 a.m. UTC | #2
On 23 August 2016 at 10:02, Ash Benz <ash.benz@bk.ru> wrote:
>  On some systems the shell built-in echo is used while on others /bin/echo and those two are not
>  identical; this causes incomplete .cflags generation on some systems.
>
> Signed-off-by: Ash Benz <ash.benz@bk.ru>

Please use a proper prefix for your subject. For a good hint see
git log --oneline package/network/services/hostapd/

Also no need to prepend every line of your commit message with a
space. Just write "On some" instead of " On some".
Ash Benz Aug. 23, 2016, 10:10 a.m. UTC | #3
Hi Bastian,

I will update them, however to do that while adhering to Felix's request 
on using perfect syntax means going over them one by one.

Perhaps a seasoned sed user can do a trick to automate it?

For now, the change I submitted is whats needed to fix breakage on my 
system. For others, I will send patches if no one takes it up in a few days.

Regards,
A. Benz

On 08/23/16 17:03, Bastian Bittorf wrote:
> * Ash Benz <ash.benz@bk.ru> [23.08.2016 10:19]:
>>  On some systems the shell built-in echo is used while on others /bin/echo and those two are not
>>  identical; this causes incomplete .cflags generation on some systems.
>
> thanks for that, it was on my list.
> can you also eleminate some other users, see:
>
> # git grep 'echo -n' | wc -l
> # 87
>
> thanks & bye, bastian
>
Felix Fietkau Aug. 23, 2016, 10:13 a.m. UTC | #4
On 2016-08-23 12:10, A. Benz wrote:
> Hi Bastian,
> 
> I will update them, however to do that while adhering to Felix's request 
> on using perfect syntax means going over them one by one.
> 
> Perhaps a seasoned sed user can do a trick to automate it?
> 
> For now, the change I submitted is whats needed to fix breakage on my 
> system. For others, I will send patches if no one takes it up in a few days.
Actually, you might be able to simplify the change even more by forcing
the hostapd build shell to bash (some other packages do this as well).

- Felix
Bastian Bittorf Aug. 23, 2016, 12:04 p.m. UTC | #5
* Felix Fietkau <nbd@nbd.name> [23.08.2016 13:59]:
> > For now, the change I submitted is whats needed to fix breakage on my 
> > system. For others, I will send patches if no one takes it up in a few days.
> Actually, you might be able to simplify the change even more by forcing
> the hostapd build shell to bash (some other packages do this as well).

this look simple, but in the long run...

# "Stick to portable constructs where possible, and
#  you will make somebody's life easier in the future.
#  Maybe your own."

so using /bin/sh with 'printf' is safe.
(no matter which underlying shell-interpreter we use)

bye, bastian
Karl Palsson Aug. 23, 2016, 1:44 p.m. UTC | #6
Felix Fietkau <nbd@nbd.name> wrote:
> On 2016-08-23 12:10, A. Benz wrote:
> > Hi Bastian,
> > 
> > I will update them, however to do that while adhering to Felix's request 
> > on using perfect syntax means going over them one by one.
> > 
> > Perhaps a seasoned sed user can do a trick to automate it?
> > 
> > For now, the change I submitted is whats needed to fix breakage on my 
> > system. For others, I will send patches if no one takes it up in a few days.
> Actually, you might be able to simplify the change even more by
> forcing the hostapd build shell to bash (some other packages do
> this as well).

really?  Surely printf is better than requiring bash?
diff mbox

Patch

diff --git a/package/network/services/hostapd/patches/200-multicall.patch b/package/network/services/hostapd/patches/200-multicall.patch
index 8b260c2..ddfec6e 100644
--- a/package/network/services/hostapd/patches/200-multicall.patch
+++ b/package/network/services/hostapd/patches/200-multicall.patch
@@ -54,10 +54,10 @@ 
  endif
  
 +dump_cflags:
-+	@echo -n $(CFLAGS) " "
++	@printf "%s " $(CFLAGS)
 +
 +dump_ldflags:
-+	@echo -n $(LDFLAGS) $(LIBS) $(EXTRALIBS) " "
++	@printf "%s " $(LDFLAGS) $(LIBS) $(EXTRALIBS)
 +
  nt_password_hash: $(NOBJS)
  	$(Q)$(CC) $(LDFLAGS) -o nt_password_hash $(NOBJS) $(LIBS_n)
@@ -142,10 +142,10 @@ 
  	@$(E) "  sed" $<
  
 +dump_cflags:
-+	@echo -n $(CFLAGS) " "
++	@printf "%s " $(CFLAGS)
 +
 +dump_ldflags:
-+	@echo -n $(LDFLAGS) $(LIBS) $(EXTRALIBS) " "
++	@printf "%s " $(LDFLAGS) $(LIBS) $(EXTRALIBS)
 +
  wpa_supplicant.exe: wpa_supplicant
  	mv -f $< $@