makefiles/utils/{strings, paths}: Add function library. by jcarrano · Pull Request #11218 · RIOT-OS/RIOT

jcarrano · 2019-03-20T13:09:26Z

Contribution description

Add a set of functions written in pure makefile. The only function that is really useful now is relpath, to calculate relative paths in systems without coreutils/realpath (e.g. Mac).

Tests were added to tests/build_system_utils. The Makefile for the test was also reworked to avoid code repetition.

Testing procedure

Run make -C tests/build_system_utils. You should see several green lines showing tests that passed.

Issues/PRs references

~~Needed for #10195 .~~

cladmi · 2019-05-08T14:07:15Z

makefiles/utils/checks.mk

+#					default message is shown.
+# Returns:
+#	empty string
+assert_eq = $(if $(call streq,$(1),$(2)),,$(error $(if $(3),$(3),Expected "$(2)" and got "$(1)")))


This one does not work as documented.

These two cases both work when they should not:

test-assert-eq: ... # Test put here as `test-assert-eq_negative` does not test correctly, and the `negative` tests cannot really have multiple commands on different lines. @test -z "$(call assert_eq,a,a b,"This should fail")" @test -z "$(call assert_eq,lol%,lola,"This should fail")"

Problem is that streq is not streq. It would maybe just be better with a define so multiline using ifneq.

cladmi · 2019-05-08T14:15:44Z

makefiles/utils/strings.mk

+
+# Return an empty string if the arguments are different (works only for single
+# words
+streq = $(filter $(1),$(2))


Even with only single words, it does not work. Using findstring would be better to not match patterns.
Here it should be checked that indeed words $(1) == 1 and words $(2) == 1.

cladmi · 2019-05-08T14:17:39Z

makefiles/utils/strings.mk

+# as true.
+
+# Return a string ("x") if the argument is empty and vice versa
+strnot = $(if $(1),,x)


The if is somehow clear enough and strnot is unused.

cladmi · 2019-05-08T14:27:17Z

makefiles/utils/strings.mk

+concat ?= $(if $(1),$(firstword $(1))$(call concat,$(call rest,$(1))),)
+
+# Intercalate: concatenate words in a list with a separator between each one.
+intercal = $(firstword $(2))$(call concat,$(addprefix $(1),$(call rest,$(2))))


By taking the list as first argument and the separator as second, concat could be the same "public" function I think.

Concatenate a list of strings: $1: list of strings $2: join str, default to empty

Maybe for the implementation you still need the other one but only one public would be enough.

In that case it could use the python join or just concat in general.

cladmi · 2019-05-08T14:46:25Z

tests/build_system_utils/Makefile

+
 define command_should_fail
-$1 2>/dev/null && { echo "Command '$1' should have failed but did not" >&2; $1; exit 1; } || true
+$1 2>/dev/null && { $(call fail_message,Command '$1' should have failed but did not) >&2; $1; exit 1; } || $(call ok_message,$2)


This does not work, none of the negative functions detect when something does not fail.

With this the test still pass:

diff --git a/makefiles/utils/test-checks.mk b/makefiles/utils/test-checks.mk index 3ee960cfd..f99a3d12e 100644 --- a/makefiles/utils/test-checks.mk +++ b/makefiles/utils/test-checks.mk @@ -4,7 +4,7 @@ test-ensure_value: @test "$@" = "$(call ensure_value,$@,"This should not fail")" test-ensure_value-negative: - @echo $(call ensure_value,$^,"This should fail") + true test-assert: @test -z "$(call assert,$@,"This should not fail")"

cladmi

It is quite big for a single commit.

I would rather split the assert thing in its own PR.

Also the tests/build_system_utils refactoring should go alone, right now the negative tests do not correctly test when test-ensure_value-negative was testing before.

I did not look at the paths part yet.

cladmi · 2019-05-08T14:54:20Z

Oops, my bad, I used "add comment" instead of "add to review" and even edited the old ones as thought it was in a review… 😞

jcarrano · 2019-05-09T12:25:44Z

@cladmi I stated to fix some of the stuff, but each time I approach this I cannot help feeling I'm reinventing the wheel, or in this case, recursive data types, and if I were to keep on going I'd probably end up defining folds, and zips, and maps, and a whole language on top of Make.

On the other hand, since we don't want a configure step (and we have our good reasons) we need to have quite a bit of functionality in make.

Now, looking around I see that Make with Guile (i.e. scheme) support is available in ubuntu. Writing all this complex logic in scheme is much easier. Even for someone who does not know programming (read SICP).

There's still the issue of Mac users (Windows users can run Ubuntu via WLS). AFAIK the newer versions of Make are available on mac (via homebrew).

In conclusion:

We should implement these functions using scheme.
Switch the ubuntu image from make to make-guile.
Mac users: if you want the build to run natively in your platform, please make a small effort and setup your system with up-to-date tools.

cladmi · 2019-05-13T18:02:18Z

Just keep the functions we currently need that are hard to do directly in make.

Some of the utils are not explicitely needed strnot and streq are just replacing what is normally done in make with a different name.

If the new needed one is currently only relpath for thin-archives we can just first use the binary/a python version and update if there is really a performance issue.

Add a set of functions written in pure makefile. The only function that is really useful now is relpath, to calculate relative paths in systems without coreutils/realpath (e.g. Mac). Tests were added to tests/build_system_utils. The Makefile for the test was also reworked to avoid code repetition.

Use the automatic rules to run the test-exported-variables, test-memoized-variables targets.

@cladmi

Add functions to convert to lowercase/uppercase and another one that also replaces dashes for underscores. This is a remake of a PR by @cladmi (pr RIOT-OS#12119) The Make version is about 20 times faster than using the shell. The following patch can be used for testing (run make --no-print-directory -C examples/hello-world/ clean) diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile index 258d8e9..1f2adf837 100644 --- a/examples/hello-world/Makefile +++ b/examples/hello-world/Makefile @@ -16,3 +16,5 @@ DEVELHELP ?= 1 QUIET ?= 1 include $(RIOTBASE)/Makefile.include + +include $(RIOTMAKE)/utils/strings.mk diff --git a/makefiles/utils/strings.mk b/makefiles/utils/strings.mk index c571cd6..7dce7d80d 100644 --- a/makefiles/utils/strings.mk +++ b/makefiles/utils/strings.mk @@ -39,3 +39,28 @@ lowercase = $(call xlate,$(_UPPER_LETTERS),$(_LOWER_LETTERS),$(1)) uppercase = $(call xlate,$(_LOWER_LETTERS),$(_UPPER_LETTERS),$(1)) uppercase_and_underscore = $(call uppercase,$(subst -,_,$1)) + + +#~ # Local testing +#~ var = abcdefghijklmnopqrstuvwxyz-123456789 +#~ VAR = ABCDEFGHIJKLMNOPQRSTUVWXYZ_123456789 + +#~ ifneq ($(call uppercase_and_underscore,$(var)),$(VAR)) + #~ $(error $(call uppercase_and_underscore,$(var))) +#~ endif + +#~ NUM ?= 10000 +#~ iters := $(shell seq $(NUM)) + +#~ d_0 := $(shell date +%s.%N) +#~ a := $(foreach i,$(iters),$(call uppercase_and_underscore,$(var))) +#~ d_end := $(shell date +%s.%N) +#~ d_diff := $(shell echo $(d_end) - $(d_0) | bc) +#~ $(warning makefile $(d_diff): $(d_0) - $(d_end)) + +#~ d_0 := $(shell date +%s.%N) +#~ $(foreach i,$(iters),$(shell echo $(var)| tr 'a-z-' 'A-Z_'>/dev/null)) +#~ d_end := $(shell date +%s.%N) + +#~ d_diff := $(shell echo $(d_end) - $(d_0) | bc) +#~ $(warning shell $(d_diff): $(d_0) - $(d_end))

The rest function gets the "tail" of a list and is often used to define recursive list processing functions. It uses make's wordlist function which requires an upper bound, which was being computed using the "words" function. Replacing this by a very high number (2**31 - 1) makes it much faster and will never be an issue (the build machine will probably run out of memory before). Note that GNU Make does not like values larger than 2**31.

jcarrano · 2019-09-12T15:08:44Z

@cladmi regardless of the fate of the string library, do you think the changes to the build_system_utils Makefile make sense? I like that with this patch it shows which tests are being run.

cladmi · 2019-09-12T15:40:13Z

The negative test does not detect errors as said in #11218 (comment)

make -C tests/build_system_utils/
make: Entering directory '/home/harter/work/git/RIOT/tests/build_system_utils'
Building application "tests_build_system_utils" for "native" with MCU "native".

"make" -C /home/harter/work/git/RIOT/boards/native
"make" -C /home/harter/work/git/RIOT/boards/native/drivers
"make" -C /home/harter/work/git/RIOT/core
"make" -C /home/harter/work/git/RIOT/cpu/native
"make" -C /home/harter/work/git/RIOT/cpu/native/periph
"make" -C /home/harter/work/git/RIOT/cpu/native/vfs
"make" -C /home/harter/work/git/RIOT/drivers
"make" -C /home/harter/work/git/RIOT/drivers/periph_common
"make" -C /home/harter/work/git/RIOT/sys
"make" -C /home/harter/work/git/RIOT/sys/auto_init
   text	   data	    bss	    dec	    hex	filename
  22798	    568	  47652	  71018	  1156a	/home/harter/work/git/RIOT/tests/build_system_utils/bin/native/tests_build_system_utils.elf
 [OK] ensure_value 
 [OK] assert 
 [OK] assert_not 
 [OK] ensure_value (negative) 
 [OK] assert (negative) 
 [OK] assert_not (negative) 
 [OK] exported-variables 
 [OK] memoized-variables 
 [OK] streq 
 [OK] assert_eq 
 [OK] assert_eq (negative) 
 [OK] strnot 
 [OK] rest 
 [OK] concat 
 [OK] intercal 
 [OK] lowercase 
 [OK] uppercase 
 [OK] uppercase_and_underscore 
 [OK] relpath 
 [OK] relpath-against-sys

Having a better visual output is indeed good, if it was working.
I do not care about the color.

stale · 2020-03-15T16:15:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jcarrano added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 20, 2019

jcarrano requested a review from cladmi March 20, 2019 13:09

jcarrano mentioned this pull request Mar 20, 2019

Makefile.base: use thin static archives. #10195

Merged

cladmi reviewed May 8, 2019

View reviewed changes

cladmi suggested changes May 8, 2019

View reviewed changes

jcarrano force-pushed the make-path-utilities branch from 6aa6917 to e955205 Compare August 28, 2019 16:07

jcarrano and others added 4 commits August 28, 2019 18:09

tests/build_system_utils: Adapt variable tests to new system.

e955205

Use the automatic rules to run the test-exported-variables, test-memoized-variables targets.

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 15, 2020

stale bot closed this Apr 15, 2020

Conversation

jcarrano commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi May 8, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi May 8, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi May 8, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented May 8, 2019

Uh oh!

jcarrano commented May 9, 2019

Uh oh!

cladmi commented May 13, 2019

Uh oh!

jcarrano commented Sep 12, 2019

Uh oh!

cladmi commented Sep 12, 2019

Uh oh!

stale bot commented Mar 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcarrano commented Mar 20, 2019 •

edited

Loading

cladmi May 8, 2019 •

edited

Loading

cladmi May 8, 2019 •

edited

Loading

cladmi May 8, 2019 •

edited

Loading