make: remove exports for flash debug reset by cladmi · Pull Request #11554 · RIOT-OS/RIOT

cladmi · 2019-05-20T18:21:02Z

Contribution description

This removes export for all variables related to flash/reset/debug.
They do not need to be exported as they are all evaluated in Makefile.include or a file included by it.

This does not remove export PORT for the moment as it is easier to review it on its own as it will have a different review scheme

Testing procedure

There is not real testing procedure except, it would all still be working, but not really realistic for testing.

Better do an in depth review to find issues.
If any issue happened because of this, it could be easier to fix the missing case.

Running make flash/make reset/make debug still works on one of your boards.
At least it should not break CI.

Review prodecure

The only usages of the variables are in Makefile.include or .inc.mk files:

git grep -e '$(FFLAGS)' -e '$(FLASHER)' -e '$(RESET)' -e '$(RESET_FLAGS)' -e '$(DEBUGGER)' -e '$(DEBUGGER_FLAGS)' -e '$(DEBUGSERVER)' -e '$(DEBUGSERVER_FLAGS)' -e '$(MSPDEBUGFLAGS)'
Makefile.include:  $(call check_cmd,$(FLASHER),Flash program)
Makefile.include:  $(FLASHER) $(FFLAGS)
Makefile.include:       $(call check_cmd,$(DEBUGGER),Debug program)
Makefile.include:       $(DEBUGGER) $(DEBUGGER_FLAGS)
Makefile.include:       $(call check_cmd,$(DEBUGSERVER),Debug server program)
Makefile.include:       $(DEBUGSERVER) $(DEBUGSERVER_FLAGS)
Makefile.include:       $(call check_cmd,$(RESET),Reset program)
Makefile.include:       $(RESET) $(RESET_FLAGS)
boards/common/msb-430/Makefile.include:FFLAGS = $(MSPDEBUGFLAGS) "prog $(HEXFILE)"
boards/common/msb-430/Makefile.include:DEBUGSERVER = $(FLASHER)
boards/common/msb-430/Makefile.include:DEBUGSERVER_FLAGS = $(MSPDEBUGFLAGS) gdb
boards/common/msba2/Makefile.include:FLASHDEPS += $(if $(findstring $(LPC2K_PGM),$(FLASHER)),$(LPC2K_PGM))
boards/native/Makefile.include:ifeq ($(shell basename $(DEBUGGER)),lldb)
boards/native/Makefile.include: $(eval DEBUGGER_FLAGS := -ex "target remote | vgdb --pid=$(VALGRIND_PID)" $(DEBUGGER_FLAGS))
boards/native/Makefile.include: $(DEBUGGER) $(DEBUGGER_FLAGS)
boards/teensy31/Makefile.include:ifeq ($(TEENSY_LOADER),$(FLASHER))
makefiles/info.inc.mk:  @echo 'FLASHER: $(FLASHER)'
makefiles/info.inc.mk:  @echo 'FFLAGS:  $(FFLAGS)'
makefiles/info.inc.mk:  @echo 'DEBUGGER:       $(DEBUGGER)'
makefiles/info.inc.mk:  @echo 'DEBUGGER_FLAGS: $(DEBUGGER_FLAGS)'
makefiles/info.inc.mk:  @echo 'DEBUGSERVER:       $(DEBUGSERVER)'
makefiles/info.inc.mk:  @echo 'DEBUGSERVER_FLAGS: $(DEBUGSERVER_FLAGS)'
makefiles/info.inc.mk:  @echo 'RESET:       $(RESET)'
makefiles/info.inc.mk:  @echo 'RESET_FLAGS: $(RESET_FLAGS)'
makefiles/murdock.inc.mk:FLASHFILE ?= $(filter $(HEXFILE) $(ELFFILE:.elf=.bin) $(ELFFILE),$(FFLAGS))
makefiles/tools/avrdude.inc.mk:DEBUGGER = $(DIST_PATH)/debug.sh $(DEBUGSERVER_FLAGS) $(DIST_PATH) $(DEBUGSERVER_PORT)
makefiles/tools/bossa.inc.mk:ifeq ($(RIOTTOOLS)/bossa/bossac,$(FLASHER))
makefiles/tools/edbg.inc.mk:ifeq ($(RIOT_EDBG),$(FLASHER))

No match when using ${}

git grep -e '${FFLAGS}' -e '${FLASHER}' -e '${RESET}' -e '${RESET_FLAGS}' -e '${DEBUGGER}' -e '${DEBUGGER_FLAGS}' -e '${DEBUGSERVER}' -e '${DEBUGSERVER_FLAGS}' -e '${MSPDEBUGFLAGS}'

Only required changes are changed

The only changes are [-export-] and comments {+#+}

git diff --color=auto --word-diff 1dc479de0 '**' ':!dist/tools/buildsystem_sanity_check/check.sh'

Only the right amount of lines have been changed

git diff  --stat  1dc479de0 '**' ':!dist/tools/buildsystem_sanity_check/check.sh'
...
 29 files changed, 181 insertions(+), 181 deletions(-)

Edited to 29 files changed.

This includes:

170 export changed.
8 modifications in makefiles/vars.inc.mk
2 lines for MSPDEBUGFLAGS (that I did not added to build_system_check as really specific)
1 newline that is reported in cpu/esp32/Makefile.include for PREFFFLAGS

And from the buildsystem_sanity_check output, on the original commit 1dc479d

git checkout d486598fdf3c72b7b110ff97a9ea2dbb28f50e1f -- dist/tools/buildsystem_sanity_check/check.sh
./dist/tools/buildsystem_sanity_check/check.sh  | grep -v 'Invalid build system patterns found by' | wc -l
178

Edited from 170 to 178 now that makefiles/vars.inc.mk is also parsed

Matches the number of export changed

Issues/PRs references

Part of #10850
In depth cleanup required for #10440

cladmi · 2019-05-23T16:08:48Z

The branch will need to be rebased, but I will do it after a review as I used commits hashs for simplifying the testing.

cladmi · 2019-05-23T16:09:25Z

All the tests succeeded on murdock, so I disable running tests for the moment.

Handle differently variables that are exported in `vars.inc.mk` from the ones that should not. This is needed for the upcoming variables change that should also be removed from `vars.inc.mk` right now. Keeping the old behavior will help migrating other variables more easily by keeping them only exported in vars.inc.mk in the first time.

FLASHER and FFLAGS are evaluated by the main Makefile.include or by file included by it. Their value does not need to be exported. This will also prevent evaluating 'PORT' for FFLAGS when not needed. Testing ------- `git diff --word-diff` only reports `export` being removed. `git show --stat` reports `84 insertions(+), 84 deletions(-)` Which is the same amount as lines that where matching `export[[:blank::]]\+VARIABLE`.

Add sanity check for removed exports.

RESET and RESET_FLAGS are evaluated by the main Makefile.include or by file included by it. Their value does not need to be exported. This will also prevent evaluating 'PORT' for RESET_FLAGS when not needed. Testing ------- `git diff --word-diff` only reports `export` being removed. `git show --stat` reports `24 insertions(+), 24 deletions(-)` Which is the same amount as lines that where matching `export[[:blank::]]\+VARIABLE`.

Add sanity check for removed exports.

DEBUGGER/DEBUGGER_FLAGS/DEBUGSERVER/DEBUGSERVER_FLAGS are evaluated by the main Makefile.include or by file included by it. Their value does not need to be exported. Testing ------- `git diff --word-diff` only reports `export` being removed. `git show --stat` reports `55 insertions(+), 55 deletions(-)` Which is the same amount as lines that where matching `export[[:blank::]]\+VARIABLE`.

Add sanity check for removed exports.

MSPDEBUGFLAGS is evaluated only in the same file. Its value does not need to be exported. This will also prevent evaluating 'PORT' for MSPDEBUGFLAGS when not needed.

PREFLASHER/PREFFLAGS/FLASHDEPS are evaluated by the main Makefile.include. Their value does not need to be exported. Testing ------- `git diff --word-diff` only reports `export` being removed. `git show --stat` reports `16 insertions(+), 16 deletions(-)` Which is the same amount as lines that where matching `export[[:blank::]]\+VARIABLE` plus the newline that is said to have changed.

Add sanity check for removed exports.

cladmi · 2019-05-28T08:05:35Z

The branch will need to be rebased, but I will do it after a review as I used commits hashs for simplifying the testing.

I found the time to rebase and update the PR description accordingly.

fjmolinas · 2019-05-28T14:00:07Z

First basic tests, without taking a deeper look into the commits: can still run make flash/make reset/make debug on my boards.

fjmolinas · 2019-05-28T14:04:48Z

I got 29 files changes instead of 28, maybe after the rebase?

git diff  --stat  1dc479de0 '**' ':!dist/tools/buildsystem_sanity_check/check.sh'
...
 29 files changed, 181 insertions(+), 181 deletions(-)

fjmolinas

Reviewed the commit history, every commit individually makes sense and the tests run individually at the different stages make sense as well.

Flashed/Debugged/Rested: frdm-kw64f, samr21-xpro, nucleo-l152re, nucleol476rg.

Everything looks good to me so ACK.

cladmi · 2019-05-28T15:12:54Z

I got 29 files changes instead of 28, maybe after the rebase?

git diff  --stat  1dc479de0 '**' ':!dist/tools/buildsystem_sanity_check/check.sh'
...
 29 files changed, 181 insertions(+), 181 deletions(-)

I was missing the makefiles/vars.inc.mk in that listing as I added it later. My bad.
EDIT: I was focusing more no the insertions and did not even looked the files changed

cladmi · 2019-05-28T16:14:57Z

Note for the future.

This may break if new boards that gets merged without re-running tests on master.

Only change required is to remove the export on the FLASHER/FFLAGS/DEBUGGER/DEBUGGER_FLAGS/DEBUGSERVER/DEBUGSERVER_FLAGS/RESET/RESET_FLAGS variables.

cladmi · 2019-05-28T16:15:17Z

Thank you for the review.

cladmi · 2019-05-28T16:17:23Z

Now its time to rebase the FLASHFILE changes.

cladmi added this to the Release 2019.07 milestone May 20, 2019

cladmi changed the title ~~Pr/make/exports/remove flash debug reset~~ make: remove exports for flash debug reset May 20, 2019

cladmi requested a review from jcarrano May 23, 2019 16:07

cladmi removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label May 23, 2019

cladmi mentioned this pull request May 27, 2019

boards/cc2538: use FLASHFILE for boards using cc2538-bsl.py #11569

Merged

cladmi requested review from aabadie and fjmolinas May 28, 2019 07:51

cladmi added 10 commits May 28, 2019 09:53

dist/tools/buildsystem_sanity_check: check no FLASHER/FFLAGS exports

95c07c5

Add sanity check for removed exports.

dist/tools/buildsystem_sanity_check: check no RESET/RESET_FLAGS exports

734410b

Add sanity check for removed exports.

dist/tools/buildsystem_sanity_check: check no DEBUG* exports

ea51cf3

Add sanity check for removed exports.

boards/tools: remove exporting MSPDEBUGFLAGS

ff00096

MSPDEBUGFLAGS is evaluated only in the same file. Its value does not need to be exported. This will also prevent evaluating 'PORT' for MSPDEBUGFLAGS when not needed.

dist/tools/buildsystem_sanity_check: check no PREFLASH* exports

d486598

Add sanity check for removed exports.

cladmi force-pushed the pr/make/exports/remove_flash_debug_reset branch from acb839f to d486598 Compare May 28, 2019 08:04

fjmolinas approved these changes May 28, 2019

View reviewed changes

cladmi merged commit dedbe9c into RIOT-OS:master May 28, 2019

cladmi deleted the pr/make/exports/remove_flash_debug_reset branch May 28, 2019 16:17

This was referenced May 28, 2019

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

Closed

makefiles/tools/uniflash.inc.mk: remove empty RESET_FLAGS #11617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make: remove exports for flash debug reset#11554

make: remove exports for flash debug reset#11554
cladmi merged 10 commits intoRIOT-OS:masterfrom
cladmi:pr/make/exports/remove_flash_debug_reset

cladmi commented May 20, 2019 •

edited

Loading

Uh oh!

cladmi commented May 23, 2019

Uh oh!

cladmi commented May 23, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

fjmolinas left a comment

Uh oh!

cladmi commented May 28, 2019 •

edited

Loading

Uh oh!

cladmi commented May 28, 2019 •

edited

Loading

Uh oh!

cladmi commented May 28, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Contribution description

Testing procedure

Review prodecure

Only required changes are changed

Issues/PRs references

Uh oh!

cladmi commented May 23, 2019

Uh oh!

cladmi commented May 23, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cladmi commented May 28, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cladmi commented May 20, 2019 •

edited

Loading

cladmi commented May 28, 2019 •

edited

Loading

cladmi commented May 28, 2019 •

edited

Loading