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

unexport TERM* variables #11064

Merged
merged 3 commits into from
Feb 25, 2019
Merged

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Feb 25, 2019

Contribution description

TERMPROG and TERMFLAGS variables do not need to be exported as they are
used directly by a make receipe.

Exporting them prevents overwriting 'RIOT_TERMINAL' in the test.

Other uses to unexport:

There is one remaining usage of export TERMFLAGS but it should be handled alone as it does not make sense as it does not handle RIOT_TERMINAL. I will add it to the tracking issue.

git grep -e 'export TERM'
boards/nz32-sc151/Makefile.include:export TERMFLAGS = -p $(PORT)

Testing procedure

Review

We can see that there are not usages in other places than recipes that are declared in Makefile.include:

git grep -e '$(TERMPROG)' -e '$(TERMFLAGS)' -e '${TERMPROG}' -e '${TERMFLAGS}'
Makefile.include:       $(call check_cmd,$(TERMPROG),Terminal program)
Makefile.include:       $(TERMPROG) $(TERMFLAGS)
boards/native/Makefile.include:TERMFLAGS := $(PORT) $(TERMFLAGS)
boards/native/Makefile.include:  export DEBUGGER_FLAGS = -- $(ELFFILE) $(TERMFLAGS)
boards/native/Makefile.include:  export DEBUGGER_FLAGS = -q --args $(ELFFILE) $(TERMFLAGS)
makefiles/info.inc.mk:  @echo 'TERMPROG:  $(TERMPROG)'
makefiles/info.inc.mk:  @echo 'TERMFLAGS: $(TERMFLAGS)'

There are no more exports of TERMFLAGS or TERMPROG except the one already mentioned earlier:

git grep -e 'export TERM'
boards/nz32-sc151/Makefile.include:export TERMFLAGS = -p $(PORT)

Testing make term

It is still working for both native and boards. You can try

make -C examples/hello-world/ all term
BOARD=samr21-xpro make -C examples/default all term

The term-gprof for native is not working both in master and with this PR.

Overwriting RIOT_TERMINAL in testrunner:

The usage within make test can be checked with this diff.
With it, running a test on a board should use socat

diff --git a/dist/pythonlibs/testrunner/__init__.py b/dist/pythonlibs/testrunner/__init__.py
index 67489f00c..06f086a02 100755
--- a/dist/pythonlibs/testrunner/__init__.py
+++ b/dist/pythonlibs/testrunner/__init__.py
@@ -38,6 +38,7 @@ def find_exc_origin(exc_info):
 
 def run(testfunc, timeout=10, echo=True, traceback=False):
     env = os.environ.copy()
+    env['RIOT_TERMINAL'] = 'socat'
     child = pexpect.spawnu("make term", env=env, timeout=timeout, codec_errors='replace')
BOARD=samr21-xpro  make -C tests/bloom_bytes/ flash test
make: Entering directory '/home/harter/work/git/RIOT/tests/bloom_bytes'
...
Programming..................................................... done.
Verification..................................................... done.
socat - open:/dev/ttyACM0,b115200,echo=0,raw
adding 512 elements took 262ms
checking 10000 elements took 2468ms

267 elements probably in the filter.
9733 elements not in the filter.
 false positive rate.

All done!
main(): Tmain(): This is RIOT! (Version: 2019.04-devel-269-g3b11-pr/bug/export/term_variables)
Testing Bloom filter.

m: 4096 k: 8

adding 512 elements took 262ms
checking 10000 elements took 2468ms

267 elements probably in the filter.
9733 elements not in the filter.
 false positive rate.

All done!

make: Leaving directory '/home/harter/work/git/RIOT/tests/bloom_bytes'

Issues/PRs references

Tracking issue #10850
Was an effective issue while working on #10952

TERMPROG and TERMFLAGS variables do not need to be exported as they are
used directly by a make receipe.

Exporting them prevents overwriting 'RIOT_TERMINAL' in the test.
TERMPROG and TERMFLAGS variables do not need to be exported as they are
used directly by a make receipe.

Exporting them prevents overwriting 'RIOT_TERMINAL' in the test.
TERMPROG and TERMFLAGS variables do not need to be exported as they are
used directly by a make receipe.
@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 labels Feb 25, 2019
@cladmi cladmi requested review from aabadie and jcarrano February 25, 2019 16:42
@cladmi cladmi added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Feb 25, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Feb 25, 2019

Please tell me if you see other testing procedure I missed.

@jcarrano
Copy link
Contributor

@aabadie, you added the boards/nz32-sc151, but it looks kind of broken in the way it sets TERMFLAGS (it won't work if one changes RIOT_TERMINAL).

@cladmi
Copy link
Contributor Author

cladmi commented Feb 25, 2019

I am doing a pull request for the nz32 alone.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

Tested make term in samr21. It still works. Also, the exports were clearly unnecessary.

@jcarrano jcarrano self-requested a review February 25, 2019 17:01
@jcarrano jcarrano merged commit 7e037d7 into RIOT-OS:master Feb 25, 2019
@cladmi cladmi added this to the Release 2019.04 milestone Feb 25, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Feb 25, 2019

Thank you for the review.

@cladmi cladmi deleted the pr/bug/export/term_variables branch February 25, 2019 17:06
@danpetry danpetry modified the milestone: Release 2019.04 Mar 11, 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants