Skip to content

shell: provide submodule for each shell command#16135

Closed
jnohlgard wants to merge 2 commits intoRIOT-OS:masterfrom
jnohlgard:shell-cmd-submodules
Closed

shell: provide submodule for each shell command#16135
jnohlgard wants to merge 2 commits intoRIOT-OS:masterfrom
jnohlgard:shell-cmd-submodules

Conversation

@jnohlgard
Copy link
Member

@jnohlgard jnohlgard commented Mar 3, 2021

Contribution description

Rewrite sys/shell/commands/Makefile to provide a submodule for each shell command and use those modules in the command list. This improves user experience by allowing us to see which shell command modules are compiled in via make info-modules etc. and makes shell_commands less "special" and more like other modules with submodules, such as ztimer.

The introduced Makefile.dep should keep the existing behaviour of adding shell commands automatically based on which modules are enabled in the build.

Testing procedure

Check that shell commands still exist and can be run from the shell in all examples and tests.

Issues/PRs references

@jnohlgard jnohlgard added Area: sys Area: System Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 3, 2021
@miri64
Copy link
Member

miri64 commented Mar 3, 2021

Great idea, but this somewhat runs contrary to the idea of having the shell commands with their module (see #16095 for the beginnings of that). That idea would allow for a better coherence of the modules themselves, in my opinion. But I think having the shell commands then be a sub-module of those modules (and shell_commands being used to collect a set of those) would still be a good way to optimize some applications.

@miri64 miri64 requested a review from kaspar030 March 9, 2021 13:50
@kaspar030
Copy link
Contributor

With #16061 in and moving shell to use XFA's in #16095, we should maybe look at a desired "end state".
This PR definitely improves the situation with the shell commands being part of, well, "shell_commands".

I hoped that we could move them to their respective modules. Still it would be nice to have all shell commands individually optional.

I'd merge this PR as an improvement but it hard clashes with #16095 where XFA was supposed to reduce clashes...

It might just work (wait for #16095, then use this PR) if we move the current ifdefs from "shell_commands.c" into the respective .c files, without ifdef, and let the pseudomodule sort out what commands to build?
We can then "simply" move the .c next to the modules later.

@kaspar030
Copy link
Contributor

@jnohlgard does that make sense to you?

@jnohlgard
Copy link
Member Author

Not sure I understand the whole picture. Do you mean that we use the makefile changes from this PR and move each of the (currently conditionally compiled) array element definitions into separate XFA fragments inside each cmd_*.c?

@kaspar030
Copy link
Contributor

Not sure I understand the whole picture. Do you mean that we use the makefile changes from this PR and move each of the (currently conditionally compiled) array element definitions into separate XFA fragments inside each cmd_*.c?

Yes!

@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
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@fjmolinas
Copy link
Contributor

ping @kaspar030 @jnohlgard

@OlegHahm
Copy link
Member

OlegHahm commented Mar 9, 2022

@jnohlgard, @kaspar030, ping again.

@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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 21, 2022
@miri64
Copy link
Member

miri64 commented Sep 21, 2022

This was now adopted in an alternative approach by @maribu #18355.

@miri64 miri64 closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: sys Area: System State: stale State: The issue / PR has no activity for >185 days Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants