search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.85k stars 1.97k forks source link

Build dependencies - processing order issues #9913

Open cladmi opened 5 years ago

cladmi commented 5 years ago

There currently are concurrency issues in the order dependencies are defined and used, and cpu dependencies cannot be easily parsed.

I plan to address this by moving the processing of Makefile.dep before including $(BOARD)/Makefile.include

Dependencies resolution uses the following variables

Steps

Prerequisites cleanup

And even future improvement to remove multiple duplication between board/board_common cpu/cpu_common path:

Explanation

To summarize, the main Makefile.include does regarding modules: (a more detailed version is available here https://gist.github.com/cladmi/1c5af63c753635b7cb173cf8311d0b5b)

include $(RIOTBOARD)/$(BOARD)/Makefile.features
    # Define FEATURES_PROVIDED
    # Include with hard written path CPU/Makefile.features

include $(RIOTBOARD)/$(BOARD)/Makefile.include
    # Defines CPU
    # Defines USEMODULE
    # Have specific behavior depending on USEMODULE/USEPKG
include $(RIOTBOARD)/$(CPU)/Makefile.include
    # Define USEMODULE
    # Have specific behavior depending on USEMODULE/USEPKG
    # Define module dependencies

include Makefile.dep
    # Resolve USEMODULE/USEPKG/FEATURES_USED using:
    # * USEMODULE
    # * USEPKG
    # * FEATURES_
    # * BOARD
    # * CPU
    # * CPU_FAM
    # * CPU_ARCH 

include $(RIOTBASE)/sys/Makefile.include
include $(RIOTBASE)/drivers/Makefile.include
-include $(USEPKG:%=$(RIOTPKG)/%/Makefile.include)
include $(RIOTMAKE)/bindist.inc.mk
    # These also add things to USEMODULE

The main Makefile.dep only includes $(BOARD)/Makefile.dep which may sometime also end up including hardwritten_cpu_name/Makefile.dep.

In the same time makefiles/info-global.inc.mk does: 

include $(RIOTBOARD)/$(BOARD)/Makefile.features
include $(RIOTBASE)/Makefile.dep

And so never takes into account any of the dependencies defined in $(BOARD)/Makefile.include or any other Makeflie.include files.

cladmi commented 5 years ago

To not re-add another file for nothing, I thought about putting CPU in Makefile.features instead.

It would contain "everything that needs to be defined before being able to process dependencies".

miri64 commented 5 years ago

I'm unclear on what the problem is with CPU defined in Makefile.include...

cladmi commented 5 years ago

Changing it is a mix of somehow required, it makes sense, do not repeat the same information 3 times, and many things in our makefiles have hard ordering requirements so leads to the proposed solution.

One hint that moving CPU earlier than Makefile.include is a good idea, is that right now the cpu name is already hardcoded in $(BOARD)/Makefile.features (or a common/Makefile.features): https://github.com/RIOT-OS/RIOT/blob/1e6009c582ab5433db1850ecfec04c1566b8f2e6/boards/samr21-xpro/Makefile.features#L15

Main issue

The first main issue that is causing me problems, is that board and CPUs need to declare dependencies, and some of their internal modules too. However, this is right now often done in $(BOARD)/Makefile.include and $(CPU)/Makefile.include but the dependencies are resolved later in Makefile.dep. So they use USEMODULE before it is defined.

One example is here: https://github.com/RIOT-OS/RIOT/blob/1e6009c582ab5433db1850ecfec04c1566b8f2e6/boards/common/nrf52xxxdk/Makefile.include#L21-L27 Another one here: https://github.com/RIOT-OS/RIOT/blob/1e6009c582ab5433db1850ecfec04c1566b8f2e6/cpu/stm32_common/Makefile.include#L13-L15 This one is even using FEATURES_REQUIRED to work because of course USEMODULE is not defined yet. But it fails if the main application only declares a dependency on a module that will add the FEATURES_REQUIRED as does the test in https://github.com/RIOT-OS/RIOT/pull/9892 and the OTA task force branch.

Proposed solution

So for this I want to move the processing of dependencies, before parsing all modules Makefile.include files (boards, cpu, sys, drivers, pkgs).

Constraint

Moving the dependency handling before Makefile.include means to somehow do:

include $(RIOTBOARD)/$(BOARD)/Makefile.dep
include $(RIOTBOARD)/$(CPU)/Makefile.dep

...

include $(RIOTBOARD)/$(BOARD)/Makefile.include

Which would require having CPU defined.

I say somehow, because it could be $(BOARD)/Makefile.dep or one of its $(RIOTBOARD)/common/one_on_the_common/Makefile.dep that does it like we are doing for Makefile.features and directly includes the cpu_name/Makefile.dep

But I am not going to push toward having 3 times the cpu name defined between Makefile.features, Makefile.dep, Makefile.include.

Then on the somehow required:

The CPU variable is used in Makefile.dep

However, it is still concept a dependency that the board depends/uses on this cpu so makes sense to be explicit when resolving Makefile.dep.

cladmi commented 5 years ago

We can also see this problem with the esp32 board that need to define CFLAGS depending on included modules. https://github.com/RIOT-OS/RIOT/pull/9426

They could in theory be defined in inline if statement, but it is not a pattern used in other boards, and would be solved by re-ordering the processing.

cladmi commented 5 years ago

Found another case showing the issue:

mips-malta is said supported for tests/pipe

make --no-print-directory -C tests/pipe/ info-boards-supported | grep mips-malta
acd52832 airfy-beacon arduino-due arduino-duemilanove arduino-mega2560 arduino-mkr1000 arduino-mkrfox1200 arduino-mkrzero arduino-uno arduino-zero avsextrem b-l072z-lrwan1 b-l475e-iot01a blackpill bluepill calliope-mini cc2538dk cc2650-launchpad cc2650stk chronos ek-lm4f120xl esp32-mh-et-live-minikit esp32-olimex-evb esp32-wemos-lolin-d32-pro esp32-wroom-32 esp32-wrover-kit esp8266-esp-12x esp8266-olimex-mod esp8266-sparkfun-thing f4vi1 feather-m0 firefly fox frdm-k22f frdm-k64f frdm-kw41z hifive1 ikea-tradfri iotlab-a8-m3 iotlab-m3 limifrog-v1 lobaro-lorabox maple-mini mbed_lpc1768 microbit mips-malta msb-430 msb-430h msba2 msbiot mulle native nrf51dongle nrf52840dk nrf52dk nrf6310 nucleo-f030r8 nucleo-f031k6 nucleo-f042k6 nucleo-f070rb nucleo-f072rb nucleo-f091rc nucleo-f103rb nucleo-f207zg nucleo-f302r8 nucleo-f303k8 nucleo-f303re
nucleo-f303ze nucleo-f334r8 nucleo-f401re nucleo-f410rb nucleo-f411re nucleo-f412zg nucleo-f413zh nucleo-f429zi nucleo-f446re nucleo-f446ze nucleo-f722ze nucleo-f746zg nucleo-f767zi nucleo-l031k6 nucleo-l053r8 nucleo-l073rz nucleo-l152re nucleo-l432kc nucleo-l433rc nucleo-l452re nucleo-l476rg nucleo-l496zg nz32-sc151 opencm904 openmote-b openmote-cc2538 pba-d-01-kw2x pic32-clicker pic32-wifire remote-pa remote-reva remote-revb ruuvitag samd21-xpro saml21-xpro samr21-xpro samr30-xpro seeeduino_arch-pro sensebox_samd21 slstk3401a slstk3402a sltb001a slwstk6000b slwstk6220a sodaq-autonomo sodaq-explorer sodaq-one spark-core stk3600 stk3700 stm32f0discovery stm32f3discovery stm32f429i-disc1 stm32f4discovery stm32f769i-disco stm32l476g-disco teensy31 telosb thingy52 ublox-c030-u201 udoo waspmote-pro wsn430-v1_3b wsn430-v1_4 yunjia-nrf51822 z1

But when compiling we get

make --no-print-directory -C tests/pipe/ BOARD=mips-malta
/home/harter/work/git/RIOT/makefiles/toolchain/gnu.inc.mk:18: objcopy not found. Hex file will not be created.
There are unsatisfied feature requirements: periph_uart

EXPECT ERRORS!

Because mips does USEMODULE += newlib in 

cpu/mips32r2_common/Makefile.include:export USEMODULE += newlib
cladmi commented 5 years ago

@MrKevinWeiss The issue I talk to you about with the dependencies processing

fjmolinas commented 5 years ago

@cladmi Regarding comments in #11424 the pointed out solution was to define all CPU_FAM, CPU_ARACH, etc.. variables in Makefile.features.

I think before doing this we need uniform the definitions of CPUFAM for all cpu that already have some form of this concept. To be specific I'm talking about Kinetis where there is KINETIS(Family, subfamily and series) and a CPU_FAMILY (used only for specifying the openocd script).

Proposed Solution

fjmolinas commented 5 years ago

Reuse Makefile.features to define CPU

But I am not going to push toward having 3 times the cpu name defined between Makefile.features, Makefile.dep, Makefile.include.

@cladmi There is probably something obvious I'm missing but why couldn't CPU definitions be moved to Makefile.features instead of re-defining CPU in this file?

cladmi commented 5 years ago

Reuse Makefile.features to define CPU

But I am not going to push toward having 3 times the cpu name defined between Makefile.features, Makefile.dep, Makefile.include.

@cladmi There is probably something obvious I'm missing but why couldn't CPU definitions be moved to Makefile.features instead of re-defining CPU in this file?

I was referring to the alternative if not moving it which would result in the same thing as this type of lines https://github.com/RIOT-OS/RIOT/blob/c7e26377ea5752ff6139ea96c043b60e03246b2b/boards/iotlab-m3/Makefile.features#L5 where currently the cpu value is "re-defined" without using the CPU variable. So somehow the value is currently duplicated.

I was questioning if it should go in Makefile.features or if it could be another file. My reasoning is that it could be a move toward defining the hierarchy for defining a BSP with all the board/common/cpu/arch but should be another task even if it duplicates moving them now that I have more feedback.

I think before doing this we need uniform the definitions of CPUFAM for all cpu that already have some form of this concept. To be specific I'm talking about Kinetis where there is KINETIS(Family, subfamily and series) and a CPU_FAMILY (used only for specifying the openocd script).

I would prefer do it in an independent task as it has its own specific requirements. But it can indeed be done before if it works.

Re-ordering is "only" solving the dependency order issues and is a stupid to do change. Where changing the names questions the definition of the names and need to look in all cpus.

We had a discussion about it with @kYc0o and I think from what I remember:

But not all variables are always defined so there are inconsistencies.

fjmolinas commented 5 years ago

I was questioning if it should go in Makefile.features or if it could be another file.

I think at least semantically it makes sense, The board provides as a feature many peripherals and also a cpu which itself can provide more features.

Re-ordering is "only" solving the dependency order issues and is a stupid to do change.

@cladmi correct me if I'm wrong but the required tasks would be:

Have you gotten enough feedback so we can push forward these changes then?

cladmi commented 5 years ago

Required tasks are indeed what you say.

I did not notice that CPU_FAM/CPU_FAMILY was in cpu and not in boards in general.

For CPU_FAM I see these dependencies:

I have a wip branch where I already moved CPU to the board/Makeflie.features but not the CPU_MODEL as I realized it too late https://github.com/cladmi/RIOT/commits/pr/make/cpu/features it showed some dependencies I need to PR before.

Before this, I need to modify the handling in makefiles/info-global.inc.mk to correctly restore all variables before parsing the next board. I have a WIP commit for this one only. It also shows issues with the current state https://github.com/cladmi/RIOT/commits/pr/makefiles/info-global/check_variables

I would however not start doing pull requests for the invasive ones before the release because of time and if a backport is needed.

fjmolinas commented 5 years ago

The samr21-xpro/saml11-xpro CPU_FAM should be moved as it is currently defined in boards.

@cladmi The inconsistency about where CPU_FAM is defined for samr21-xpro/saml11-xpro come from the fact that it groups them as saml1x and there are specific defines for saml11 and saml10.

cladmi commented 5 years ago

@fjmolinas As they can be deduced from CPU_MODEL so can be moved to CPU too.

I found another one definition that bothers me:

https://github.com/RIOT-OS/RIOT/blob/4c3e49ee40ecb25c5947ab28b17eb010d6171e3f/makefiles/arch/cortexm.inc.mk#L44

But this file is problematic anyway as it includes cortexm_common/Makefile.include instead of being the other way around.

Currently, files in cpu including cortexm_common/Makefile.features are not the same as the ones including arch/cortexm.inc.mk (in term of in which directory it is).

git grep cortexm_common/Makefile.features --
cpu/cc2538/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/cc26x0/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/efm32/Makefile.features:include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/ezr32wg/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/kinetis/Makefile.features:include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/lm4f120/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/lpc1768/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/nrf5x_common/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/sam0_common/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/sam3/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
cpu/stm32_common/Makefile.features:-include $(RIOTCPU)/cortexm_common/Makefile.features
git grep arch/cortexm.inc.mk --
cpu/cc2538/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/cc26x0/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/efm32/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/ezr32wg/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/kinetis/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/lm4f120/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/lpc1768/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/nrf51/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/nrf52/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/sam3/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/samd21/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/saml1x/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/saml21/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f0/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f1/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f2/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f3/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f4/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32f7/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32l0/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32l1/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk
cpu/stm32l4/Makefile.include:include $(RIOTMAKE)/arch/cortexm.inc.mk

I will provide a pull request for the moving this afternoon. It will still need to be split and maybe fix some case like the arch/cortexm.inc.mk case but I have something with self tests at least.

cladmi commented 5 years ago

Another non standard one with a non namespaced BOARD_MODULE configuration. I do not know what it is exactly and which concept it should represent in RIOT.

It also gets the CPU_MODEL using grep and sets JLINK_DEVICE in the same pass https://github.com/RIOT-OS/RIOT/blob/4c3e49ee40ecb25c5947ab28b17eb010d6171e3f/boards/slwstk6000b/Makefile.include#L1-L8

So this handling should be modified. It is the same handling as in the efm32 cpu.

cladmi commented 5 years ago

For CPU_MODEL defined in arch/cortexm.inc.mk currently only the boards based on cpu/lpc1768 use the default value from that file. So I could move the definition to cpu/cortexm_common/Makefile.features or to the cpu/lpc1768 or always define this fallback as other boards already do the same. Like the esp, the hifive1.

I will do other changes first and see later.

cladmi commented 5 years ago

I put a tracking PR in https://github.com/RIOT-OS/RIOT/pull/11477 for moving CPU/CPU_MODEL.

cladmi commented 5 years ago

@kaspar030 How do you propose to solve the issues if not with #11478 ?

kaspar030 commented 5 years ago

@kaspar030 How do you propose to solve the issues if not with #11478 ?

11478 is not completely off the table.

There currently are concurrency issues in the order dependencies are defined and used, and cpu dependencies cannot be easily parsed.

Can you elaborate on this? How to fix "it" is has a lot of explanations, but the underlying problem has not.

Is the initial issue actually "concurrency"? Isn't the problem that the board/cpu Makefile.include change the state that Makefile.dep should handle, like, "if CPU=foo then USEMODULE+=bar", which only works when CPU is set, but not in "make info-boards-supported"?

cladmi commented 5 years ago

The explanation is a bit messy and dissolved in both the first post and following comments as there are many problems together.

However, there are concurrency issues even without info-boards-supported.

I can link the code reference for each issue if wanted.

Between features and include

Between include and deps

More CPU specific things:

info-boards-supported

Having a separate way of parsing dependencies also shows inconsistencies:

My goal is to be able to parse the dependencies the same way in both cases, so do it without Makefile.include.Makefile.include.

fjmolinas commented 5 years ago

Sorry, mistakenly pressed the button. Reverted the Software Updates automation too. Really sorry.

fjmolinas commented 5 years ago

Any alternatives for #11478 ?

gschorcht commented 4 years ago

As part of the code deduplication for ESP platforms, I'm reworking the Makefiles. I still have the problem that some compiler/linker settings in Makefile.include like CFLAGS, INCLUDES, LINKFLAGS, ... depend on the enabled modules. However, the module dependencies are defined in Makefile.dep, which is included after Makefile.include.

So my question is whether there is any work or progress to move the inclusion of Makefile.dep before Makefile.include?

aabadie commented 4 years ago

Is any work or progress to move the inclusion of Makefile.dep before Makefile.include?

Thanks for reviving this issue, which is still valid. I just had a look at it and there is still some work to be done, and in many different places. That will require several PRs before Makefile.dep can be included before Makefile.include.

aabadie commented 4 years ago

@gschorcht, if you are interested, I have a branch where I started to move cpu dependencies in dedicated Makefile.dep. I'm hesitating to PR it. If you think it's worth, I'll do.

I could already see problems if we include Makefile.dep before board/cpus Makefile.include: some cpu add modules based on variables defined in board/cpu level Makefile.include (for example pic32, esp32). There's also the kinetis-info.mk that should be split I think (maybe @fjmolinas has some ideas here).

I haven't looked at esp.

gschorcht commented 4 years ago

The ESP32 wont be a problem. For ESPs, some variables are defined dependent on some modules but not vise versa. Therefore, some module dependencies have to be defined at the very beginning of Makefile.include.

But, once Makefile.dep is included before Makefile.include, it will become possible to move all module dependency definitions to Makefile.dep and all dependent variable definitions to Makefile.include.

I'm hesitating to PR it. If you think it's worth, I'll do.

I think it is worth. It would be further step to cleanup the makefiles.

gschorcht commented 4 years ago

@aabadie If you'd like, I can contribute the ESP changes to your branch.

aabadie commented 4 years ago

If you'd like, I can contribute the ESP changes to your branch.

My branch is touching almost all CPUs so I think it will be difficult to review it and get it merged fast. Some reviewers may also ask to split it. I would prefer you open a separate PR for esp.

gschorcht commented 4 years ago

OK, I see. It seems like a chicken-and-egg problem :worried: It's certainly not possible to split the PRs without affecting the compilability for some CPUs. On the other hand, I can't provide the changes as separate PRs until the include order is changed without affecting the compilability for the EPSs.

aabadie commented 4 years ago

I think it is worth

Ok, then it's here: #12898

For ESP, you can still move forward to an intermediate state where non conditional dependencies could be moved to Makefile.dep.

aabadie commented 4 years ago

@fjmolinas @leandrolanzieri, what is the status of this issue ? Are all cpu dependencies addressed ?

fjmolinas commented 4 years ago

@fjmolinas @leandrolanzieri, what is the status of this issue ? Are all cpu dependencies addressed ?

I think we are at a point where we could test changing the inclusion order, other than sketch_module I'm not sure what is in the way. Would be worth launching debugdependencies on a PR that changes the inclusion order.

fjmolinas commented 4 years ago

I believe:

are done, can you think of something that is missing @aabadie @leandrolanzieri?