Makefile.base: cleanup non selected source object files by leandrolanzieri · Pull Request #16945 · RIOT-OS/RIOT

leandrolanzieri · 2021-10-04T09:33:38Z

Contribution description

As explained in #16942, since #14754 we select all object files when linking, which is a problem when the selection of module sources change between two builds. This adds a cleanup for non selected source object files on modules.

Testing procedure

A minimal example that shows the current issue:

diff --git a/tests/external_module_dirs/Makefile b/tests/external_module_dirs/Makefile
index f44d1e3e8e..f4a82e6c16 100644
--- a/tests/external_module_dirs/Makefile
+++ b/tests/external_module_dirs/Makefile
@@ -4,4 +4,6 @@ USEMODULE += random
 USEMODULE += external_module
 EXTERNAL_MODULE_DIRS += external_modules
 
+USEMODULE += external_module_implementation_a
+
 include $(RIOTBASE)/Makefile.include
diff --git a/tests/external_module_dirs/external_modules/external_module/Makefile b/tests/external_module_dirs/external_modules/external_module/Makefile
index 48422e909a..a0fd02f4b4 100644
--- a/tests/external_module_dirs/external_modules/external_module/Makefile
+++ b/tests/external_module_dirs/external_modules/external_module/Makefile
@@ -1 +1,9 @@
+# enable submodules
+SUBMODULES := 1
+
+# base module name
+BASE_MODULE := external_module
+
+SRC += external_module.c
+
 include $(RIOTBASE)/Makefile.base
diff --git a/tests/external_module_dirs/external_modules/external_module/Makefile.include b/tests/external_module_dirs/external_modules/external_module/Makefile.include
index b6d95d22b5..d7225f8d78 100644
--- a/tests/external_module_dirs/external_modules/external_module/Makefile.include
+++ b/tests/external_module_dirs/external_modules/external_module/Makefile.include
@@ -1,3 +1,6 @@
 # Use an immediate variable to evaluate `MAKEFILE_LIST` now
 USEMODULE_INCLUDES_external_module := $(LAST_MAKEFILEDIR)/include
 USEMODULE_INCLUDES += $(USEMODULE_INCLUDES_external_module)
+
+PSEUDOMODULES += external_module_implementation_a
+PSEUDOMODULES += external_module_implementation_b
diff --git a/tests/external_module_dirs/external_modules/external_module/implementation_a.c b/tests/external_module_dirs/external_modules/external_module/implementation_a.c
new file mode 100644
index 0000000000..19c48d0020
--- /dev/null
+++ b/tests/external_module_dirs/external_modules/external_module/implementation_a.c
@@ -0,0 +1,2 @@
+
+char *message_implementation = "Linked implementation A";
diff --git a/tests/external_module_dirs/external_modules/external_module/implementation_b.c b/tests/external_module_dirs/external_modules/external_module/implementation_b.c
new file mode 100644
index 0000000000..8b126371e8
--- /dev/null
+++ b/tests/external_module_dirs/external_modules/external_module/implementation_b.c
@@ -0,0 +1,2 @@
+
+char *message_implementation = "Linked implementation B";
diff --git a/tests/external_module_dirs/external_modules/external_module/include/external_module.h b/tests/external_module_dirs/external_modules/external_module/include/external_module.h
index f358ef4786..aefd12679e 100644
--- a/tests/external_module_dirs/external_modules/external_module/include/external_module.h
+++ b/tests/external_module_dirs/external_modules/external_module/include/external_module.h
@@ -29,6 +29,8 @@ extern "C" {
  */
 extern char *external_module_message;
 
+extern char *message_implementation;
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/tests/external_module_dirs/main.c b/tests/external_module_dirs/main.c
index 9695fb6826..421438a0c3 100644
--- a/tests/external_module_dirs/main.c
+++ b/tests/external_module_dirs/main.c
@@ -34,5 +34,6 @@ int main(void)
 {
     puts("If it compiles, it works!");
     printf("Message: %s\n", external_module_message);
+    printf("Submodule message: %s\n", message_implementation);
     return 0;
 }

Compile once with USEMODULE += external_module_implementation_a and once with USEMODULE += external_module_implementation_b, message_implementation will have two implementations at link time on master. With this PR this should be fixed.

Issues/PRs references

#16942
Issue introduced by #14754

fjmolinas · 2021-10-04T11:31:23Z

I tested that this fixes the issue for me.

fjmolinas · 2021-10-04T11:47:35Z

I was trying to see if there was another way of doing this where instead of cleaning unused object files we would specify the want to use, but this information is AFAIK not easy to obtain when we are linking. Nonetheless, I'm under the impression that with this change building examples/gnrc_networking is 20% slower,

leandrolanzieri · 2021-10-04T12:41:01Z

Nonetheless, I'm under the impression that with this change building examples/gnrc_networking is 20% slower,

What about now?

fjmolinas · 2021-10-04T13:45:52Z

Nonetheless, I'm under the impression that with this change building examples/gnrc_networking is 20% slower,

What about now?

Still seem to increase in about 13%, but well I guess there is bound to be an increase...

leandrolanzieri · 2021-10-05T06:58:50Z

Nonetheless, I'm under the impression that with this change building examples/gnrc_networking is 20% slower,

What about now?

Still seem to increase in about 13%, but well I guess there is bound to be an increase...

As discussed offline, I now avoid calling rm if no objects should be removed.

fjmolinas

Looks good!

fjmolinas · 2021-10-05T08:46:40Z

Please squash @leandrolanzieri!

fjmolinas · 2021-10-05T08:47:37Z

@leandrolanzieri please trigger the ci after squashing!

kaspar030 · 2021-10-05T09:12:35Z

LGTM to me as well, quite elegant actually!

chrysn · 2021-10-05T09:13:06Z

Looks good; I didn't test, but it ticks all other boxes.

kaspar030 · 2021-10-05T09:39:43Z

please squash!

kaspar030

ACK.

leandrolanzieri · 2021-10-05T11:32:17Z

Release managers since #14754 was merged (@bergzand @jia200x @kaspar030 @MrKevinWeiss): how far do you think this should be backported?

jia200x · 2021-10-05T11:35:33Z

Release managers since #14754 was merged (@bergzand @jia200x @kaspar030 @MrKevinWeiss): how far do you think this should be backported?

IMO it should only be backported to the last release. We usually don't provide LTS

kaspar030 · 2021-10-05T11:44:52Z

IMO it should only be backported to the last release.

yup, or maybe not even that. this basically fixes an inconvenience. it is also non-intrusive, so merging it before the next RC wouldn't hurt.

MrKevinWeiss · 2021-10-05T11:53:33Z

I guess that is me then 🦸

kfessel · 2021-10-05T11:59:34Z

does this work for non pseudo modules?

i see - it does not need to delete object of not used modules since they are just not added to the obj list

fjmolinas · 2021-10-05T12:43:09Z

does this work for non pseudo modules?

Nonpseudomodules are ignored from linking in another stage here:

RIOT/Makefile.include

Lines 669 to 674 in e804a73

    
           $(ELFFILE): FORCE 
        
           ifeq ($(BUILDOSXNATIVE),1) 
        
             _LINK = $(if $(CPPMIX),$(LINKXX),$(LINK)) $$(find $(BASELIBS:%.module=$(BINDIR)/%/) -name "*.o" 2> /dev/null | sort) $(ARCHIVES_GROUP) $(LINKFLAGS) $(LINKFLAGPREFIX)-no_pie 
        
           else 
        
             _LINK = $(if $(CPPMIX),$(LINKXX),$(LINK)) $$(find $(BASELIBS:%.module=$(BINDIR)/%/) -name "*.o" 2> /dev/null | sort) $(ARCHIVES_GROUP) $(LINKFLAGS) $(LINKFLAGPREFIX)-Map=$(BINDIR)/$(APPLICATION).map 
        
           endif # BUILDOSXNATIVE

MODULES that are not selected would not be in BASELIBS, the issues is that we here linking everything under MODULE/ with the wildcard, see:

RIOT/makefiles/modules.inc.mk

Lines 11 to 14 in e804a73

    
           # filter "pseudomodules" from "real modules", but not "no_pseudomodules" 
        
           REALMODULES += $(filter-out $(PSEUDOMODULES), $(_ALLMODULES)) 
        
           REALMODULES += $(filter $(NO_PSEUDOMODULES), $(_ALLMODULES)) 
        
           BASELIBS += $(REALMODULES:%=%.module)

And conditional SRC selection is always based on PSEUDOMODULES or SUBMODULES (which are the same), or in some cases on CPU_FAM but this do not change per build.

fjmolinas

ACK and GO!

leandrolanzieri · 2021-10-05T14:54:12Z

Thanks for reviewing !

leandrolanzieri · 2021-10-05T15:02:06Z

Backport provided in #16953

MrKevinWeiss · 2021-10-05T15:33:54Z

Were we backporting to the 07 release too or just the 10?

leandrolanzieri added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 4, 2021

leandrolanzieri requested review from cgundogan and fjmolinas October 4, 2021 09:33

github-actions Bot added the Area: build system Area: Build system label Oct 4, 2021

fjmolinas reviewed Oct 5, 2021

View reviewed changes

Comment thread Makefile.base Outdated

fjmolinas requested a review from kaspar030 October 5, 2021 08:46

leandrolanzieri force-pushed the pr/build_system/delete_non_selected_objects branch from 92130d8 to 5bcd138 Compare October 5, 2021 08:52

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 5, 2021

kaspar030 self-assigned this Oct 5, 2021

kaspar030 reviewed Oct 5, 2021

View reviewed changes

Comment thread Makefile.base

kaspar030 approved these changes Oct 5, 2021

View reviewed changes

leandrolanzieri force-pushed the pr/build_system/delete_non_selected_objects branch from 0009c32 to 452333c Compare October 5, 2021 10:59

Makefile.base: cleanup non selected source object files

452333c

fjmolinas approved these changes Oct 5, 2021

View reviewed changes

fjmolinas merged commit 58726bf into RIOT-OS:master Oct 5, 2021

fjmolinas mentioned this pull request Oct 5, 2021

Deleted or non selected source files are linked in #16942

Closed

leandrolanzieri deleted the pr/build_system/delete_non_selected_objects branch October 5, 2021 14:54

leandrolanzieri mentioned this pull request Oct 5, 2021

Makefile.base: cleanup non selected source object files [backport 2021.10] #16953

Merged

david-vankampen mentioned this pull request Oct 12, 2021

make check_bindist fails to find reference to main #16977

Closed

leandrolanzieri mentioned this pull request Oct 13, 2021

Makefile.base: do not clean objects of bindist modules #16981

Merged

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

Uh oh!

Conversation

leandrolanzieri commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Oct 4, 2021

Uh oh!

fjmolinas commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leandrolanzieri commented Oct 4, 2021

Uh oh!

fjmolinas commented Oct 4, 2021

Uh oh!

leandrolanzieri commented Oct 5, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas commented Oct 5, 2021

Uh oh!

fjmolinas commented Oct 5, 2021

Uh oh!

kaspar030 commented Oct 5, 2021

Uh oh!

chrysn commented Oct 5, 2021

Uh oh!

Uh oh!

kaspar030 commented Oct 5, 2021

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Oct 5, 2021

Uh oh!

jia200x commented Oct 5, 2021

Uh oh!

kaspar030 commented Oct 5, 2021

Uh oh!

MrKevinWeiss commented Oct 5, 2021

Uh oh!

kfessel commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Oct 5, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Oct 5, 2021

Uh oh!

leandrolanzieri commented Oct 5, 2021

Uh oh!

MrKevinWeiss commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

leandrolanzieri commented Oct 4, 2021 •

edited

Loading

fjmolinas commented Oct 4, 2021 •

edited

Loading

kfessel commented Oct 5, 2021 •

edited

Loading