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

make: use of immediate value of variables before they have their final value #8913

Open
1 of 6 tasks
cladmi opened this issue Apr 10, 2018 · 2 comments
Open
1 of 6 tasks
Assignees
Labels
Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: tracking The issue tracks and organizes the sub-tasks of a larger effort

Comments

@cladmi
Copy link
Contributor

cladmi commented Apr 10, 2018

Description

Some makefiles use variables with their immediate value before they get the final one.
It's not problematic for build flags but for variables containing files/path.

The root of the problem is that the makefiles do not follow a pattern of defining all variables first/including sub makefiles, and then only after define the rules using the variables as file target or file dependency.

This issue will be the central one to reference sub pull requests to fix this uses.

Debugging

The problem can be seen by using $(info debug VARIABLE $(VARIABLE)) before usage.

TODO list:

  • USEMODULE USEPKG FEATURES_REQUIRED in board/cpu Makefile.include
  • Use of PKG_BUILDDIR
    • ccn-lite
    • relic
    • ... ?
  • BASELIBS in Makefile.include
board/cpu Makefile.include

Some boards and cpus Makefile.include have different behaviors depending on the modules.

git grep -e USEPKG -e USEMODULE -e FEATURES boards/ cpu/ | grep Makefile.include | grep -e ifeq -e ifneq
boards/common/nrf52xxxdk/Makefile.include:ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))
boards/native/Makefile.include:ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter pthread, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_sw_timer, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_sdk, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_gdbstub, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_gdb, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_spiffs, $(USEMODULE)))
cpu/esp8266/Makefile.include:ifneq (, $(filter esp_sdk, $(USEMODULE)))
cpu/esp8266/Makefile.include:    ifneq (, $(filter esp_now, $(USEMODULE)))
cpu/stm32_common/Makefile.include:ifneq (,$(filter periph_flashpage periph_eeprom,$(FEATURES_REQUIRED)))

However, modules dependencies are only resolved after including these files.

Some even include Makefile.dep to fix issues (other for no reason)

https://github.com/RIOT-OS/RIOT/blob/master/boards/common/nrf52xxxdk/Makefile.include#L21-L27

git grep Makefile.dep boards | grep 'Makefile.include:'
boards/common/arduino-atmega/Makefile.include:include $(RIOTBOARD)/common/arduino-atmega/Makefile.dep
boards/common/arduino-mkr/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/common/nrf52xxxdk/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/common/nrf52xxxdk/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/feather-m0/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/sensebox_samd21/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/sodaq-explorer/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/sodaq-one/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep

Dependencies need to be processed before including any other Makefile.include.

Also referenced in #9811

pkg/ PKG_BUILDDIR

Some packages makefile use $(PKG_BUILDDIR) with its immediate value when defining targets before it is defined by including pkg/pkg.mk.

It is mitigated because then they are treated as non file targets, and because the command repeats the variable and does not use the automatic make variables like $@. #8509 (comment)

Done

Makefile.include

In Makefile.include, all uses of $(BASELIBS), except when defining _LINK, are using the immediate > value of $(BASELIBS) which does not have its final value as this is done by including modules.inc.mk 200 lines below.

All the uses are currently solved by hidden "tricks"

  • $(ELFFILE): $(BASELIBS) which works because there is a $(ELFFILE): FORCE rule, so it will be rebuilt anyway, and _LINK is using the deferred value of BASELIBS.
  • link: $(BASELIBS) when in the RIOTNOLINK case, where BASELIBS is missing $(USEPKG:%=$(BINDIR)/%.a) files, but as $(BINDIR)/$(APPLICATION_MODULE).a has a dependency to $(USEPKG:%=$(BINDIR)/%.a) they are still build anyway.
  • The clean before building trick for parallel execution is using $(BASELIBS) also without the $(USEPKG:%=$(BINDIR)/%.a) files. But as there is the RIOTPKG/%/Makefile.include target also depending on clean, the packages are built after clean.
@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 10, 2018
@cladmi cladmi self-assigned this Apr 10, 2018
@cladmi cladmi added the Type: tracking The issue tracks and organizes the sub-tasks of a larger effort label Sep 28, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Nov 12, 2018

Includes is also evaluated directly

INCLUDES := $(NEWLIB_INCLUDES) $(INCLUDES)
even if CFLAGS_FPU can be changed later by pkg/nordic_softdevice_ble which would change the INCLUDES when using llvm
INCLUDES += $(GCC_C_INCLUDES)

In that case it is not breaking as llvm is said incompatible but it shows an issue with these immediate evaluations.

@cladmi
Copy link
Contributor Author

cladmi commented Dec 4, 2018

This one is also related, where a file that should be put in BUILDDEPS is declared by a makefile included after using BUILDDEPS: #10492 (comment)

It goes with many definitions orders in our Makefiles that should be addressed in a deterministic way at some point.

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: tracking The issue tracks and organizes the sub-tasks of a larger effort
Projects
None yet
Development

No branches or pull requests

2 participants