Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dist/tools/buildsystem_sanity_check: check no PORT exports #12209

Merged

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Sep 12, 2019

Contribution description

Add sanity check for removed exports.

PORT is not exported anymore since #10440 so prevent for re-appearing.

Testing procedure

./dist/tools/buildsystem_sanity_check/check.sh

Returns no errors. (It is executed by the static tests too so would detect issues).

If adding this diff:

diff --git a/Makefile b/Makefile
index 144cbd7e6..fe51b1bda 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,9 @@
 .all:
+# Nothing after: export PORT
+export PORT = space
+export PORT?= question_mark
+export PORT= equal
+export PORT:= colon
 
 .PHONY: all doc doc-man doc-latex docclean print-versions welcome

All the lines are detected

./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Variables must not be exported:
	Makefile:# Nothing after: export PORT
	Makefile:export PORT = space
	Makefile:export PORT?= question_mark
	Makefile:export PORT= equal
	Makefile:export PORT:= colon

Issues/PRs references

Follow-up to #10440 and part of #10850

Add sanity check for removed exports.
@miri64
Copy link
Member

miri64 commented Sep 12, 2019

ACK. Tested by applying the following patch:

diff --git a/makefiles/tools/serial.inc.mk b/makefiles/tools/serial.inc.mk
index 215a5057e8..bad2d95f85 100644
--- a/makefiles/tools/serial.inc.mk
+++ b/makefiles/tools/serial.inc.mk
@@ -1,9 +1,9 @@
 # set default port depending on operating system
 OS := $(shell uname)
 ifeq ($(OS),Linux)
-  PORT ?= $(PORT_LINUX)
+  export PORT ?= $(PORT_LINUX)
 else ifeq ($(OS),Darwin)
-  PORT ?= $(PORT_DARWIN)
+  export PORT ?= $(PORT_DARWIN)
 endif
 ifeq ($(PORT),)
   $(info Warning: no PORT set!)
$ ./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Variables must not be exported:
	makefiles/tools/serial.inc.mk:  export PORT ?= $(PORT_LINUX)
	makefiles/tools/serial.inc.mk:  export PORT ?= $(PORT_DARWIN)
$ echo $?
1

When reverting the patch the script passes without any output and status code 0

@miri64 miri64 added Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 12, 2019
@jcarrano jcarrano merged commit 6570a9b into RIOT-OS:master Sep 12, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Sep 12, 2019

Thank you for the review.

@cladmi cladmi deleted the pr/buildsystem_sanity_check/port_not_exported branch September 12, 2019 14:52
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants