Skip to content

*/Makefile.dep: remove usage of DEFAULT_MODULE += stdio%#13791

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
fjmolinas:pr_remove_stdio_default_module
Apr 2, 2020
Merged

*/Makefile.dep: remove usage of DEFAULT_MODULE += stdio%#13791
aabadie merged 2 commits intoRIOT-OS:masterfrom
fjmolinas:pr_remove_stdio_default_module

Conversation

@fjmolinas
Copy link
Contributor

Contribution description

This PR removes all usage of DEFAULT_MODULE += stdio_%. When this was added it was to make explicit an implementation choice, from all stdio_ modules which one to use in an application.
Using DEFAULT_MODULE made the "preferred" choice clear, and also allowed for applications or applications requirements to change the preferred one.

This seemed like a good choice at the time because DEFAULT_MODULE and the issues with its recursive expansion where not properly understood. This leads to a lot of weird corner cases, comments in #13651 have been in the direction of simply removing that kind of usage. In #13785 I had included a temporal exception for stdio_ until something like #13469 (comment)_ or as a result of #13469.

But know I think this is already causing more harm than good, because its poorly understood and also causes issues in info-boards-supported where recursive expansion of DEFAULT_MODULE doesn't happen.

Maybe the declaration of STDIO_MODULES can be moved earlier?

PS: @miri64 and @haukepetersen had spoken against this before, but we didn't understand DEFAULT_MODULEs issues at the time, there gut feeling seemed to be right.

Testing procedure

  • green murdock (lets see if issues stop popping up :))

  • test on BOARD that used to declare DEFAULT_MODULE += stdio_%

BOARD=arduino-mkrfox1200 make -C tests/xtimer_usleep flash
make: Entering directory '/home/francisco/workspace/RIOT2/tests/xtimer_usleep'
Building application "tests_xtimer_usleep" for "arduino-mkrfox1200" with MCU "samd21".

"make" -C /home/francisco/workspace/RIOT2/boards/arduino-mkrfox1200
"make" -C /home/francisco/workspace/RIOT2/boards/common/arduino-mkr
"make" -C /home/francisco/workspace/RIOT2/boards/common/samd21-arduino-bootloader
"make" -C /home/francisco/workspace/RIOT2/core
"make" -C /home/francisco/workspace/RIOT2/cpu/samd21
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common
"make" -C /home/francisco/workspace/RIOT2/cpu/cortexm_common/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/sam0_common
"make" -C /home/francisco/workspace/RIOT2/cpu/sam0_common/periph
"make" -C /home/francisco/workspace/RIOT2/cpu/samd21/periph
"make" -C /home/francisco/workspace/RIOT2/drivers
"make" -C /home/francisco/workspace/RIOT2/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT2/sys
"make" -C /home/francisco/workspace/RIOT2/sys/auto_init
"make" -C /home/francisco/workspace/RIOT2/sys/auto_init/usb
"make" -C /home/francisco/workspace/RIOT2/sys/div
"make" -C /home/francisco/workspace/RIOT2/sys/event
"make" -C /home/francisco/workspace/RIOT2/sys/isrpipe
"make" -C /home/francisco/workspace/RIOT2/sys/newlib_syscalls_default
"make" -C /home/francisco/workspace/RIOT2/sys/pm_layered
"make" -C /home/francisco/workspace/RIOT2/sys/test_utils/interactive_sync
"make" -C /home/francisco/workspace/RIOT2/sys/tsrb
"make" -C /home/francisco/workspace/RIOT2/sys/usb/usbus
"make" -C /home/francisco/workspace/RIOT2/sys/usb/usbus/cdc/acm
"make" -C /home/francisco/workspace/RIOT2/sys/usb_board_reset
"make" -C /home/francisco/workspace/RIOT2/sys/xtimer
   text	   data	    bss	    dec	    hex	filename
  17580	    132	   5908	  23620	   5c44	/home/francisco/workspace/RIOT2/tests/xtimer_usleep/bin/arduino-mkrfox1200/tests_xtimer_usleep.elf
stty -F /dev/ttyACM0 raw ispeed 1200 ospeed 1200 cs8 -cstopb ignpar eol 255 eof 255
sleep 1
/home/francisco/workspace/RIOT2/dist/tools/bossa-1.9/bossac -p /dev/ttyACM0 -o 0x2000 -e -i -w -v -b -R /home/francisco/workspace/RIOT2/tests/xtimer_usleep/bin/arduino-mkrfox1200/tests_xtimer_usleep.bin
Device       : ATSAMD21x18
Version      : v2.0 [Arduino:XYZ] Mar 10 2017 12:20:17
Address      : 0x0
Pages        : 4096
Page Size    : 64 bytes
Total Size   : 256KB
Planes       : 1
Lock Regions : 16
Locked       : none
Security     : false
BOD          : true
BOR          : true
Erase flash

Done in 0.840 seconds
Write 17712 bytes to flash (277 pages)
[==============================] 100% (277/277 pages)
Done in 0.101 seconds
Verify 17712 bytes of flash
[==============================] 100% (277/277 pages)
Verify successful
Done in 0.179 seconds
Set boot flash true

The "default" is still excluded when needed to be.

BOARD=arduino-mkrfox1200 make -C examples/gnrc_border_router/ info-debug-variable-USEMODULE | grep stdio
auto_init auto_init_gnrc_ipv6 auto_init_gnrc_ipv6_nib auto_init_gnrc_netif auto_init_gnrc_pktbuf auto_init_gnrc_sixlowpan auto_init_gnrc_udp auto_init_gnrc_uhcpc auto_init_random auto_init_xtimer board boards_common_arduino-mkr core core_init core_mbox core_msg core_panic cortexm_common cortexm_common_periph cpu div ethos evtimer fmt gnrc gnrc_icmpv6 gnrc_icmpv6_echo gnrc_ipv6 gnrc_ipv6_hdr gnrc_ipv6_nib gnrc_ipv6_nib_6lbr gnrc_ipv6_nib_6ln gnrc_ipv6_nib_6lr gnrc_ipv6_nib_router gnrc_ipv6_router gnrc_ipv6_router_default gnrc_ndp gnrc_netapi gnrc_netapi_mbox gnrc_netdev_default gnrc_netif gnrc_netif_ethernet gnrc_netif_hdr gnrc_netif_init_devs gnrc_netreg gnrc_pkt gnrc_pktbuf gnrc_pktbuf_static gnrc_sixlowpan gnrc_sixlowpan_border_router_default gnrc_sixlowpan_ctx gnrc_sixlowpan_frag gnrc_sixlowpan_frag_fb gnrc_sixlowpan_frag_rb gnrc_sixlowpan_iphc gnrc_sixlowpan_iphc_nhc gnrc_sixlowpan_nd gnrc_sixlowpan_router gnrc_sock gnrc_sock_udp gnrc_udp gnrc_uhcpc icmpv6 inet_csum iolist ipv6_addr ipv6_hdr isrpipe l2util luid netdev_default netdev_eth netif newlib newlib_nano newlib_syscalls_default periph periph_common periph_cpuid periph_gpio periph_init periph_init_cpuid periph_init_gpio periph_init_pm periph_init_timer periph_init_uart periph_pm periph_timer periph_uart pm_layered posix_headers posix_inet prng prng_tinymt32 ps random sam0_common_periph shell shell_commands sixlowpan sock sock_udp stdin stdio_ethos stdio_uart stdio_uart_rx sys tinymt32 tsrb udp uhcpc xtimer

Issues/PRs references

Related to #13651 #13469
Introduced in #12724

@fjmolinas fjmolinas added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 2, 2020
@fjmolinas fjmolinas requested review from aabadie and miri64 April 2, 2020 07:40
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2020
@fjmolinas fjmolinas force-pushed the pr_remove_stdio_default_module branch from 31f2287 to eeef8b5 Compare April 2, 2020 07:48
@fjmolinas
Copy link
Contributor Author

The build is failing for nrf52 boards because of the hack in boards/common/nrf52/Makefile.include:

# The following configuration is dependencies specific
# but they are resolved later
# Hack to know now if 'nordic_softdevice_ble' is used
include $(RIOTBOARD)/$(BOARD)/Makefile.dep

@fjmolinas
Copy link
Contributor Author

The build is failing for nrf52 boards because of the hack in boards/common/nrf52/Makefile.include:

Isn't soft device due for deprecation anyway, I can patch this until deprecation by adding -include $(APPDIR)/Makefile.board.dep to the hack.

# The following configuration is dependencies specific
# but they are resolved later
# Hack to know now if 'nordic_softdevice_ble' is used
-include $(APPDIR)/Makefile.board.dep
include $(RIOTBOARD)/$(BOARD)/Makefile.dep

@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2020
@fjmolinas fjmolinas requested a review from haukepetersen as a code owner April 2, 2020 08:04
@fjmolinas fjmolinas force-pushed the pr_remove_stdio_default_module branch from 943ed2e to 3f0357c Compare April 2, 2020 08:06
@fjmolinas
Copy link
Contributor Author

Any suggestions on where I could define STDIO_MODULES early?

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2020
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. I have one inline comment. Please squash that right in, if you choose to take this comment.

@maribu
Copy link
Member

maribu commented Apr 2, 2020

It would be good to have one more round of Murdock, now that #13349 is merged. They should not have any interactions and everything should still be fine. But if there is one thing I learned in the last few days, it is being careful with assuming what will/won't break the build process in possibly subtle ways.

(If you take the comment, that rebuild would happen any, so I resist toggling the label for now.)

@fjmolinas fjmolinas force-pushed the pr_remove_stdio_default_module branch from efb0902 to 98d20fa Compare April 2, 2020 12:50
nrf52 includes include $(RIOTBOARD)/$(BOARD)/Makefile.dep to know
if `nordic_softdevice_ble` is used, this changes dependency
resolution sinnce -include $(APPDIR)/Makefile.board.dep should
be resolved before.

This can be removed once RIOT-OS#9913 is if `nordic_softdevice` is
deprecated.
@fjmolinas fjmolinas force-pushed the pr_remove_stdio_default_module branch from 98d20fa to 72a2220 Compare April 2, 2020 12:54
@fjmolinas
Copy link
Contributor Author

Addressed both comments and squashed.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK from my side. But let's wait for @aabadie before hitting merge, so that he can have a second look.

@aabadie
Copy link
Contributor

aabadie commented Apr 2, 2020

I'm also OK with this PR. The current position of STDIO_MODULES in stdio.inc.mk is good for me as it's consistent with how stdio modules dependency is managed (in a single place I mean).

Let's go with this one, ACK

@aabadie aabadie merged commit 192c851 into RIOT-OS:master Apr 2, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020
@fjmolinas fjmolinas deleted the pr_remove_stdio_default_module branch July 31, 2020 07:49
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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants