make: replace curly braces with parenthesis by aabadie · Pull Request #8822 · RIOT-OS/RIOT

aabadie · 2018-03-22T15:29:30Z

Contribution description

This PR is a follow-up of #8815 but applied to all Makefiles. Then the use of $(variable) is consistent across all makefiles (instead of mixing it with ${variable}).

Maybe @cladmi is interested ?

Issues/PRs references

jnohlgard · 2018-03-22T15:31:54Z

Did you use a script to perform the replaces?

aabadie · 2018-03-22T15:33:51Z

Did you use a script to perform the replaces?

No, just query replaces by hand.

jnohlgard

There are many places where the text in a makefile is passed to the shell, there curly braces can't be replaced or it will break

jnohlgard · 2018-03-22T15:32:26Z

makefiles/buildtests.inc.mk

-		${COLOR_ECHO} -n "Building for $$board ... " ; \
-		BOARD=$${board} RIOT_CI_BUILD=1 RIOT_VERSION_OVERRIDE=buildtest \
+		$(COLOR_ECHO) -n "Building for $$board ... " ; \
+		BOARD=$$(board) RIOT_CI_BUILD=1 RIOT_VERSION_OVERRIDE=buildtest \


Here, the curly braces matter

jnohlgard · 2018-03-22T15:36:47Z

makefiles/buildtests.inc.mk

 		$(MAKE) clean-intermediates >/dev/null 2>&1 || true; \
 	done ; \
-	$${RESULT}
+	$$(RESULT)


curly braces matter

jnohlgard · 2018-03-22T15:37:32Z

makefiles/info-global.inc.mk

-	for board in ${BOARDS}; do \
-	    echo "$$(BOARD=$${board} $(MAKE) --no-print-directory info-buildsize 2>/dev/null | tail -n-1 | cut -f-4)" "$${board}"; \
+	for board in $(BOARDS); do \
+	    echo "$$(BOARD=$$(board) $(MAKE) --no-print-directory info-buildsize 2>/dev/null | tail -n-1 | cut -f-4)" "$$(board)"; \


curly braces

jnohlgard · 2018-03-22T15:37:46Z

makefiles/info-global.inc.mk

-	  for BINDIRBASE in $${OLDBIN} $${NEWBIN}; do \
-	      BINDIRBASE=$${BINDIRBASE} BOARD=$${board} $(MAKE) info-buildsize --no-print-directory 2>/dev/null | tail -n-1 | cut -f-4; \
+	for board in $(BOARDS); do \
+	  for BINDIRBASE in $$(OLDBIN) $$(NEWBIN); do \


must be curly

jnohlgard · 2018-03-22T15:37:56Z

makefiles/info-global.inc.mk

-	      BINDIRBASE=$${BINDIRBASE} BOARD=$${board} $(MAKE) info-buildsize --no-print-directory 2>/dev/null | tail -n-1 | cut -f-4; \
+	for board in $(BOARDS); do \
+	  for BINDIRBASE in $$(OLDBIN) $$(NEWBIN); do \
+	      BINDIRBASE=$$(BINDIRBASE) BOARD=$$(board) $(MAKE) info-buildsize --no-print-directory 2>/dev/null | tail -n-1 | cut -f-4; \


more curlys

jnohlgard · 2018-03-22T15:38:26Z

makefiles/info-global.inc.mk

-	    for I in 0 1 2 3; do echo -ne "$${NEW[I]-${COLOR_RED}ERR${COLOR_RESET}}\t"; done; echo -e "$${NEWBIN}\n"; \
+	    echo "$$(board)"; \
+	    for I in 0 1 2 3; do echo -ne "$$(OLD[I]-$(COLOR_RED)ERR$(COLOR_RESET))\t"; done; echo -e "$$(OLDBIN)"; \
+	    for I in 0 1 2 3; do echo -ne "$$(NEW[I]-$(COLOR_RED)ERR$(COLOR_RESET))\t"; done; echo -e "$$(NEWBIN)\n"; \


curly all above

jnohlgard · 2018-03-22T15:38:32Z

makefiles/info-global.inc.mk


 info-boards-features-missing:
-	@for f in $(BOARDS_FEATURES_MISSING); do echo $${f}; done | column -t
+	@for f in $(BOARDS_FEATURES_MISSING); do echo $$(f); done | column -t


jnohlgard · 2018-03-22T15:39:12Z

makefiles/info.inc.mk

 	  tail -n+2 | \
 	  sed -e 's#$(BINDIR)##' | \
-	  sort -rnk$${SORTROW}
+	  sort -rnk$$(SORTROW)


jnohlgard · 2018-03-22T15:41:17Z

I did not mark all mistakes, but look for double dollar signs in the diff and verify those one more time

aabadie · 2018-03-22T15:50:46Z

oups, you are right, I'll fix them.

aabadie · 2018-03-22T16:14:05Z

@gebart, I reverted the wrong changes. I think it should be ok now.

aabadie · 2018-03-26T14:55:22Z

@gebart do you see other problems here ?

jnohlgard

Looks fine now

cladmi · 2018-03-27T13:21:53Z

I will check this afternoon.

cladmi

I did not find other occurrence of ${ in makefiles. I used this command to check:

git grep '${' | grep -v '$${' | grep -v -e '\.sh:' -e '^dist/' -e .murdock -e mkbootc -e README.md -e 'generate-cmake-xcompile.perl' -e 'doc/doxygen'

The changes look ok by looking at the diff.

Follow up to RIOT-OS#8822 cleanup.

jnohlgard removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 22, 2018

jnohlgard requested changes Mar 22, 2018

View reviewed changes

aabadie force-pushed the pr/make_curly branch from 1f169f8 to 0cc4818 Compare March 22, 2018 16:06

make: replace curly braces with parenthesis

61f1b94

aabadie force-pushed the pr/make_curly branch from 74bd7de to 61f1b94 Compare March 22, 2018 19:43

jnohlgard approved these changes Mar 26, 2018

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 26, 2018

cladmi self-assigned this Mar 27, 2018

cladmi mentioned this pull request Mar 27, 2018

Makefiles: add support to generate both .hex and .bin file and add FLASHFILE variable #8838

Merged

49 tasks

cladmi approved these changes Mar 27, 2018

View reviewed changes

cladmi merged commit 21a9958 into RIOT-OS:master Mar 27, 2018

cladmi mentioned this pull request Apr 10, 2018

make: add and export RIOTTOOLS directory #8821

Merged

9 tasks

cladmi added a commit to cladmi/RIOT that referenced this pull request Apr 10, 2018

pkg/ccn-lite: replace curly braces with parenthesis

14d6cd5

Follow up to RIOT-OS#8822 cleanup.

cladmi mentioned this pull request Apr 10, 2018

pkg/ccn-lite: replace curly braces with parenthesis #8907

Merged

jcarrano pushed a commit to jcarrano/RIOT that referenced this pull request May 7, 2018

pkg/ccn-lite: replace curly braces with parenthesis

273a61f

Follow up to RIOT-OS#8822 cleanup.

jcarrano pushed a commit to jcarrano/RIOT that referenced this pull request May 9, 2018

pkg/ccn-lite: replace curly braces with parenthesis

2b5f15e

Follow up to RIOT-OS#8822 cleanup.

aabadie deleted the pr/make_curly branch June 8, 2018 20:40

jia200x pushed a commit to jia200x/RIOT that referenced this pull request Jun 11, 2018

pkg/ccn-lite: replace curly braces with parenthesis

eddcba2

Follow up to RIOT-OS#8822 cleanup.

panail pushed a commit to panail/RIOT that referenced this pull request Oct 29, 2018

pkg/ccn-lite: replace curly braces with parenthesis

1fa9250

Follow up to RIOT-OS#8822 cleanup.

Conversation

aabadie commented Mar 22, 2018

Contribution description

Issues/PRs references

Uh oh!

jnohlgard commented Mar 22, 2018

Uh oh!

aabadie commented Mar 22, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Mar 22, 2018

Uh oh!

aabadie commented Mar 22, 2018

Uh oh!

aabadie commented Mar 22, 2018

Uh oh!

aabadie commented Mar 26, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 27, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants