sys/Makefile.dep: Some cleanup #16268

maribu · 2021-04-01T08:07:27Z

Contribution description

cleaned two low hanging fruits in sys/Makefile.dep
split out all GNRC handling to sys/net/gnrc/Makefile.dep

Testing procedure

Tests and applications using GNRC should still generate the same binaries.

Issues/PRs references

Depends on sys/Makefile.dep: clean up #16271

maribu · 2021-04-01T08:09:30Z

Let's see if Murdock finds some obvious regressions.

fjmolinas · 2021-04-01T08:48:47Z

e16c4b6 seems like a more complicated thing to test, the first commit could be acked right away.

maribu · 2021-04-01T09:09:30Z

I split the first commit out as suggested: #16271

fjmolinas · 2021-04-07T07:31:11Z

Seems there is still one conflict @maribu

fjmolinas · 2021-04-07T07:32:05Z

@miri64 can you take a quick look? I can verify dependencies afterwards.

miri64 · 2021-04-07T08:04:08Z

sys/Makefile.dep

+ifneq (,$(filter auto_init_gnrc_netif,$(USEMODULE)))
+  USEMODULE += gnrc_netif_init_devs
+endif


Why can't this be moved to the new gnrc-specific Makefile? At least a comment should be added to explain it, if there is a reason.

If it would be renamed to auto_init_netif_gnrc or gnrc_auto_init_netif it would work. But with gnrc in the middle, GNRC's Makefile.dep will not be included (unless by chance another module with a name starting or ending with gnrc is used). I'll add the comment.

Or we just add auto_init%_gnrc% to the check for GNRC's Makefile.dep inclusion ;-).

I'm not sure if two wildcards are allowed. I once failed to match something like %foo% for some reason, while both %foo and foo% worked.

Ok, then let's keep it as is for now.

miri64 · 2021-04-07T08:08:26Z

sys/Makefile.dep

+  ifneq (,$(filter gnrc%,$(USEMODULE)))
+    USEMODULE += gnrc_sock_async
+  endif


This might also be movable to the new gnrc-specific Makefile.dep:

ifneq (,$(filter gcoap,$(USEMODULE))) ifneq (,$(filter gnrc%,$(USEMODULE))) USEMODULE += gnrc_sock_async endif endif

If it would be renamed to auto_init_netif_gnrc or gnrc_auto_init_netif it would work. But with gnrc in the middle, GNRC's Makefile.dep will not be included (unless by chance another module with a name starting or ending with gnrc is used). I'll add the comment.

Could this become a problem here as well?

miri64 · 2021-04-07T11:09:13Z

Apparently, according to Murdock, there is also a problem with alternate implementation module selection for gnrc_pktbuf now 😕.

fjmolinas · 2021-04-13T08:44:31Z

@maribu the simplest fix is moving fuzzing dependency before the inclusion of the gnrc file.

fjmolinas · 2021-04-13T08:57:09Z

@maribu the simplest fix is moving fuzzing dependency before the inclusion of the gnrc file.

An other option would be:

diff --git a/sys/Makefile.dep b/sys/Makefile.dep
index 77f44e5ff0..a415deec5f 100644
--- a/sys/Makefile.dep
+++ b/sys/Makefile.dep
@@ -302,6 +302,7 @@ ifneq (,$(filter fuzzing,$(USEMODULE)))
   USEMODULE += netdev_test
   USEMODULE += gnrc_netif
   USEMODULE += gnrc_pktbuf_malloc
+  DISABLE_MODULE += gnrc_pktbuf_static
 endif
 
 ifneq (,$(filter netstats_%, $(USEMODULE)))
diff --git a/sys/net/gnrc/Makefile.dep b/sys/net/gnrc/Makefile.dep
index cc0b3d2716..28cb4b9718 100644
--- a/sys/net/gnrc/Makefile.dep
+++ b/sys/net/gnrc/Makefile.dep
@@ -390,18 +390,18 @@ ifneq (,$(filter gnrc,$(USEMODULE)))
   endif
 endif
 
-ifneq (,$(filter gnrc_pktbuf, $(USEMODULE)))
-  ifeq (,$(filter gnrc_pktbuf_%, $(USEMODULE)))
-    USEMODULE += gnrc_pktbuf_static
+ifneq (,$(filter gnrc_pktbuf,$(USEMODULE)))
+  ifeq (,$(filter gnrc_pktbuf_%,$(USEMODULE)))
+    DEFAULT_MODULE += gnrc_pktbuf_static
   endif
-  ifeq (gnrc_pktbuf_cmd,$(filter gnrc_pktbuf_%, $(USEMODULE)))
+  ifneq (,$(filter gnrc_pktbuf_cmd,$(USEMODULE)))
     USEMODULE += gnrc_pktbuf_static
   endif
   DEFAULT_MODULE += auto_init_gnrc_pktbuf
   USEMODULE += gnrc_pkt
 endif
 
-ifneq (,$(filter gnrc_pktbuf_%, $(USEMODULE)))
+ifneq (,$(filter gnrc_pktbuf_cmd, $(USEMODULE)))
   USEMODULE += gnrc_pktbuf # make MODULE_GNRC_PKTBUF macro available for all implementations
 endif
 
diff --git a/tests/unittests/tests-pktbuf/Makefile.include b/tests/unittests/tests-pktbuf/Makefile.include
deleted file mode 100644
index dfacccf361..0000000000
--- a/tests/unittests/tests-pktbuf/Makefile.include
+++ /dev/null
@@ -1 +0,0 @@
-USEMODULE += gnrc_pktbuf_static

It makes explicit when there is a hard dependency and when it's a default choice. What do you think @miri64 @maribu? (I would nonetheless for this PR just move fuzzing up, and do this in a follow up)

miri64 · 2021-04-13T09:01:40Z

I would prefer to not use DISABLE_MODULE/DEFAULT_MODULE for the packet buffer. DEFAULT_MODULE implies (at least to me), that the module is always there and needs to be disabled otherwise. However, the GNRC packet buffer is only there, if GNRC is there. Also: why is it suddenly only gnrc_pktbuf_cmd that pulls in gnrc_pktbuf, when gnrc_pktbuf is required for any gnrc_pktbuf_ implementations?

fjmolinas · 2021-04-13T09:26:47Z

DEFAULT_MODULE implies (at least to me), that the module is always there and needs to be disabled otherwise.

I thought of DEFAULT_MODULE here because both implementations can be resolved at the end, and you specify the default implementation of gnrc_pktbuf which is gnrc_pktbuf_static but then there is the alternative choice gnrc_pktbuf_malloc.

I saw it the other way around, gnrc_pktbuf_static/malloc don't depend on gnrc_pktbu, but the other way around, gnrc_pktbuf depends on having an implementation. I was also thinking in how it would be in Konfig and imagined something like (maybe @leandrolanzieri will say this is horribly wrong):

menuconfig MODULE_GNRC_PKTBUF
    bool "GNRC Packet Buffer"
    default y
    select MODULE_GNRC_PKT

if MODULE_GNRC_PKTBUF

choice
    bool "Packet Buffer implementation"
    default MODULE_GNRC_PKTBUF_STATIC

configMODULE_GNRC_PKTBUF_STATIC
    bool "Static"

config MODULE_GNRC_PKTBUF_MALLOC
    bool "Malloc"

endchoice

Which is why I thought it that way.

Also: why is it suddenly only gnrc_pktbuf_cmd that pulls in gnrc_pktbuf, when gnrc_pktbuf is required for any gnrc_pktbuf_ implementations?

Because gnrc_pktbuf_cmd requires the 'api' so gnrc_pktbuf, while the other two implement the 'api' gnrc_pktbuf.

In any case I can bring it up in a follow up to not cloud discussion here.

miri64 · 2021-04-13T09:36:27Z

Yes, we should focus on moving the GNRC dependencies to their own file first here. Improvements beyond that or Kconfig dependencies we should solve separately. Otherwise, an easy fix suddenly might grow large ;-).

fjmolinas · 2021-04-20T08:55:01Z

Rebased and pushed the fuzzing fix, lets see if murdock is happy.

fjmolinas · 2021-04-20T10:15:55Z

All is green now.

jeandudey · 2021-04-23T11:05:18Z

Another low hanging fruit before this gets merged is this:

RIOT/sys/Makefile

Lines 155 to 157 in c0c3a76

    
           ifneq (,$(filter routing,$(USEMODULE))) 
        
             DIRS += net/routing 
        
           endif

The routing module (the one outside of GNRC) is not present right now, so why not :-)

maribu · 2021-04-23T12:02:49Z

Another low hanging fruit before this gets merged is this:

RIOT/sys/Makefile

Lines 155 to 157 in c0c3a76

ifneq (,$(filter routing,$(USEMODULE)))

DIRS += net/routing

endif

The routing module (the one outside of GNRC) is not present right now, so why not :-)

Maybe as follow up? Murdock just got happy with the current state.

@fjmolinas: I think if you would do the squashing, the shared authorship would remain visible. But I can also do.

miri64 · 2021-04-23T12:31:45Z

Maybe as follow up? Murdock just got happy with the current state.

I also think that this is best. It has the additional advantages, that most moves here can be easily identified with the Git parameter --color-moved.

miri64

For the record: I'm mostly fine with this PR. However, I identified a little issue when looking at the change with git diff --color-moved HEAD~3

sys/net/gnrc/Makefile.dep

jeandudey · 2021-04-23T16:57:06Z

Maybe as follow up? Murdock just got happy with the current state.

Yep, it can wait for this PR

fjmolinas · 2021-04-23T17:35:29Z

Another low hanging fruit before this gets merged is this:

RIOT/sys/Makefile

Lines 155 to 157 in c0c3a76

ifneq (,$(filter routing,$(USEMODULE)))

DIRS += net/routing

endif

The routing module (the one outside of GNRC) is not present right now, so why not :-)

Maybe as follow up? Murdock just got happy with the current state.

@fjmolinas: I think if you would do the squashing, the shared authorship would remain visible. But I can also do.

I don't really care for the autorship, you can do it :)

fjmolinas · 2021-04-23T17:41:59Z

Other than that I did a diff on master and this branch and there is no difference in USEMODULE, only duplicates in FEATURES_%, see diff.txt

fjmolinas

Other than that I did a diff on master and this branch and there is no difference in USEMODULE, only duplicates in FEATURES_%, see diff.txt

Let not delay and have this diverge, please squash out https://github.com/RIOT-OS/RIOT/pull/16268/files#r619182779 and lets get this in

fjmolinas

ACK! maybe wait for @miri64 ACK as well...

fjmolinas · 2021-04-23T19:27:46Z

ACK! maybe wait for @miri64 ACK as well...

Although @miri64 is longterm AFK, maybe already, if there is no echo I'll hit the button

miri64 · 2021-04-23T19:34:07Z

Nope, still there for a few hours ;-). As I said already: I'm fine with merging this, since my only remaining comment was addressed ;-)

maribu requested review from cgundogan, haukepetersen, miri64 and PeterKietzmann as code owners April 1, 2021 08:07

maribu requested review from benpicco and fjmolinas April 1, 2021 08:07

maribu added Area: build system Area: Build system Area: sys Area: System Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 1, 2021

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 1, 2021

maribu mentioned this pull request Apr 1, 2021

sys/auto_init: Updated sht1x auto_init #11872

Closed

maribu mentioned this pull request Apr 1, 2021

sys/Makefile.dep: clean up #16271

Merged

maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 1, 2021

benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 1, 2021

maribu force-pushed the sys/Makefile.dep branch from e16c4b6 to 3dc28e2 Compare April 1, 2021 13:32

miri64 reviewed Apr 7, 2021

View reviewed changes

fjmolinas force-pushed the sys/Makefile.dep branch from 9eac733 to adac828 Compare April 20, 2021 08:54

miri64 reviewed Apr 23, 2021

View reviewed changes

sys/net/gnrc/Makefile.dep Outdated Show resolved Hide resolved

fjmolinas reviewed Apr 23, 2021

View reviewed changes

sys/net/gnrc/Makefile.dep: split out of sys/Makefile.dep

19d92cb

maribu force-pushed the sys/Makefile.dep branch from adac828 to 19d92cb Compare April 23, 2021 18:30

fjmolinas approved these changes Apr 23, 2021

View reviewed changes

miri64 merged commit f7992bb into RIOT-OS:master Apr 23, 2021

leandrolanzieri mentioned this pull request Apr 27, 2021

sys/crypto: Enable support for AES-192, AES-256 #16183

Merged

MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021

maribu deleted the sys/Makefile.dep branch January 23, 2022 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/Makefile.dep: Some cleanup #16268

sys/Makefile.dep: Some cleanup #16268

maribu commented Apr 1, 2021 •

edited by benpicco

Loading

maribu commented Apr 1, 2021

fjmolinas commented Apr 1, 2021 •

edited

Loading

maribu commented Apr 1, 2021

fjmolinas commented Apr 7, 2021

fjmolinas commented Apr 7, 2021

miri64 Apr 7, 2021

maribu Apr 7, 2021

miri64 Apr 7, 2021

maribu Apr 7, 2021

miri64 Apr 7, 2021

miri64 Apr 7, 2021

miri64 Apr 7, 2021

miri64 commented Apr 7, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

miri64 commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

miri64 commented Apr 13, 2021

fjmolinas commented Apr 20, 2021

fjmolinas commented Apr 20, 2021

jeandudey commented Apr 23, 2021

maribu commented Apr 23, 2021

miri64 commented Apr 23, 2021

miri64 left a comment

jeandudey commented Apr 23, 2021

fjmolinas commented Apr 23, 2021

fjmolinas commented Apr 23, 2021

fjmolinas left a comment

fjmolinas left a comment

fjmolinas commented Apr 23, 2021

miri64 commented Apr 23, 2021

sys/Makefile.dep: Some cleanup #16268

sys/Makefile.dep: Some cleanup #16268

Conversation

maribu commented Apr 1, 2021 • edited by benpicco Loading

Contribution description

Testing procedure

Issues/PRs references

maribu commented Apr 1, 2021

fjmolinas commented Apr 1, 2021 • edited Loading

maribu commented Apr 1, 2021

fjmolinas commented Apr 7, 2021

fjmolinas commented Apr 7, 2021

miri64 Apr 7, 2021

Choose a reason for hiding this comment

maribu Apr 7, 2021

Choose a reason for hiding this comment

miri64 Apr 7, 2021

Choose a reason for hiding this comment

maribu Apr 7, 2021

Choose a reason for hiding this comment

miri64 Apr 7, 2021

Choose a reason for hiding this comment

miri64 Apr 7, 2021

Choose a reason for hiding this comment

miri64 Apr 7, 2021

Choose a reason for hiding this comment

miri64 commented Apr 7, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

miri64 commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

miri64 commented Apr 13, 2021

fjmolinas commented Apr 20, 2021

fjmolinas commented Apr 20, 2021

jeandudey commented Apr 23, 2021

maribu commented Apr 23, 2021

miri64 commented Apr 23, 2021

miri64 left a comment

Choose a reason for hiding this comment

jeandudey commented Apr 23, 2021

fjmolinas commented Apr 23, 2021

fjmolinas commented Apr 23, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas commented Apr 23, 2021

miri64 commented Apr 23, 2021

maribu commented Apr 1, 2021 •

edited by benpicco

Loading

fjmolinas commented Apr 1, 2021 •

edited

Loading