Skip to content

makefile/utils: Add a function for checking that a string is not empty.#10439

Merged
cladmi merged 3 commits intoRIOT-OS:masterfrom
jcarrano:ensure-value
Dec 7, 2018
Merged

makefile/utils: Add a function for checking that a string is not empty.#10439
cladmi merged 3 commits intoRIOT-OS:masterfrom
jcarrano:ensure-value

Conversation

@jcarrano
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano commented Nov 20, 2018

Contribution description

A call to $(call ensure_value,x,y) will fail with message y if x is empty, and otherwise return x. This can be useto write more compact makefiles, while still producing friendly error messages.

Testing procedure

I should provide an automated test for this, in the meantime:

$ cd makefiles/util
$ make -f test-checks.mk test-ensure_value
test-ensure_value
$ make -f test-checks.mk test-ensure_value-negative
test-checks.mk:7: *** "This should fail".  Stop.

Issues/PRs references

Split from #10342.

@jcarrano jcarrano added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 20, 2018
@jcarrano jcarrano mentioned this pull request Nov 20, 2018
Closed
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 5, 2018

I will add a test so that it is run in murdock.

@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 5, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 5, 2018

I added a test and updated the behavior of your test to verify the returned value of ensure_value.
The test prints nothing in case of success.

It can be tested by replacing, in the Makefile, the tested target by the other: {,_negative}.
Verifying that the check tests ensure_value returned value can be done by replacing it in checks.mk.

On the naming, I would prefer having the directory named utils instead of using the singular form.
I still kept my script with your convention.

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Dec 6, 2018

I renamed it to utils and squashed.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 7, 2018

Nice catch on the typo in the error message.

Codacy wants one line less in the README… you can inline fix it directly.
The commit titles mention makefile/util and makefiles/util, please update them to makefiles/utils at the same time.

I re-ran the tests and verified that they also detected the errors by tweaking the test

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Dec 7, 2018

Done. Stupid codacy.

jcarrano and others added 3 commits December 7, 2018 18:19
@jcarrano
makefiles/utils: Add a function for checking that a string is not empty.
547d9f0 
A call to `$(ensure_value x,y)` will fail with message y if x is empty, and
otherwise return x. This can be useto write more compact makefiles, while still
producing friendly error messages.
@jcarrano
makefiles/utils: add tests for checks.mk.
f466171 
make -f test-checks.mk test-ensure_value should succeed, while
make -f test-checks.mk test-ensure_value-negative should fail.
@cladmi @jcarrano
tests/build_system_utils: add test for build system utilities
9d252a8 
This will allow testing the build system 'utils' functions.
@cladmi cladmi changed the title makefile/util: Add a function for checking that a string is not empty. makefile/utils: Add a function for checking that a string is not empty. Dec 7, 2018
cladmi
cladmi approved these changes Dec 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with the implementation and tested too.

@cladmi cladmi merged commit 5a2609c into RIOT-OS:master Dec 7, 2018
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cladmi cladmi cladmi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

3 participants

@jcarrano @cladmi @aabadie