make: Allow for true pseudo-submodules

[miri64]

Contribution description

When providing a pseudo-submodule for a module already using the SUBMODULES mechanism to provide submodules, it is not possible to create a true pseudo-module as submodule (i.e. one without any code on its own), since the build system currently always expects there to be a C file module_submodule.c.

This removes this requirement.

Testing procedure

Build an application with e.g. USEMODULE += core_foobar (since anything starting with core_ is a pseudo-module this implicitly defines a pseudo-module at the moment). Without this PR, the build system will complain

make[2]: *** No rule to make target 'foobar.c', needed by '/home/mlenders/Repositories/RIOT-OS/RIOT/examples/hello-world/bin/native/core/foobar.o'.  Stop.

With this PR this error is does not exist anymore.

Issues/PRs references

None (see #10971 for a use-case)

[miri64]

Ok, I just realized that this causes compilation errors with a minimal application:

include ../Makefile.tests_common

DISABLE_MODULE += auto_init

include $(RIOTBASE)/Makefile.include

int main(void)
{
    return 0;
}

/home/mlenders/Repositories/RIOT-OS/RIOT/tests/core_setup/bin/native/cpu/startup.o: In function `startup':
startup.c:(.text.startup+0x18a): undefined reference to `tty_uart_setup'
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/core_setup/bin/native/core.a(kernel_init.o): In function `idle_thread':
kernel_init.c:(.text.idle_thread+0x18): undefined reference to `pm_set_lowest'
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/core_setup/bin/native/core.a(panic.o): In function `core_panic':
panic.c:(.text.core_panic+0x78): undefined reference to `pm_off'
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/core_setup/bin/native/cpu.a(irq_cpu.o): In function `native_shutdown':
irq_cpu.c:(.text.native_shutdown+0x24): undefined reference to `pm_off'
collect2: error: ld returned 1 exit status

Any idea why?

[cladmi]

The wildcard should be around everything else. The %.c there is the patsubst replacement pattern argument.

It would be equivalent to by default doing SUBMODULE_NOFORCE = 1 handled below.
It would in the current state prevent wrong module name detection which there currently is.

[miri64]

The wildcard should be around everything else.

What do you mean by "everything else"?

The %.c there is the patsubst replacement pattern argument.

True... don't know, what I was thinking there...

It would be equivalent to by default doing SUBMODULE_NOFORCE = 1 handled below.
It would in the current state prevent wrong module name detection which there currently is.

This onl is used by makefiles/periph.mk and seems to me more like a patch on top of the deeper problem that SUBMODULES basically makes it impossible to have true pseudo-submodules.
I don't understand the meaning your last sentence means.

[cladmi]

The wildcard should be around everything else.

What do you mean by "everything else"?

I meant around the value: $(wildcard $(patsubst ...)

It would be equivalent to by default doing SUBMODULE_NOFORCE = 1 handled below.
It would in the current state prevent wrong module name detection which there currently is.

This onl is used by makefiles/periph.mk and seems to me more like a patch on top of the deeper problem that SUBMODULES basically makes it impossible to have true pseudo-submodules.
I don't understand the meaning your last sentence means.

I think, the reason to not allow real pseudomodules by default and so not do the SUBMODULE_NOFORCE behavior was to provide errors on non existing submodule.

USEMODULE=core_not_an_existing_submodule make --no-print-directory -C examples/hello-world/

USEMODULE=core_not_an_existing_submodule make --no-print-directory -C examples/hello-world/
Building application "hello-world" for "native" with MCU "native".

"make" -C /home/harter/work/git/RIOT/boards/native
"make" -C /home/harter/work/git/RIOT/boards/native/drivers
"make" -C /home/harter/work/git/RIOT/core
make[2]: *** No rule to make target 'not_an_existing_submodule.c', needed by '/home/harter/work/git/RIOT/examples/hello-world/bin/native/core/not_an_existing_submodule.o'.  Stop.
/home/harter/work/git/RIOT/Makefile.base:20: recipe for target 'ALL--/home/harter/work/git/RIOT/core' failed
make[1]: *** [ALL--/home/harter/work/git/RIOT/core] Error 2
/home/harter/work/git/RIOT/examples/hello-world/../../Makefile.include:473: recipe for target '/home/harter/work/git/RIOT/examples/hello-world/bin/native/application_hello-world.a' failed
make: *** [/home/harter/work/git/RIOT/examples/hello-world/bin/native/application_hello-world.a] Error 2

So the same way as you have an error when you ask for a non existing module.

USEMODULE=not_an_existing_module make --no-print-directory -C examples/hello-world/

USEMODULE=not_an_existing_module make --no-print-directory -C examples/hello-world/
Building application "hello-world" for "native" with MCU "native".

"make" -C /home/harter/work/git/RIOT/boards/native
"make" -C /home/harter/work/git/RIOT/boards/native/drivers
"make" -C /home/harter/work/git/RIOT/core
"make" -C /home/harter/work/git/RIOT/cpu/native
"make" -C /home/harter/work/git/RIOT/cpu/native/periph
"make" -C /home/harter/work/git/RIOT/cpu/native/vfs
"make" -C /home/harter/work/git/RIOT/drivers
"make" -C /home/harter/work/git/RIOT/drivers/periph_common
"make" -C /home/harter/work/git/RIOT/sys
"make" -C /home/harter/work/git/RIOT/sys/auto_init
gcc: error: /home/harter/work/git/RIOT/examples/hello-world/bin/native/not_an_existing_module.a: No such file or directory
/home/harter/work/git/RIOT/examples/hello-world/../../Makefile.include:470: recipe for target '/home/harter/work/git/RIOT/examples/hello-world/bin/native/hello-world.elf' failed
make: *** [/home/harter/work/git/RIOT/examples/hello-world/bin/native/hello-world.elf] Error 1

Which would not for a periph_ as this does not produce any error

USEMODULE=periph_beer_tap make --no-print-directory -C examples/hello-world/`

I started working on it too for https://github.com/RIOT-OS/RIOT/pull/11781/files#r303438572 and I remembered this PR existed.
It would somehow require listing all the valid module names when using SUBMODULES so the SUBMODULES and the PSEUDOMODULES for that file. (listing the SUBMODULES instead of doing = 1 is for me a good thing).
Or list them all as SUBMODULES directly to make it simpler.
I am not sure how to do it to be consistent with other modules that have pseudo-modules.

[miri64]

The thing is, the whole concept of SUBMODULES blurs the border between proper modules and pseudo-modules somewhat, as those modules are somehow both: They do need code and they do need to be defined in makefiles/pseudomodules.mk (if by wildcard or not is arbitrary for this discussion). I'd rather have a developer define the submodules by naming them explicitly and not just block the whole pseudo-module (i.e. use wildcards in pseudomodules.mk), than this blurred mess, but I am lazy as well :-P (and given how long this one-line change was unattended, I guess I was right to be so ;-)).

You also would get compile errors if a function your sub-module is supposed to implement is not in the binary (i.e. the C-file is missing).

[miri64]

Ping @kaspar030?

[miri64]

Ping @kaspar030 @fjmolinas?

[kaspar030]

Ping @kaspar030 @fjmolinas?

I don't care much. IMO the make based build system should just go. We're losing hundreds of hours on simple things like this.

[miri64]

I don't care much. IMO the make based build system should just go. We're losing hundreds of hours on simple things like this.

If something is interpreted as a pseudo-module (module without code) or not has nothing to do with the build system, but how it is used. If we would go to a different build system, we could have the same problem.

[fjmolinas]

@miri64, I think you still need to address the wildcard comment. Otherwise why not use the same approach that is already done in other places (as suggested by @cladmi)

  # for each $(BASE_MODULE)_<name> in USEMODULE, add <name>.c to SRC
  SRC += $(patsubst $(BASE_MODULE)_%,%.c,$(filter-out $(PSEUDOMODULES),$(filter $(BASE_MODULE)_%,$(USEMODULE))))

[fjmolinas]

@miri64, I think you still need to address the wildcard comment. Otherwise why not use the same approach that is already done in other places (as suggested by @cladmi)

  # for each $(BASE_MODULE)_<name> in USEMODULE, add <name>.c to SRC
  SRC += $(patsubst $(BASE_MODULE)_%,%.c,$(filter-out $(PSEUDOMODULES),$(filter $(BASE_MODULE)_%,$(USEMODULE))))

Or PSUEDO_SUBMODULE

[fjmolinas]

The thing is, the whole concept of SUBMODULES blurs the border between proper modules and pseudo-modules somewhat, as those modules are somehow both: They do need code and they do need to be defined in makefiles/pseudomodules.mk (if by wildcard or not is arbitrary for this discussion). I'd rather have a developer define the submodules by naming them explicitly and not just block the whole pseudo-module (i.e. use wildcards in pseudomodules.mk), than this blurred mess, but I am lazy as well :-P (and given how long this one-line change was unattended, I guess I was right to be so ;-)).

You also would get compile errors if a function your sub-module is supposed to implement is not in the binary (i.e. the C-file is missing).

I'm also in favor of this BTW :)

[miri64]

That wouldn't work... sub-modules are pseudo-modules. That is the whole problem I am trying to solve. I am not sure I understand the usage of wildcard as proposed.

[fjmolinas]

That wouldn't work... sub-modules are pseudo-modules. That is the whole problem I am trying to solve. I am not sure I understand the usage of wildcard as proposed.

It does when using a PSUEDO_SUBMODULE variable :)

[miri64]

I don't really understand how your comments are meant... Probably, because I don't really know that much about this PR anymore either, but the main idea of this PR is basically: make all pseudo-modules who have C-files according to the pattern defined by SUBMODULES sub-modules and keep the rest pseudo-modules.

[fjmolinas]

I am not sure I understand the usage of wildcard as proposed.

I think this is what was meant?

SRC += $(wildcard $(patsubst $(BASE_MODULE)_%,%.c,$(filter $(BASE_MODULE)_%,$(USEMODULE))))```

[miri64]

It does when using a PSUEDO_SUBMODULE variable :)

This variable (neither with or without your typo) does not exist in RIOT master.

[miri64]

SRC += $(wildcard $(patsubst $(BASE_MODULE)_%,%.c,$(filter $(BASE_MODULE)_%,$(USEMODULE))))

Yes, that's what @cladmi's comment says basically. But at the point you use wildcard there, there are no wildcards anymore in the list, so why use it still? And how is it different from the approach in master?

[fjmolinas]

It does when using a PSUEDO_SUBMODULE variable :)

This variable (neither with or without your typo) does not exist in RIOT master.

Sorry I did not understand at first, I tested doing the following https://gist.github.com/fjmolinas/e651bcddecd40c437b07513b53ca2074. But I see that its adding a new mechanism to under-do another so looks weird as well. And sorry If I make other unclear comments, I haven't used this mechanism before, so I might miss some things.

But at the point you use wildcard there, there are no wildcards anymore in the list, so why use it still?

Lets say there are the following c files with a BASEMODULE = foo

• bar1.c
• bar2.c

Currently in master if you do USMODULE+=foo_bar3, SRC will end up as SRC += foo_bar1.c bar2.c bar3.c, but bar3.c doesn't exist so you get an error, if you have SRC += $(wildcard bar1.c bar2.c bar3.c), then it matches all files matching those patterns, and since there is no bar3.c only the first 2 are matched, so you end up with SRC += foo_bar1.c bar2.c.

[miri64]

Sorry I did not understand at first, I tested doing the following https://gist.github.com/fjmolinas/e651bcddecd40c437b07513b53ca2074. But I see that its adding a new mechanism to under-do another so looks weird as well. And sorry If I make other unclear comments, I haven't used this mechanism before, so I might miss somethings.

Huh? You just add that there. So this is not really helpful getting rid of all the submodules from PSEUDOMODULES in a lazy way (core is not the only module doing that).

Currently in master if you do USMODULE+=foo_bar3, SRC will end up as SRC += foo_bar1.c bar2.c bar3.c, but bar3.c doesn't exist so you get an error, if you have SRC += $(wildcar bar1.c bar2.c bar3.c), the it matches all files matching those patterns, and since there is no bar3.c only the first 2 are matched, so you end up with SRC += foo_bar1.c bar2.c.

Mh... that is not what I would expect a function named "wildcard" to do. So it is basically filter with file_exists as condition in this case. Ok then let me do it like that.

[miri64]

Sorry I did not understand at first, I tested doing the following https://gist.github.com/fjmolinas/e651bcddecd40c437b07513b53ca2074. But I see that its adding a new mechanism to under-do another so looks weird as well. And sorry If I make other unclear comments, I haven't used this mechanism before, so I might miss somethings.

Huh? You just add that there. So this is not really helpful getting rid of all the submodules from PSEUDOMODULES in a lazy way (core is not the only module doing that).

I would also prefer if we don't make the SUBMODULES way of defining submodules even more complicated and allowing for more ways to introduce random % constructs ;-).

[fjmolinas]

Mh... that is not what I would expect a function named "wildcard" to do. So it is basically filter with file_exists as condition in this case.

"This string, used anywhere in a makefile, is replaced by a space-separated list of names of existing files that match one of the given file name patterns."

I agree with it not being that intuitive, but I think its whats usually done to check if a file exists in make.

[miri64]

OK, it worked as you described so I applied the patch accordingly :-)

[fjmolinas]

The thing is, the whole concept of SUBMODULES blurs the border between proper modules and pseudo-modules somewhat, as those modules are somehow both: They do need code and they do need to be defined in makefiles/pseudomodules.mk (if by wildcard or not is arbitrary for this discussion).

In my understanding PSEUDOMODULES are not modules with no code, they are modules with no .a file, so they are not static libraries. A PSEUDOMODULE may or may not have a c file associated and therefore generate an object file that is contained in a REALMODULE. SUBMODULES declare just that, so they are still PSEUDOMODULES. I see the same kind of handling going on for skald_ibeacon, this module goes exactly though the same handling as a SUBMODULE, but implemented manually.

Currently if a module is implicitly declared as a PSEUDOMODULE through wildcards (periph_% ), there is no error handling if that module doesn't actually exists, either because the c file is missing, or because of a typo(e.g.: periph_urat or periph_foo).

The only PSEUDOMODULES that had this kind of handling where SUBMODULES. We loose this with this PR, but IMO that is fine as a first approach. The right fix for this error handling is just to list all the PSEUDOMODULES instead of the wildcards. Again, as was said above, the best thing would be listing all SUBMODULES and then SUBMODULES handling could just become:

## submodules
ifneq (,$(SUBMODULES))
  # don't use *.c as SRC if SRC is empty (e.g., no module selected)
  NO_AUTO_SRC := 1

  # allow different submodule basename (e.g., MODULE=cpu_periph_common, but match just periph_%)
  BASE_MODULE ?= $(MODULE)

  # for each $(BASE_MODULE)_<name> in USEMODULE, add <name>.c to SRC
  SRC += $(patsubst $(BASE_MODULE)_%,%.c,$(filter $(SUBMODULES),$(USEMODULE)))

  # don't fail if a selected *.c file does not exist
  ifeq (1, $(SUBMODULES_NOFORCE))
    SRC := $(filter $(SRC), $(wildcard *.c))
  endif
endif

But the lazy way works as well, and as I'm not willing to address this right now either, so I'm fine with the PR as is, I think the definition of PSEUDOMODULES in our doc should be changed tough as it is not accurate.

With this change SUBMODULES_NOFORCE := 1 does not make sense anymore though, and I think should be removed, If you agree @miri64 please trigger the ci!

[miri64]

With this change SUBMODULES_NOFORCE := 1 does not make sense anymore though, and I think should be removed, If you agree @miri64 please trigger the ci!

I agree, from what the comment in Makefile.base states, this basically is just what I am doing here, stated for submodules explicitly. This should be done in a follow-up however IMHO.

[miri64]

Squashed and triggered CI.

[fjmolinas]

ACK.