Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] tests: group applications in subdirectories #15358

Closed
wants to merge 5 commits into from

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Nov 2, 2020

Contribution description

This PR slightly adapts the makefiles/app_dirs.inc.mk in order to declare lists of subdirectories that can contain applications. This change is applied to the tests directory where the bench, cpp, drivers, gnrc, periph, pkg, thread, xtimer and ztimer subdirectories are introduced.
Application name prefix are also removed, since they are not redundant (gnrc_, driver_, etc).

Some changes were also required in each application Makefile:

include ../Makefile.tests_common

=>

RIOTBASE ?= $(CURDIR)/../../..
include $(RIOTBASE)/tests/Makefile.tests_common

The application is relatively one level lower compared to the common tests Makefile, so this had to be adapted.

This PR is marked as RFC because I think some discussion is needed regarding how to group applications.
In the current state, a preliminary set of groups is defined because I found them the most obvious. Maybe we could also define other groups (like sys ?).
This PR could also be a starting work to group example applications into categories (network, basic, etc) and maybe some test applications (in drivers and/or pkg) could be moved to the examples applications.

All this needs discussion and if we can reach some agreement, the remaining work could be done in follow-up PRs (a tracking issue could be worth I think).

Testing procedure

  • A green Murdock should be enough to detect side effects
  • Verify that the number of applications remain unchanged compared to master:
$ git checkout master 
Switched to branch 'master'
Your branch is up to date with 'riot/master'.
$ make info-applications | wc -l
459
$ git checkout pr/examples/app_in_subdirs
Switched to branch 'pr/examples/app_in_subdirs'
$ make info-applications | wc -l
459

Issues/PRs references

similar to #9933 (and maybe some ideas from there could be reused)
loosely related to #5105

@aabadie aabadie added Area: build system Area: Build system Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Nov 2, 2020
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 2, 2020
@miri64
Copy link
Member

miri64 commented Nov 2, 2020

As a new user, looking at the tests for possible examples, I would wonder, what gnrc is. If we want to do this, how about using a similar scheme many other modular projects use and reflect the tree of the system under test. E.g for GNRC applications tests/sys/net/gnrc/....

Another problem I see with the current hierarchy (but also my own proposal) is that tests that should be together (because they are very similar and might become one application at some point in the future) are split up. The perfect example that comes to mind for me are the *_sock_{udp,ip,tcp} applications. See also #14669

@aabadie
Copy link
Contributor Author

aabadie commented Nov 2, 2020

As a new user, looking at the tests for possible examples, I would wonder, what gnrc is

Then we could rename this category networking and keep the gnrc prefix in the applications inside it. Other test applications could be moved there then.
As a new user (I remember I had this impression when I discovered RIOT), I would wonder why there are so many applications in tests that are not tests but are really example applications (as you said when "looking at the tests for possible examples"). As a new user, I would not spontaneously have the idea to look into the tests directory for example applications. Here I'm more thinking at all the applications for driver and packages but that certainly applies to any applications that are not based on the embunit framework.
If the approach in this PR makes sense (e.g. grouping applications by categories), then the drivers/ and pkg/ subdirectories could be moved to examples/ in follow-up PRs, I guess.

how about using a similar scheme many other modular projects use and reflect the tree of the system under test

The problem I see is the tree depth that would increase. We should find a good trade-off between tree depth and the number of applications at a given level. I think we all agree that the situation in master is not good (427 directories in tests/, 145 with this PR and we can still group more).

Another problem I see with the current hierarchy (but also my own proposal) is that tests that should be together (because they are very similar and might become one application at some point in the future) are split up. The perfect example that comes to mind for me are the *sock{udp,ip,tcp} applications. See also #14669

I agree that some applications could be merged but this is orthogonal to this PR. I also think that we should keep in mind that it's important to have good and visible examples for end-users. Keeping a detailed application for each _sock_udp and _sock_tcp would be useful for newcomers to discover how to use different transport protocols in RIOT. The IP related layer makes a little less sense for end-users.

@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 20c1932 to 0b20ad5 Compare November 3, 2020 10:33
@aabadie aabadie requested a review from basilfx as a code owner November 3, 2020 10:33
@aabadie
Copy link
Contributor Author

aabadie commented Nov 3, 2020

reflect the tree of the system under test

This is probably the best in the current situation. After continuing the work on this PR I came up with the following subdirectories: arch, bench, boards, bootloaders, build_system, core, cpp, drivers, periph, pkg and sys, plus the minimal and unittests applications. I'm not about cpp, maybe applications in this subdiretory should be moved to sys or arch ?

@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 0b20ad5 to 04db28d Compare November 3, 2020 10:52
@aabadie
Copy link
Contributor Author

aabadie commented Nov 3, 2020

Since a lot of test files are touched by this PR, a lot of static tests are failing (cppcheck, coccinelle, flake8, etc...). Even worse, running the static tests takes +7 minutes locally (cppcheck is very slow), and this times out on Murdock.

@miri64
Copy link
Member

miri64 commented Nov 3, 2020

Since a lot of test files are touched by this PR, a lot of static tests are failing (cppcheck, coccinelle, flake8, etc...). Even worse, running the static tests takes +7 minutes locally (cppcheck is very slow), and this times out on Murdock.

Maybe split it up in multiple PRs then?

@miri64
Copy link
Member

miri64 commented Nov 3, 2020

I agree that some applications could be merged but this is orthogonal to this PR. I also think that we should keep in mind that it's important to have good and visible examples for end-users. Keeping a detailed application for each _sock_udp and _sock_tcp would be useful for newcomers to discover how to use different transport protocols in RIOT. The IP related layer makes a little less sense for end-users.

The *_sock_{ip,udp,tcp} tests are more unittests than usage examples (another reason why I would prefer unifying those to make sure the API is unified across all implementations).

@miri64
Copy link
Member

miri64 commented Nov 3, 2020

As a new user (I remember I had this impression when I discovered RIOT), I would wonder why there are so many applications in tests that are not tests but are really example applications (as you said when "looking at the tests for possible examples"). As a new user, I would not spontaneously have the idea to look into the tests directory for example applications. Here I'm more thinking at all the applications for driver and packages but that certainly applies to any applications that are not based on the embunit framework.

looks at @benpicco

@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 04db28d to 4c94a42 Compare November 3, 2020 12:20
@aabadie
Copy link
Contributor Author

aabadie commented Nov 3, 2020

Maybe split it up in multiple PRs then?

This is certainly how it will end up. Reviewing a 2k files changed and +7,538 −7,118 diff cannot be done safely in a single PR.

@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 4c94a42 to 08fd4b5 Compare November 3, 2020 13:10
@miri64
Copy link
Member

miri64 commented Nov 3, 2020

Maybe split it up in multiple PRs then?

This is certainly how it will end up. Reviewing a 2k files changed and +7,538 −7,118 diff cannot be done safely in a single PR.

Not to mention various rebases, due to PRs getting merged, while this review is happening :-/

@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 66b6ca1 to 152b72c Compare February 11, 2021 22:16
@aabadie
Copy link
Contributor Author

aabadie commented Feb 12, 2021

I see a potential drawback related to CI this PR: if a contributor put a test application in a subdirectory that is not listed in app_dirs.inc.mk, it will be skipped by Murdock since it won't be in the list returned by info-applications.
One way to prevent this is to add a check at compile time to verify that the application directory is part of the available applications returned by info-applications. If it's not the case, the build will fail. This won't be checked by Murdock either but will enforce the developer to update app_dirs.inc.mk (assuming he/she will try to build its new application locally!).

@miri64
Copy link
Member

miri64 commented Feb 12, 2021

I see a potential drawback related to CI this PR: if a contributor put a test application in a subdirectory that is not listed in app_dirs.inc.mk

That is basically already a problem, when an application is not in examples or tests ;-).

Comment on lines 7 to 16
EXAMPLES_APPLICATIONS_SUBDIRS := \
ble \
coap \
cord \
dtls \
gnrc \
icn \
lora \
mqtt \
posix \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXAMPLES_APPLICATIONS_SUBDIRS := \
ble \
coap \
cord \
dtls \
gnrc \
icn \
lora \
mqtt \
posix \
EXAMPLES_APPLICATIONS_SUBDIRS := $(shell ls -d examples/)

Why not just do this? This would prevent something like that.

Copy link
Contributor Author

@aabadie aabadie Feb 12, 2021

Choose a reason for hiding this comment

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

I think the idea is to limit calls to external shell commands as it's less efficient than pure make code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then... why not

Suggested change
EXAMPLES_APPLICATIONS_SUBDIRS := \
ble \
coap \
cord \
dtls \
gnrc \
icn \
lora \
mqtt \
posix \
EXAMPLES_APPLICATIONS_SUBDIRS := $(dir $(wildcard examples/*/))

...

Copy link
Contributor Author

@aabadie aabadie Feb 23, 2021

Choose a reason for hiding this comment

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

With the proposed solution, you would get unwanted directories listed (in examples/bindist for instance, but not only):

  • this PR:
$ make info-applications | wc -l
494
  • your suggestion
$ make info-applications | wc -l
496

Copy link
Member

@chrysn chrysn Sep 16, 2022

Choose a reason for hiding this comment

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

Then maybe

$(filter-out $(dir $(wildcard examples/*/Makefile)),$(wildcard examples/*/))

to get all subdirectories of examples that do not contain a Makefile (which the grouping dirs would all not)?

coap \
cord \
dtls \
gnrc \
Copy link
Member

Choose a reason for hiding this comment

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

I think this could change with the discussion below, but I'm not sure if adding gnrc here instead of ipv6, udp is a good idea. I expect that most newcomers would try to search for those and gnrc doesn't say too much unless someone uses RIOT beforehand.

For instance, where would we put GNRC LoRaWAN? gnrc? lora?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just saw #15358 (comment).

But in any case, I still think it would be better to have a more "functional" name than gnrc here. All the examples mentioned in #15358 (comment) have different categories (gnrc_border_router is a border router, gnrc_networking_mac is just using one MAC layer, etc).

I think it would make sense something like:

- border_routers
    - gnrc_border_router
    - openthread_ncp (coming soon)
- ieee802154
    - lwmac
    - gomac
    - submac
-  netif
    - gnrc_networking

or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that one day some applications in the tests might arrive in the examples directory. The current proposal is a way to organize the "storage" of applications in the repo. How well it's organized is not so important IMHO and it will be difficult to find an organization that fits with the diversity of applications. In the long term, what would be beneficial is to add another layer of documentation (using Sphinx, who knows) where example applications could be referenced in different categories (networking, lorawan, gnrc => gnrc_lorawan, etc) but the application would remain stored in the examples/lorawan, because it's the most obvious category.

I fear that it will be impossible to find an organization that fits everyone vision and that this discussion will be endless.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see your point and agree. I also agree that this wouldn't replace documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

I fear that it will be impossible to find an organization that fits everyone vision and that this discussion will be endless.

I agree here, a first step is having an organization, moving that organization around should be easier once the initial change is in.

makefiles/app_dirs.inc.mk Outdated Show resolved Hide resolved
@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from db3d481 to 886cc8d Compare February 23, 2021 15:33
@aabadie aabadie removed the Area: build system Area: Build system label May 18, 2021
@aabadie aabadie force-pushed the pr/examples/app_in_subdirs branch from 886cc8d to 7ea6ad5 Compare May 18, 2021 09:19
@github-actions github-actions bot added the Area: build system Area: Build system label May 18, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@aabadie
Copy link
Contributor Author

aabadie commented Mar 23, 2022

Still of interest for me. I think I'll rebase and split this PR into smaller chunks.

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 23, 2022
# List all available applications
include $(RIOTMAKE)/app_dirs.inc.mk

ifeq (,$(filter $(APPDIR),$(APPLICATION_DIRS_ABSOLUTE)))
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this be screaming in the face of every user of an application built out of tree?

@aabadie
Copy link
Contributor Author

aabadie commented May 12, 2023

I'm closing this since the work is being done somewhere else and this PR is quite outdated.

@aabadie aabadie closed this May 12, 2023
@aabadie aabadie deleted the pr/examples/app_in_subdirs branch May 12, 2023 13:17
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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants