makefiles: remove exports so that PORT is not evaluated if it's not needed. by jcarrano · Pull Request #10440 · RIOT-OS/RIOT

jcarrano · 2018-11-20T11:42:57Z

Contribution description

Remove instances of := and export that were causing $(PORT) to be evaluated where it was not needed.

This is needed for #10342 (it verifies if port is not set). If term is not specified as a target, then PORT is not evaluated and no error will be produced.

Testing procedure

If I'm not mistaken, the test for this is already carried out by murdock.

Issues/PRs references

Split from: #10342

cladmi

Why including boards/cc2538dk and murdock.inc.mk in this one ?
There are also other boards that export the variables.

If they are required, maybe split them in different commits with justification.

I will add some testing procedure in an upcoming comment.

cladmi · 2018-11-20T13:23:59Z

makefiles/vars.inc.mk

 export GIT_CACHE_DIR         # path to git-cache cache directory
 export FLASHER               # The command to call on "make flash".
-export FFLAGS                # The parameters to supply to FLASHER.
+# FFLAGS                	 # The parameters to supply to FLASHER.


There is a tab included instead of spaces.

cladmi · 2018-11-20T13:35:07Z

I looked at the usages of PORT, the "problematic" ones could have been the one not being Makefile.include or make included file. I just left them to see the other exports.

But in the implementation flashutil.sh sets PORT internally, and start_network.sh gets it from command line.

So everything should be ok with PORT.

git grep PORT | grep  -e '{PORT}' -e '(PORT)' | grep -v -e '\.h' -e '\.c'
boards/cc2538dk/Makefile.include:  export FFLAGS  = -p "$(PORT)" -e -w -v $(HEXFILE)
boards/common/arduino-atmega/Makefile.include:export PROGRAMMER_FLAGS = -P $(PORT) -b $(PROGRAMMER_SPEED)
boards/common/msb-430/Makefile.include:  export MSPDEBUGFLAGS += -d $(PORT)
boards/common/msba2/Makefile.include:export TERMFLAGS += -tg -p "$(PORT)"
boards/common/msba2/Makefile.include:ifeq ($(PORT),)
boards/common/msba2/Makefile.include:export FFLAGS = $(PORT) $(HEXFILE)
boards/common/msba2/tools/flashutil.sh:        if [ "${PORT}" = "openocd" ]; then
boards/common/msba2/tools/flashutil.sh:            windows_flash_fm ${PORT}
boards/common/msba2/tools/flashutil.sh:        if [ "${PORT}" = "openocd" ]; then
boards/common/msba2/tools/flashutil.sh:        echo Flashing ${HEXFILE} to ${PORT}
boards/common/msba2/tools/flashutil.sh:            ${FLASHUTIL_SHELL} "${BASEDIR}/bin/lpc2k_pgm ${PORT} ${HEXFILE}; sleep 2" &
boards/common/wsn430/Makefile.include:export FFLAGS = -d $(PORT) -j uif "prog $(HEXFILE)"
boards/lobaro-lorabox/Makefile.include:FFLAGS += -p $(PORT) -e -u -S -l 0x1ff -w $(BINFILE)
boards/mulle/Makefile.include:ifeq ($(PORT),)
boards/mulle/Makefile.include:ifeq ($(PORT),)
boards/native/Makefile.include:export TERMFLAGS := $(PORT) $(TERMFLAGS)
boards/native/Makefile.include: $(VALGRIND) $(VALGRIND_FLAGS) $(ELFFILE) $(PORT)
boards/native/Makefile.include: $(VALGRIND) $(VALGRIND_FLAGS) $(ELFFILE) $(PORT)
boards/native/Makefile.include: $(VALGRIND) $(CACHEGRIND_FLAGS) $(ELFFILE) $(PORT)
boards/nz32-sc151/Makefile.include:export TERMFLAGS = -p $(PORT)
boards/openmote-cc2538/Makefile.include:  export FFLAGS  = -p "$(PORT)" -e -w -v -b 460800 $(HEXFILE)
boards/telosb/Makefile.include:export FFLAGS = --telosb -c $(PORT) -r -e -I -p $(HEXFILE)
boards/waspmote-pro/Makefile.include:  export PROGRAMMER_FLAGS = -P $(PORT) -b 115200
boards/z1/Makefile.include:export FFLAGS = --z1 -I -c $(PORT) -r -e -p $(HEXFILE)
cpu/esp32/Makefile.include:    export FFLAGS += --chip esp32 -p $(PORT) -b $(PROGRAMMER_SPEED)
cpu/esp8266/Makefile.include:    export FFLAGS += -p $(PORT) -b $(PROGRAMMER_SPEED) write_flash
dist/tools/ethos/start_network.sh:[ -z "${PORT}" -o -z "${TAP}" -o -z "${PREFIX}" ] && {
dist/tools/ethos/start_network.sh:create_tap && start_uhcpd && "${ETHOS_DIR}/ethos" ${TAP} ${PORT} ${BAUDRATE}
dist/tools/usb-serial/README.md:      ifeq ($(PORT),)
dist/tools/usb-serial/README.md:    ifeq ($(PORT),)
dist/tools/usb-serial/README.md:    ifeq ($(PORT),)
examples/gnrc_border_router/Makefile:TERMFLAGS ?= $(PORT) $(TAP) $(IPV6_PREFIX)
makefiles/info.inc.mk:  @echo 'PORT:      $(PORT)'
makefiles/tools/bossa.inc.mk:export FFLAGS  ?= -p $(PORT) -e -i -w -v -b -R $(HEXFILE)
makefiles/tools/bossa.inc.mk:  PREFFLAGS  ?= $(STTY_FLAG) $(PORT) raw ispeed 1200 ospeed 1200 cs8 -cstopb ignpar eol 255 eof 255
makefiles/tools/serial.inc.mk:ifeq ($(PORT),)
makefiles/tools/serial.inc.mk:    export TERMFLAGS ?= -p "$(PORT)" -b "$(BAUD)"
makefiles/tools/serial.inc.mk:    export TERMFLAGS ?= --nolock --imap lfcrlf --echo --baud "$(BAUD)" "$(PORT)"

cladmi · 2018-11-20T13:37:54Z

TERMPROG and TERMFLAGS usage are only in Makefile.include so do not need to be exported.

git grep TERMPROG | grep  -e '{TERMPROG}' -e '(TERMPROG)' | grep -v -e '\.h' -e '\.c'
Makefile.include:       $(call check_cmd,$(TERMPROG),Terminal program)
Makefile.include:       $(TERMPROG) $(TERMFLAGS)
makefiles/info.inc.mk:  @echo 'TERMPROG:  $(TERMPROG)'

git grep TERMFLAGS | grep  -e '{TERMFLAGS}' -e '(TERMFLAGS)' | grep -v -e '\.h' -e '\.c'
Makefile.include:       $(TERMPROG) $(TERMFLAGS)
boards/native/Makefile.include:export 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 'TERMFLAGS: $(TERMFLAGS)'

cladmi · 2018-11-20T13:40:14Z

FFLAGS is only in make included files so does not need to be exported:

git grep FFLAGS | grep  -e '{FFLAGS}' -e '(FFLAGS)' | grep -v -e '\.h' -e '\.c'
Makefile.include:       $(FLASHER) $(FFLAGS)
makefiles/info.inc.mk:  @echo 'FFLAGS:  $(FFLAGS)'
makefiles/mcuboot.mk:   FLASH_ADDR=0x0 $(FLASHER) $(FFLAGS)
makefiles/mcuboot.mk:   FLASH_ADDR=$(MCUBOOT_SLOT0_SIZE) $(FLASHER) $(FFLAGS)
makefiles/murdock.inc.mk:FLASHFILE:=$(filter $(HEXFILE) $(ELFFILE:.elf=.bin) $(ELFFILE),$(FFLAGS))

jcarrano · 2018-11-21T13:36:48Z

But in the implementation flashutil.sh sets PORT internally, and start_network.sh gets it from command line.

git grep flashutil shows no reference to flashutil outside of itself. I suspect this is dead code and is not being used anywhere.

jcarrano · 2019-01-15T18:48:43Z

@cladmi What is the status here?

cladmi · 2019-01-16T10:59:46Z

For the moment, there are still many exported PORT, FFLAGS which will be evaluated.
I would find it better to get rid of them completely step by step. Even maybe with a non-regression tests for variables that must not be exported.

Only removing the current ones will not be enough for #7695.

jcarrano · 2019-03-19T17:59:31Z

I rebased on top of master to solve the conflicts.

cladmi · 2019-03-20T11:05:34Z

My comment #10440 (comment) still holds.
This pull request is not enough for #7695 and with this, there are still cases where PORT is evaluated when not needed I would rather go for an in depth cleanup like followed in #10850

jcarrano · 2019-05-28T14:27:22Z

I rebased on master and added one more commit to remove one more evaluation.

cladmi · 2019-06-03T09:42:00Z

The goal was not to remove the warning but to replace it with #10342 from which this PR was split. Please remove the last commit. You can also rebase both PRs to allow testing properly.

Please also add the PORT variable as variable that must not be exported. There was an example in #11484

jcarrano · 2019-06-03T11:22:30Z

@cladmi #10342 happened so long ago that I forgot, I'm rebasing. One thing I noticed is that PORT_LINUX and PORT_DARWIN are exported in several places:

RIOT/boards/sodaq-explorer/Makefile.include

Lines 5 to 7 in 68dc5b0

    
           #export needed for flash rule 
        
           export PORT_LINUX ?= /dev/ttyACM0 
        
           export PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))

Should these be banned too?

Another thing: look at this doc, it is telling people to do things wrong. I'm fixing it as part of this PR, it thats Ok: https://github.com/RIOT-OS/RIOT/blob/2ba303bc8452d72122d1d82b889bf1fb1f9fcb12/dist/tools/usb-serial/README.md

cladmi · 2019-06-03T14:17:30Z

Yes the export PORT_* should be removed too (there are 7 instances from my tracking). The line before saying "export needed for flash rule" should also be removed :D
I will do it in parallel to this one as it does not affect the changes here.

I would indeed like to have the documentation updated at the same time too. Having the variable in dist/tools/buildsystem_sanity_check/check.sh should find the README.md you are mentioning if things work as I hope.

jcarrano · 2019-06-03T14:19:59Z

should find the README.md you are mentioning if things work as I hope.

That's exactly how I found it.

This variable is only used for the term recipe (and maybe for flashing). They should not be evaluated if they are not needed and the user should not see a warning that the port is not set if he does not use port (for example in make all.)

The example in the tool documentation contains several things that are wrong: - exports PORT. - Defines the port using :=. - Defines PORT instead of PORT_LINUX, PORT_DARWIN - ifeq-based logic (which will force an evaluation). I have not tested the new example script.

jcarrano · 2019-06-03T14:32:13Z

I rebased #10342 and then took the first two commits for this PR. I cannot add PORT to the check script untill all "export PORT_DARWIN/PORT_LINUX" is fixed as the regex for PORT also catches those.

cladmi · 2019-06-03T14:44:32Z

I rebased #10342 and then took the first two commits for this PR. I cannot add PORT to the check script untill all "export PORT_DARWIN/PORT_LINUX" is fixed as the regex for PORT also catches those.

It does not if you do UNEXPORTED_VARIABLES+=('PORT[ ?=]' 'PORT$') as used as example in #11484 ;) (in the testing procedure. I added the diff coloring to show it better as example)

cladmi · 2019-06-04T13:04:50Z

dist/tools/usb-serial/README.md

-          PORT := $(shell ls -1 /dev/tty.SLAB_USBtoUART* | head -n 1)
-        endif
-    endif
+    PORT_LINUX_EXACT = $(if $(PROGRAMMER_SERIAL),$(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh "^$(PROGRAMMER_SERIAL)$$")),)


This changes more than removing exports. Only remove the mention to export here I would say.

When changing this one, the files using the pattern will also need to be updated, and even put in common which was somehow done in #7695 (with other issues)

git grep 'usb-serial/find-tty.sh' boards/cc2538dk/Makefile.include:PORT_LINUX ?= $(word 2,$(shell $(RIOTTOOLS)/usb-serial/find-tty.sh '^$(PROGRAMMER_SERIAL)')) boards/mulle/Makefile.include: PORT := $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh '^$(DEBUG_ADAPTER_ID)$$')) boards/mulle/Makefile.include: PORT := $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh)) boards/pba-d-01-kw2x/Makefile.include: SERIAL_TTY = $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh $(SERIAL))) dist/tools/usb-serial/README.md: PORT := $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh "^$(PROGRAMMER_SERIAL)$$")) dist/tools/usb-serial/README.md: PORT := $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh)) makefiles/boards/sam0.inc.mk: SERIAL_TTY = $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh $(SERIAL)))

the ifeqs should also go

Nope, we still kept it in the serial.inc.mk for this PR as it only remove exports as removing this warning is done by #10342

ifeq ($(PORT),) $(info Warning: no PORT set!) endif

Otherwise, the whole change should be consistent globally and may require other dependencies to fix boards.

fjmolinas · 2019-09-12T07:13:58Z

@cladmi Does your #10440 (comment) still stand here?

cladmi · 2019-09-12T10:09:11Z

From the Makefile only it is good like this.

My remark is about the whole change in the documentation that for me should be done in #10342, expect removing 'export PORT' that should stay there.

fjmolinas

Changes look good and murdock takes care of testing. ACK

jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Nov 20, 2018

jcarrano requested a review from cladmi November 20, 2018 11:42

jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 20, 2018

jcarrano mentioned this pull request Nov 20, 2018

makefiles/tools/serial: Check empty port #10342

Closed

cladmi reviewed Nov 20, 2018

View reviewed changes

This was referenced Nov 22, 2018

boards/msba2: Do not set PORT. #10454

Merged

make: make clean all does not make sense and should be removed #10459

Closed

jcarrano force-pushed the no-export-ports branch from a248c47 to 83825b6 Compare November 23, 2018 12:50

jcarrano added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 26, 2018

cladmi mentioned this pull request Dec 19, 2018

Makefiles: add support to generate both .hex and .bin file and add FLASHFILE variable #8838

Merged

49 tasks

jcarrano force-pushed the no-export-ports branch from 83825b6 to 39c3931 Compare March 19, 2019 17:59

cladmi mentioned this pull request May 3, 2019

dist/tools/buildsystem_sanity_check: add an export variable check #11484

Merged

cladmi mentioned this pull request May 20, 2019

make: remove exports for flash debug reset #11554

Merged

jcarrano force-pushed the no-export-ports branch from 39c3931 to 955b75a Compare May 28, 2019 14:25

jcarrano force-pushed the no-export-ports branch from 955b75a to 7f1357b Compare June 3, 2019 14:30

cladmi and others added 2 commits June 3, 2019 16:31

serial: Do not export PORT.

c103b3f

This variable is only used for the term recipe (and maybe for flashing). They should not be evaluated if they are not needed and the user should not see a warning that the port is not set if he does not use port (for example in make all.)

cladmi mentioned this pull request Jun 3, 2019

board: remove exporting PORT_LINUX|PORT_DARWIN #11619

Merged

cladmi suggested changes Jun 4, 2019

View reviewed changes

cladmi mentioned this pull request Jun 4, 2019

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

Closed

19 tasks

jcarrano requested a review from fjmolinas September 10, 2019 11:44

fjmolinas approved these changes Sep 12, 2019

View reviewed changes

fjmolinas merged commit 2d890db into RIOT-OS:master Sep 12, 2019

cladmi mentioned this pull request Sep 12, 2019

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

Merged

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Conversation

jcarrano commented Nov 20, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

jcarrano Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 20, 2018

Uh oh!

jcarrano commented Nov 21, 2018

Uh oh!

jcarrano commented Jan 15, 2019

Uh oh!

cladmi commented Jan 16, 2019

Uh oh!

jcarrano commented Mar 19, 2019

Uh oh!

cladmi commented Mar 20, 2019

Uh oh!

jcarrano commented May 28, 2019

Uh oh!

cladmi commented Jun 3, 2019

Uh oh!

jcarrano commented Jun 3, 2019

Uh oh!

cladmi commented Jun 3, 2019

Uh oh!

jcarrano commented Jun 3, 2019

Uh oh!

jcarrano commented Jun 3, 2019

Uh oh!

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

Uh oh!

Uh oh!

cladmi Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

jcarrano Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 4, 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!

fjmolinas commented Sep 12, 2019

Uh oh!

cladmi commented Sep 12, 2019

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi commented Nov 20, 2018 •

edited

Loading

cladmi commented Nov 20, 2018 •

edited

Loading

cladmi commented Jun 3, 2019 •

edited

Loading

cladmi Jun 4, 2019 •

edited

Loading