make: unify app folder search (examples/*, tests/*, ...) #9867
Conversation
kaspar030
added
Area: build system
kaspar030
force-pushed
the
pr/unify_app_folder_search
branch
from
b629d33 to
4a5aee8
Compare
|
@cladmi didn't you have a PR for this somewhere already? |
kaspar030
changed the title
There was a problem hiding this comment.
I tested this PR locally and everything is fine.
It also fixes error messages (find: ‘./examples/hello-world/bin’: No such file or directory) that were displayed from time to time when calling distclean.
There are flake8 issues in this PR, as reported by the cI. And since compile_test.py is touched and already contains flake8 issues, there are unrelated issues raised as well. I'll push a PR for this.
| @@ -83,12 +85,31 @@ def get_results_and_output_from(fd): | |||
| elif read_more_output: | |||
| output.write(line) | |||
|
|
|||
| def get_app_dirs(): | |||
| yield from check_output(["make", "-f", "makefiles/app_dirs.inc.mk", "info-apps"]) \ | |||
There was a problem hiding this comment.
Any reason for using yield from ?
|
@kaspar030, this needs rebase now that #9868 is merged. |
kaspar030
force-pushed
the
pr/unify_app_folder_search
branch
3 times, most recently
from
83180a1 to
80ecb71
Compare
|
rebased |
Fixed. (I accidentally squashed, sorry). |
|
@kaspar030 No I only had a test branch that I never pushed and lost. |
makefiles/app_dirs.inc.mk
Outdated
| # 2. use patsubst to drop trailing "/" | ||
| # 3. use patsubst to drop possible leading "./" | ||
| # 4. sort | ||
| APP_DIRS := $(sort $(patsubst ./%,%,$(patsubst %/,%,$(dir $(wildcard \ |
There was a problem hiding this comment.
I do not like the APP_ I would prefer APPLICATION_DIRS.
In Rapstore we think about putting multiple Apps in one RIOT APPLICATION, using the abbreviation here does not help with the confusion.
| folder, app = app_dir.split("/", 1) | ||
| res[folder].append(app) | ||
|
|
||
| return res |
There was a problem hiding this comment.
I think it is better to return a normal dict return dict(res) as using a default dict without knowing it is one can lead to trouble.
There was a problem hiding this comment.
I do not find any reference about it anymore tough.
There was a problem hiding this comment.
then let's keep it.
|
The other things I had in my branch and that I can propose with upcoming PRs is:
Something I am thinking about but not drafted yet, is move the |
makefiles/app_dirs.inc.mk
Outdated
| $(RIOTBASE)/tests/*/Makefile \ | ||
| ))))) | ||
|
|
||
| info-apps: |
There was a problem hiding this comment.
Same here, I would use applications.
| @@ -86,12 +89,33 @@ def get_results_and_output_from(fd): | |||
| output.write(line) | |||
|
|
|||
|
|
|||
| def get_app_dirs(): | |||
| yield from check_output(["make", "-f", "makefiles/app_dirs.inc.mk", "info-applications"]) \ | |||
There was a problem hiding this comment.
The script is currently supposed to be python2 compatible and python2 is used by default on ubuntu because of the shebang. yield from breaks this compatibility.
Also here, as split returns a list and not a generator, yield from adds nothing.
Even check_output and decode are not generator.
There was a problem hiding this comment.
ok, it is just returning now.
There was a problem hiding this comment.
Tested dist/tools/compile_test/compile_test.py on ubuntu stable (python->python2) / arch (python->python3) apps are split as before.
make distclean works correctly.
make info-applications too.
I did not check the number of jobs compared to master in murdock.
@aabadie please tell when you agree to squash.
This helper Makefile is supposed to unify finding folders with applications that can be built.
kaspar030
force-pushed
the
pr/unify_app_folder_search
branch
from
9a11f4a to
de55265
Compare
|
Nothing else to add from my side. Let's merge this one. |
Contribution description
There were at least three variants of getting a list of examples and tests application folders at even more places in the code.
This PR tries to unify this.
Testing procedure