makefiles/utils/variables: add functions to help managing variables by cladmi · Pull Request #11664 · RIOT-OS/RIOT

cladmi · 2019-06-07T17:12:19Z

Contribution description

Add functions to set variables and environment for targets
These functions should help replacing immediate evaluation and global 'export'

`target-export-variables`

target-export-variables <target> <variables> function that exports a list of variables for some targets

This should help exporting variables only for the required targets so not evaluate shell calls for nothing.
Like make clean does not need CFLAGS but if the variable is exported, it will evaluate which options are supported in CFLAGS anyway.

`memoized`

VARIABLE = $(call memoized,VARIABLE,<value>) function that allows only evaluating value when the variable is used like a normal deferred variable, but further evaluations will use an immediate value.

This should help not assigning shell commands results with := to call it once only, as it result to evaluating even if not needed. Replacing by a direct = would result in the command be executed on each evaluation which is not wanted.
This gives the best of both worlds.

The implementation requires to repeat the variable name in the call memoized arguments.

Testing procedure

Review and run the test in tests/build_system_utils.

make -C tests/build_system_utils/

If it executes without error, it is working.

You can also run it with QUIET=0 to see the executed commands.

Issues/PRs references

Tracking: remove harmful use of export in make and immediate evaluation #10850

smlng

First, to me memoized sounds odd, but that's subjective and non-blocking - I would use stored or memorized instead.

Second, I get the following errors when building in docker:

Building application "tests_build_system_utils" for "native" with MCU "native".

"make" -C /data/riotbuild/riotbase/boards/native
"make" -C /data/riotbuild/riotbase/boards/native/drivers
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/cpu/native
"make" -C /data/riotbuild/riotbase/cpu/native/periph
"make" -C /data/riotbuild/riotbase/cpu/native/vfs
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/auto_init
   text	   data	    bss	    dec	    hex	filename
  22770	    568	  47652	  70990	  1154e	/data/riotbuild/riotbase/tests/build_system_utils/bin/native/tests_build_system_utils.elf
bash: line 0: test: 1560237203N: integer expression expected
ERROR: 1560237203N >= 1560237203N
make[1]: *** [test-exported-variables] Error 1
Command '/Library/Developer/CommandLineTools/usr/bin/make -C /Volumes/devel/github/smlng/RIOT/makefiles/utils -f test-variables.mk test-exported-variables' failed
bash: line 0: test: 1560237203N: integer expression expected
ERROR: 1560237203N >= 1560237203N
make[1]: *** [test-exported-variables] Error 1
make: *** [test-exported-variables] Error 1

Third: see comments.

smlng · 2019-06-11T07:02:26Z

makefiles/utils/variables.mk

+# These functions should help replacing immediate evaluation and global 'export'
+
+
+# Evaluate a deferred variable only on once on its first usage


typo: double on, remove before once

I can see that I piggybacked that change from a wip branch a bit too fast :D

smlng · 2019-06-11T07:03:35Z

makefiles/utils/variables.mk

+# variable = $(call memoized,<variable>,<value>)
+#
+# Parameters
+#  variable: name on the variable you set


typo: /on/of/

smlng · 2019-06-11T07:09:29Z

makefiles/utils/variables.mk

+
+
+# Evaluate a deferred variable only on once on its first usage
+# Uses after will be as if it was an immediate evaluation


wording: better to read: # Uses after [that] will be as ...

cladmi · 2019-06-11T11:24:23Z

Damn it does not support date +%s%N I will see what is available and does not require 1s sleep.

Memoized comes from the concept https://en.wikipedia.org/wiki/Memoization but I welcome alternative names.

aabadie · 2019-06-11T11:25:12Z

Memoized comes from the concept https://en.wikipedia.org/wiki/Memoization but I welcome alternative names.

I think this name suits perfectly, no need for another one.

cladmi · 2019-06-11T11:37:11Z

bash: line 0: test: 1560237203N: integer expression expected
ERROR: 1560237203N >= 1560237203N
make[1]: *** [test-exported-variables] Error 1
Command '/Library/Developer/CommandLineTools/usr/bin/make -C /Volumes/devel/github/smlng/RIOT/makefiles/utils -f test-variables.mk test-exported-variables' failed
bash: line 0: test: 1560237203N: integer expression expected

The error is not in docker but in osx.
I could replace it with date; sleep 1 in osx but will see if I find something better.

cladmi · 2019-06-11T11:52:34Z

@smlng I updated to use python to give the timestamp in nanoseconds and it worked on OSx 17.7.0. Does it work for you too ?

smlng · 2019-06-11T12:04:06Z

yep, python works on macOS ... alternative would be to use gdate on macOS (and other non-Linux systems), but that's not available by default.

cladmi · 2019-06-11T12:23:44Z

yep, python works on macOS ... alternative would be to use gdate on macOS (and other non-Linux systems), but that's not available by default.

Here it is just a test, not the all applications workflow, so I did not look for performance optimization. And it gives a solution without asking for uname and if gdate is there or not.

jcarrano

Good addition.

cladmi · 2019-06-27T14:05:13Z

@smlng do you agree with the updated changes ?

smlng · 2019-06-27T14:14:11Z

yep, pleas squash

smlng · 2019-06-27T14:14:21Z

please 😄

This allows exporting variables only for some target. It will allow not exporting variables when not needed, and so prevent unnecessary evaluation.

This allow deferring a variable evaluation to its usage but still benefit from only evaluating it once on multiple uses.

Import utils functions in Makefile.include to allow using them.

cladmi · 2019-06-28T09:36:04Z

Squashed.

cladmi · 2019-07-01T11:49:08Z

Thank you for the review. We need to start using it to remove more exports.

cladmi added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system Area: tools Area: Supplementary tools labels Jun 7, 2019

cladmi added this to the Release 2019.07 milestone Jun 7, 2019

cladmi requested review from aabadie, cgundogan, jcarrano and smlng June 7, 2019 17:12

smlng suggested changes Jun 11, 2019

View reviewed changes

jcarrano approved these changes Jun 13, 2019

View reviewed changes

smlng approved these changes Jun 27, 2019

View reviewed changes

cladmi added 3 commits June 28, 2019 11:33

makefiles/utils: function to export variables for a target

fcf8c47

This allows exporting variables only for some target. It will allow not exporting variables when not needed, and so prevent unnecessary evaluation.

makefiles/utils: function to memoize a variable evaluation

2c5eeca

This allow deferring a variable evaluation to its usage but still benefit from only evaluating it once on multiple uses.

makefiles/utils: include in Makefile.include

25a1bc4

Import utils functions in Makefile.include to allow using them.

cladmi force-pushed the pr/make/variables_lazy_evaluation branch from 3cbafa3 to 25a1bc4 Compare June 28, 2019 09:35

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 28, 2019

jcarrano merged commit 50ea0d8 into RIOT-OS:master Jun 28, 2019

cladmi mentioned this pull request Jul 1, 2019

Tracking: remove harmful use of export in make and immediate evaluation #10850

Closed

19 tasks

cladmi deleted the pr/make/variables_lazy_evaluation branch July 1, 2019 12:15

cladmi mentioned this pull request Jul 1, 2019

Makefile.include: remove RIOT_VERSION_OVERRIDE #11771

Merged

		# These functions should help replacing immediate evaluation and global 'export'


		# Evaluate a deferred variable only on once on its first usage



		# Evaluate a deferred variable only on once on its first usage
		# Uses after will be as if it was an immediate evaluation

Conversation

cladmi commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

target-export-variables

memoized

Testing procedure

Issues/PRs references

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

aabadie commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

smlng commented Jun 11, 2019

Uh oh!

cladmi commented Jun 11, 2019

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 27, 2019

Uh oh!

smlng commented Jun 27, 2019

Uh oh!

smlng commented Jun 27, 2019

Uh oh!

cladmi commented Jun 28, 2019

Uh oh!

cladmi commented Jul 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi commented Jun 7, 2019 •

edited

Loading

`target-export-variables`

`memoized`