Skip to content

make: boards: remove use of export with debug adapter related variables#11614

Merged
cladmi merged 7 commits intoRIOT-OS:masterfrom
aabadie:pr/make/export_debug_adapter_remove
Jun 3, 2019
Merged

make: boards: remove use of export with debug adapter related variables#11614
cladmi merged 7 commits intoRIOT-OS:masterfrom
aabadie:pr/make/export_debug_adapter_remove

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Jun 3, 2019 •
edited by cladmi
Loading

Contribution description

There's no need to export DEBUG_ADAPTER/DEBUG_ADAPTER_ID/ST_LINK variables since they are not passed to an external process.

Testing procedure

  • Verify that flashing a sam0 based board (samr21-xpro, samr30-xpro, etc) works. Try also by passing DEBUG_ADAPTER_ID with multiple boards plugged on the same computer
  • Verify that flashing an st-link based board (one that is modified by this PR) works

Issues/PRs references

Removal of unused exports #10850

@aabadie aabadie added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up 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 Area: boards Area: Board ports labels Jun 3, 2019
@aabadie aabadie requested a review from cladmi June 3, 2019 06:20
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jun 3, 2019

Indeed all usages of these variables are inside directly included makefiles:

git grep -e '$(DEBUG_ADAPTER)' -e '${DEBUG_ADAPTER}' -e '$(DEBUG_ADAPTER_ID)' -e '${DEBUG_ADAPTER_ID}' -e '$(STLINK_.*)' -e '${STLINK_.*}'
boards/mulle/Makefile.include:    ifneq ($(DEBUG_ADAPTER_ID),)
boards/mulle/Makefile.include:      PORT := $(firstword $(shell $(RIOTTOOLS)/usb-serial/find-tty.sh '^$(DEBUG_ADAPTER_ID)$$'))
boards/mulle/Makefile.include:    ifneq ($(DEBUG_ADAPTER_ID),)
boards/mulle/Makefile.include:      PORT := /dev/tty.usbserial-$(DEBUG_ADAPTER_ID)B
makefiles/boards/sam0.inc.mk:ifeq ($(DEBUG_ADAPTER),dap)
makefiles/tools/edbg.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/edbg.inc.mk:  EDBG_ARGS += --serial $(DEBUG_ADAPTER_ID)
makefiles/tools/openocd-adapters/dap.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/openocd-adapters/dap.inc.mk:  OPENOCD_ADAPTER_INIT += -c 'cmsis_dap_serial $(DEBUG_ADAPTER_ID)'
makefiles/tools/openocd-adapters/jlink.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/openocd-adapters/jlink.inc.mk:  OPENOCD_ADAPTER_INIT += -c 'jlink serial $(DEBUG_ADAPTER_ID)'
makefiles/tools/openocd-adapters/mulle.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/openocd-adapters/mulle.inc.mk:  ifeq "100" "$(word 1, $(sort 100 $(DEBUG_ADAPTER_ID)))"
makefiles/tools/openocd-adapters/mulle.inc.mk:    ifneq "149" "$(word 1, $(sort 149 $(DEBUG_ADAPTER_ID)))"
makefiles/tools/openocd-adapters/mulle.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/openocd-adapters/mulle.inc.mk:  OPENOCD_ADAPTER_INIT += -c 'ftdi_serial $(DEBUG_ADAPTER_ID)'
makefiles/tools/openocd-adapters/stlink.inc.mk:  -c 'source [find interface/stlink-v$(STLINK_VERSION).cfg]' \
makefiles/tools/openocd-adapters/stlink.inc.mk:ifneq (,$(DEBUG_ADAPTER_ID))
makefiles/tools/openocd-adapters/stlink.inc.mk:  OPENOCD_ADAPTER_INIT += -c 'hla_serial $(DEBUG_ADAPTER_ID)'
makefiles/tools/openocd.inc.mk:ifneq (,$(DEBUG_ADAPTER))
makefiles/tools/openocd.inc.mk:  include $(RIOTMAKE)/tools/openocd-adapters/$(DEBUG_ADAPTER).inc.mk

Could you also add these variables to the list on variables that must not be exported

RIOT/dist/tools/buildsystem_sanity_check/check.sh

Line 57 in f8c070c

UNEXPORTED_VARIABLES+=('PREFLASHER' 'PREFFLAGS' 'FLASHDEPS')

It should prevent new usages to export them.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 3, 2019

Could you also add these variables to the list on variables that must not be exported

Wasn't aware of this. Will do.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jun 3, 2019

Could you also add these variables to the list on variables that must not be exported

Wasn't aware of this. Will do.

I will update the tracking issue about exports.

@cladmi cladmi mentioned this pull request Jun 3, 2019
Closed
19 tasks
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 3, 2019

@cladmi, I added the variable to the build system sanity check script. In the meantime, I also cleaned up the PROGRAMMER_SERIAL variable (only done in one place)

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jun 3, 2019

There are two commented DEBUG_ADAPTER_ID usage still reported by the check script. You can fix them directly fix them and squash.

@aabadie aabadie force-pushed the pr/make/export_debug_adapter_remove branch from f5f111e to 06013cf Compare June 3, 2019 08:32
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 3, 2019

There are two commented DEBUG_ADAPTER_ID usage still reported by the check script.

Fixed

cladmi
cladmi approved these changes Jun 3, 2019
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.

ACK, the variables do not need to be updated.

Also from analysis, the number of warnings reported by the updated ./dist/tools/buildsystem_sanity_check/check.sh in master match the number of changes.
A check with --word-diff only shows removing exports.

@cladmi cladmi merged commit 0d090ad into RIOT-OS:master Jun 3, 2019
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 3, 2019

Thanks @cladmi :)

@aabadie aabadie deleted the pr/make/export_debug_adapter_remove branch June 3, 2019 12:14
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: boards Area: Board ports 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aabadie @cladmi