buildsystem: add EXTERNAL_PKG_DIRS functionality

[NikLeberg]

Contribution description

The current buildsystem only allows packages residing inside the RIOT tree. For modules this is handled differently, those can be added from external directories with the EXTERNAL_MUDULE_DIRS variable. With this pull request I add the same mechanism of a EXTERNAL_PKG_DIRS variable for packages outside of the RIOT tree. This mechanism can be used to separate RIOT and user supplied packages without cluttering the RIOT tree. Ultimately packages should be added to the RIOT upstream with a pull request. But with this mechanism one can develop more easily.

This is a draft. My goal is to get early feedback.
@fjmolinas Provided some feedback (thanks!), which resulted in splitting of a part of these changes into pr #17551 that was sucessfully merged into master.

ToDo:

• Documentation
• Test
• Kconfig

Testing procedure

I've ran make static-test with success.
For precise test case similar to tests/external_module_dirs I'll need a bit of guidance. I've managed to create a test based on riotgen test --interactive and the mentioned similar testcase.
Interested what the CI has to say about this...
In my own project riot-tdd I'm using this functionality with no problems.

Issues/PRs references

resolves #15309
depends on pr #17551

[maribu]

From a quick look this looks very nice and consistent to how external modules are handled. Let's see if the CI likes this.

[benpicco]

looks like CI does not like this

[fjmolinas]

Currently, EXTERNAL_MODULES are handled separately then internal MODULES. This PR takes a similar approach but is as well doing other things:

• replacing current package dependency inclusion
• forcing internal packages to directories matching module name

Those two need more testing and IMO would be better be handled in individual PR's. I think it would be best to first add this functionality without touching how internal packages are handled.

Other than that it would be good to be sure that this works with docker as well.

[NikLeberg]

I see. Thanks @benpicco and @fjmolinas for your comments.

I'll take a closer look and try to do it more "the EXTERNAL_MODULES - way". The two other things you mentioned @fjmolinas are not intended (replacing dependency inclusion and forcing directory name). Seems like I was lucky to not run into those problems with native and board esp8266.

Is there a way to run the checks / builds the CI / Murdock does locally?

[NikLeberg]

I'm not entirely happy with how I implemented it currently. I want to separate the make targets but also dont want to change too much like the first attempt.
But either way, I wanted to see how the CI likes this approach. (Seems like it does?)

I know that it currently builds the external packages multiple times (second time calls make, but it is already up to date).

@fjmolinas do you have any comments about this approach?

[fjmolinas]

Hi @NikLeberg I'm sorry for taking so long for getting back to you, some paper work and vacation got in the way. I took a second closer look at this. Regarding:

replacing current package dependency inclusion
forcing internal packages to directories matching module name

I now realize that I was wrong, this was already implicitly the case since rules where already doing something like @for i in $(USEPKG) ; do "$(MAKE)" -C $(RIOTPKG)/$$i distclean ; done.

But I still think it's better to split the change of using PKG_PATHS and adding the external package directory (which is a very nice feature).

What do you think of opening a PR that only does the PKG_PATHS change, I'll be more reactive this time around and we can quickly run tests for that. And once that one is merged we add the external pkg directory part.

[fjmolinas]

This one now needs a rebase!

[fjmolinas]

@leandrolanzieri how should we handle Kconfig for this one? Something like we did for EXTERNAL_MODULES?

[NikLeberg]

@fjmolinas I rebased and applied the relevant commits. Kconfig will fail in CI as it is not yet implemented, so to not waste Murdock time, could you remove the CI: ready for build label?
I'll work on this next week. Until then, is there a general overview of how Kconfig works?

[leandrolanzieri]

@leandrolanzieri how should we handle Kconfig for this one? Something like we did for EXTERNAL_MODULES?

I think that approach worked pretty well, I don't see why not take it also for this.

[NikLeberg]

@fjmolinas I found a bit of time anyway and played around with Kconfig. It seems that you also were the one that implemented it for the external modules with pr #16053 perfect! 😀

The way the external modules are handled in Kconfig confuses me a bit. It uses the make variabe EXTERNAL_MODULE_PATHS that is only ever set in dependency_resolution.inc.mk. But that dependency resolution isn't run when Kconfig is enabled, so from where is this variable set?

Anyway with the lastest commit cd tests/external_pkg_dirs && make TEST_KCONFIG=1 works without issues.
What are your thoughts about this?

[fjmolinas]

The way the external modules are handled in Kconfig confuses me a bit. It uses the make variable EXTERNAL_MODULE_PATHS that is only ever set in dependency_resolution.inc.Mk. But that dependency resolution isn't run when Kconfig is enabled, so from where is this variable set?

I think you caught that the test case was insufficient then..., when I made the PR #16053, #16104 was not yet in, so that test case was not addressed... Thanks for the catch

Anyway with the lastest commit cd tests/external_pkg_dirs && make TEST_KCONFIG=1 works without issues.
What are your thoughts about this?

Looks good I think, could you add a test case in tests/kconfig as well?

[fjmolinas]

I think you caught that the test case was insufficient then..., when I made the PR #16053, #16104 was not yet in, so that test case was not addressed... Thanks for the catch

Although I think the original PR was only about configurations... not sure about dependencies

[fjmolinas]

Thanks for the catch @NikLeberg I opened #17596 to fix, could you do something similar here in tests/kconfig for external packages? Thanks! Otherwise this one looks good!

[NikLeberg]

I extended tests/kconfig. As soon as pr #17596 is in master, I'll rebase and run the test. Without the mentioned pr, the test won't work (module includes will not be found).

[NikLeberg]

Rebased. Kconfig now works on my machine. @fjmolinas Feel free to re-add the CI labels, I think code wise its done.
I also added a bit of documentation. Does it look right?

[fjmolinas]

Look good! Lets see what the ci thinks!

[fjmolinas]

CI had some unrelated issues, re-scheduling and lets see

[fjmolinas]

All green here! Go! Thanks @NikLeberg!