Murdock results

✔️ PASSED

69b3e05 sys/shell: restructure deps

Artifacts

• Documentation preview

This could go even further by removing all USEMODULE += shell from applications where there is:

USEMODULE += shell
USEMODULE += shell_cmds_default

And there's a lot of these :)

There's something weird with the way shell dependencies are managed.

I think

ifneq (,$(filter shell_cmds,$(USEMODULE)))
  USEMODULE += shell
endif

should go in sys/Makefile.dep instead of sys/shell/Makefile.dep. In the second case, it means when both shell and shell_cmds are included, include shell again...

The following code in sys/Makefile.dep should rather be rewritten:

with something like:

ifneq (,$(filter shell_commands,$(USEMODULE)))
  # shell_commands has been renamed to shell_cmds_default, but let's keep this
  # for backward compatibility
  USEMODULE += shell_cmds_default
endif

ifneq (,$(filter shell_cmd_%,$(USEMODULE)))
  # each and every command is a submodule of shell_cmds
  USEMODULE += shell_cmds
endif

ifneq (,$(filter shell_cmds,$(USEMODULE)))
  USEMODULE += shell
endif

And let the shell module manage its own dependencies (so in sys/shell/Makefile.dep):

USEMODULE += stdin

ifneq (,$(filter shell_cmds_default,$(USEMODULE)))
  USEMODULE += shell_cmd_sys

  ...

Also isn't the benefit of this PR to allow to just include shell_command_default instead of both shell and shell_command_default. So why not apply this new pattern everywhere for consistency, from the start ?

One last thing: should Kconfig be also updated ?

One last thing: should Kconfig be also updated ?

No, in KConfig the dependency is there already. It was just missing in the Makefile bases dependency modeling.

There's something weird with the way shell dependencies are managed.

I think

ifneq (,$(filter shell_cmds,$(USEMODULE)))
  USEMODULE += shell
endif

should go in sys/Makefile.dep instead of sys/shell/Makefile.dep.

But why? sys/Makefile.dep is already pretty crowded and a hot spot for merge conflicts. Having all shell related dependencies in sys/shell/Makefile.dep is much better structured.

Also isn't the benefit of this PR to allow to just include shell_command_default instead of both shell and shell_command_default. So why not apply this new pattern everywhere for consistency, from the start ?

I can do that, one sec.

But why?

Because it's more correct ?

Having all shell related dependencies in sys/shell/Makefile.dep is much better structured.

Except that shell_cmds and shell_cmd_default (both are pseudomodules) are not dependencies of shell but the other way around. If you look at how drivers dependencies are managed, my proposal goes into that direction. Each driver has a Makefile.dep to manage its own dependencies but the drivers/Makefile.dep contains some dependency resolution logic when pseudomodules are pulled in (for example "sx126x" is the concrete module but sx126% dependencies are handled in drivers/Makefile.dep.

My proposal above does exactly the same for shell.

Actually to get the same behavior as for driver modules, there's something missing in the top level Makefile.dep file:

# pull Makefile.dep of each sys modules if they exist
-include $(sort $(USEMODULE:%=$(RIOTBASE)/sys/%/Makefile.dep))

And that should do the trick (and we could have a Makefile.dep per sys module that contain their own dependencies)

Using the following patch works for me and I find it cleaner than the current master version:

git format-patch -1 --stdout 
From 4221a118d48c4c3d4930d03df1e2bb5a5ba8162c Mon Sep 17 00:00:00 2001
From: Alexandre Abadie <alexandre.abadie@inria.fr>
Date: Fri, 21 Apr 2023 09:23:52 +0200
Subject: [PATCH] sys/shell: cleanup shell dependency resolution

---
 Makefile.dep           |  3 +++
 sys/Makefile.dep       | 16 +++++++++++++---
 sys/shell/Makefile.dep | 11 +----------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/Makefile.dep b/Makefile.dep
index db56c9c1f0..d8a1074c6d 100644
--- a/Makefile.dep
+++ b/Makefile.dep
@@ -19,6 +19,9 @@ include $(RIOTBASE)/drivers/Makefile.dep
 # pull Makefile.dep of each driver modules if they exist
 -include $(sort $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep))
 
+# pull Makefile.dep of each sys modules if they exist
+-include $(sort $(USEMODULE:%=$(RIOTBASE)/sys/%/Makefile.dep))
+
 # pull dependencies from packages
 -include $(PKG_PATHS:%=%Makefile.dep)
 
diff --git a/sys/Makefile.dep b/sys/Makefile.dep
index 5cb8f39945..2849fc029f 100644
--- a/sys/Makefile.dep
+++ b/sys/Makefile.dep
@@ -334,9 +334,19 @@ ifneq (,$(filter random_cmd,$(USEMODULE)))
   USEMODULE += shell_cmd_random
 endif
 
-ifneq (,$(filter shell%,$(USEMODULE)))
-  USEMODULE += stdin
-  include $(RIOTBASE)/sys/shell/Makefile.dep
+ifneq (,$(filter shell_commands,$(USEMODULE)))
+  # shell_commands has been renamed to shell_cmds_default, but let's keep this
+  # for backward compatibility
+  USEMODULE += shell_cmds_default
+endif
+
+ifneq (,$(filter shell_cmd%,$(USEMODULE)))
+  # each and every command is a submodule of shell_cmds
+  USEMODULE += shell_cmds
+endif
+
+ifneq (,$(filter shell_cmds,$(USEMODULE)))
+  USEMODULE += shell
 endif
 
 # Include all stdio_% dependencies after all USEMODULE += stdio_%
diff --git a/sys/shell/Makefile.dep b/sys/shell/Makefile.dep
index eb2360479c..da3616e0e5 100644
--- a/sys/shell/Makefile.dep
+++ b/sys/shell/Makefile.dep
@@ -1,13 +1,4 @@
-ifneq (,$(filter shell_cmd_%,$(USEMODULE)))
-  # each and every command is a submodule of shell_cmds
-  USEMODULE += shell_cmds
-endif
-
-ifneq (,$(filter shell_commands,$(USEMODULE)))
-  # shell_commands has been renamed to shell_cmds_default, but let's keep this
-  # for backward compatibility
-  USEMODULE += shell_cmds_default
-endif
+USEMODULE += stdin
 
 ifneq (,$(filter shell_cmds_default,$(USEMODULE)))
   USEMODULE += shell_cmd_sys
-- 
2.37.2

Since you insist, I move the dependencies on sys/shell to sys/Makefile.dep

diff --git a/Makefile.dep b/Makefile.dep
index db56c9c1f0..d8a1074c6d 100644
--- a/Makefile.dep
+++ b/Makefile.dep
@@ -19,6 +19,9 @@ include $(RIOTBASE)/drivers/Makefile.dep
 # pull Makefile.dep of each driver modules if they exist
 -include $(sort $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep))
 
+# pull Makefile.dep of each sys modules if they exist
+-include $(sort $(USEMODULE:%=$(RIOTBASE)/sys/%/Makefile.dep))
+
 # pull dependencies from packages
 -include $(PKG_PATHS:%=%Makefile.dep)
 
diff --git a/sys/Makefile.dep b/sys/Makefile.dep
index 5cb8f39945..2849fc029f 100644

Let's not get overboard with this PR. This PR is intended to just fix a simple bug.

Let's not get overboard with this PR. This PR is intended to just fix a simple bug.

I agree. If unsure we can still do in a follow up PR. I tried to apply that change (the -include $(sort $(USEMODULE:%=$(RIOTBASE)/sys/%/Makefile.dep)) trick) and I think it might have some impact in the dependency resolution of xtimer/ztimer where it seems inclusion order matters a lot. I opened a draft in #19492 (doesn't contain the shell changes though)

Since you insist, I move the dependencies on sys/shell to sys/Makefile.dep

Thanks! I don't see the diff from #19485 (comment) in the PR changes. But let's keep it like this as I think it can have a high impact and requires special care.

bors merge

Build succeeded:

• Murdock
• static-tests
• tools-build-success