Skip to content

build-system: auto-locate modules by name#16465

Closed
maribu wants to merge 4 commits intoRIOT-OS:masterfrom
maribu:build_system
Closed

build-system: auto-locate modules by name#16465
maribu wants to merge 4 commits intoRIOT-OS:masterfrom
maribu:build_system

Conversation

@maribu
Copy link
Member

@maribu maribu commented May 10, 2021

Contribution description

This PR introduces the MODULE_DIRS variable that works just like EXTERNAL_MODULE_DIRS, but it is intended for internal modules. The major difference is that external modules can have the same name as an internal module, in which case the external module is used instead
of the internal one. This allows to provide drop-in replacements of internal modules, which can be useful during development e.g. to quickly benchmark different implementations.

Testing procedure

All applications for all boards should still build and yield identical binaries.

Issues/PRs references

Follow up of #16104

Matching folder and module names make live easier
@maribu maribu requested review from benpicco and fjmolinas May 10, 2021 15:58
@maribu maribu added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances 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 labels May 10, 2021
This commit introduces the MODULE_DIRS variable that works just like
EXTERNAL_MODULE_DIRS, but it is intended for internal modules. The
major difference is that external modules can have the same name as
an internal module, in which case the external module is used instead
of the internal one. This allows to provide drop-in replacements of
internal modules, which can be useful during development e.g. to
quickly benchmark different implementations.
@leandrolanzieri leandrolanzieri added the Area: build system Area: Build system label May 11, 2021
@fjmolinas
Copy link
Contributor

This only works with modules in a directory matching the module name right?

@maribu
Copy link
Member Author

maribu commented May 21, 2021

This only works with modules in a directory matching the module name right?

Exactly. For those modules, one still has to manually add the dirs to the DIRS variable. But e.g. for all modules in drivers, this already works.

@fjmolinas
Copy link
Contributor

This only works with modules in a directory matching the module name right?

Exactly. For those modules, one still has to manually add the dirs to the DIRS variable. But e.g. for all modules in drivers, this already works.

Can this be documented?

@fjmolinas
Copy link
Contributor

This only works with modules in a directory matching the module name right?

Exactly. For those modules, one still has to manually add the dirs to the DIRS variable. But e.g. for all modules in drivers, this already works.

Can this be documented?

BTW, couldn't we do the same for external modules as well?

@maribu
Copy link
Member Author

maribu commented May 21, 2021

This only works with modules in a directory matching the module name right?

Exactly. For those modules, one still has to manually add the dirs to the DIRS variable. But e.g. for all modules in drivers, this already works.

Can this be documented?

BTW, couldn't we do the same for external modules as well?

Yes, but at least one external module has to be found without, which could extend the DIRS variable as needed. (Or the applications Makefile could do so.)

I intend btw. to extend the mechanism to allow assigning prefixes to search dirs. E.g sys/gnrc should find module gnrc_%, where % is the folder name. With this, we might be able to automatically locate all modules eventually.

endif
else
# ztimer_xtimer_compat is used, all of *xtimer's API will be mapped on ztimer.*
NO_AUTOLOCATE += xtimer
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this be affected by the order of inclusion?

Copy link
Member Author

Choose a reason for hiding this comment

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

The location of modules is done fully again at each recursion step. If xtimer would be auto-located in a transient state, it would be dropped later on. But some care should be taken so that an Makefile.dep doesn't do anything xtimer related when ztimer_xtimer_compat is also used. But since there is no Makefile.dep in sys/xtimer and in sys/Makefile.dep (above code) the correct guards are in place, this should just work.

A bit of a foot gun, but choosing different implementation options for the same API is just not properly modeled in the build system, so some friction is expected.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss
Copy link
Contributor

Do you think this should be included in this coming release or should we push it?

@benpicco
Copy link
Contributor

Needs a rebase (feel free to squash while you're at it)

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

One thing that is not clear to me is if this PR wants to imply that module to have the same name as the directory?, Module names that mix the directories into it are a common practice in the build system, e.g.: foo/bar and MODULE = foo_bar. It is widely used in sys so I don't see why do it for sys/bus and not other modules. I won't necessarily say its an argument one way or another but I do find it cleaner to have MODULEs grouped under a directory. E.g.

sys
│
└───ssp
│   │   Makefile 
│   └───subfolder1
└───shell
│   │
│   └───commands
│       │  Makefile
│       │   ...
└───test_utils
│   │
│   └───interactive_sync
│   │   │  Makefile (MODULE=test_utils_interactive_sync)
│   │   │   ...
│   └───result_output
│       │  Makefile (MODULE=test_utils_result_output)
│       │   ...
│       └───check
│       │  │  Makefile (MODULE=test_utils_result_output_check)
│       │  │  ....
│       └───json
│             │  Makefile (MODULE=test_utils_result_output_json)
│             │  ....

It seems to me that if the same change as done for sys_bus is done here things would get messy, and so what is the guideline here? If the name is long and messy then split into subdirs? if not mix them all up together? There is a question here on how we want to structure our code, what will be our conventions, etc.. EXTERNAL_MODULES now have a hard requirement on MODULE names that are not enforced for the internal module.

So what is for me not clear from the PR description is:

  • what is the goal of the PR: is it being able to provide drop-in replacements or for conveniently include modules?
  • this PR only works for modules that can be auto-located this leads to different behaviour depending on the module naming which is not documented
  • what kind of tree structure or we envisioning, what shall be our conventions for naming modules, structuring directories, etc...

@@ -0,0 +1,11 @@
# Only locate non-pseudo-modules
MODS2LOCATE := $(filter-out $(PSEUDOMODULES) $(NO_AUTOLOCATE),\
Copy link
Contributor

Choose a reason for hiding this comment

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

What is mods2?

Copy link
Member Author

Choose a reason for hiding this comment

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

modules to locate ;-)

@maribu
Copy link
Member Author

maribu commented Oct 20, 2021

Module names that mix the directories into it are a common practice in the build system, e.g.: foo/bar and MODULE = foo_bar.

That should actually be possible to still auto-detect, as long as the pattern is consistent. I have an idea how this might be possible to implement in make without complexity getting too high.

@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@OlegHahm OlegHahm added the State: waiting for author State: Action by the author of the PR is required label Mar 9, 2022
@OlegHahm
Copy link
Member

OlegHahm commented Mar 9, 2022

@maribu, ping!

@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@maribu
Copy link
Member Author

maribu commented Jan 2, 2023

Let's close this. It is so outdated that one would need to start from scratch anyway. And maybe after the KConfig migration we can switch to a better build system instead :)

@maribu maribu closed this Jan 2, 2023
@maribu maribu deleted the build_system branch January 2, 2023 20:26
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 State: waiting for author State: Action by the author of the PR is required Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants