Makefile.include: separate `link` in subtargets by cladmi · Pull Request #8844 · RIOT-OS/RIOT

cladmi · 2018-03-28T13:54:26Z

Contribution description

Add ELFFILE, HEXFILE, print-size targets

It re-introduces #1098 which was removed by #1198

I removed the rebuilding issue by making ELFFILE and $(APPLICATION_MODULE).a depend on a FORCE .PHONY target. I did not use .PHONY because it disables using the file timestamp.
Calling make a second time shows that we are indeed re-going in RIOT directories and re-calling the link command.

For BUILDOSXNATIVE, I also re-used _LINK variable to harmonize things.
I tested it before but would like another run on a mac.

Issues/PRs references

Part of #8838

cladmi · 2018-03-28T15:39:01Z

For me this one is the "tricky" part of #8838 because it is something that had be done before, reverted, assumed to be that way for a long time and should be checked properly to prevent non-rebuilding the elffile.

kaspar030 · 2018-03-28T16:17:55Z

Makefile.include


-_LINK = $(if $(CPPMIX),$(LINKXX),$(LINK)) $(UNDEF) $(LINKFLAGPREFIX)--start-group $(BASELIBS) -lm $(LINKFLAGPREFIX)--end-group  $(LINKFLAGPREFIX)-Map=$(BINDIR)/$(APPLICATION).map $(LINKFLAGS)
+# Linker rule
+$(ELFFILE): FORCE  # Force rebuilding in case _LINK changes


I find the comment confusing. Why should the variable change?

You are right it's not clear.

I tried to prevent adding a new case where make -B would be required to rebuild if you changed LINKFLAGS for any reason.
For example in native by calling first make all then make all-asan or as developer by changing LINKFLAGS in one of the cpu/board makefile.include.

In the current state, during every build the elf file is re-linked even if nothing changed in the archives.
That is what I tried to mimic by putting the FORCE dependency here.

I welcome any suggestion for the comment :)

kaspar030 · 2018-03-28T16:19:49Z

Makefile.include

 ifeq (,$(RIOTNOLINK))
-ifeq ($(BUILDOSXNATIVE),1)
-	$(Q)$(if $(CPPMIX),$(LINKXX),$(LINK)) $(UNDEF) -o $(ELFFILE) $$(find $(BASELIBS) -size +8c) $(LINKFLAGS) $(LINKFLAGPREFIX)-no_pie
+link: ..compiler-check ..build-message $(ELFFILE) $(HEXFILE) print-size


changing hexfile here to flashfile comes later?

Yes, because changing to FLASHFILE requires other changes in the flasher scripts.

kaspar030 · 2018-03-28T16:21:00Z

Makefile.include

 ifeq ($(BUILD_IN_DOCKER),1)
 link: ..in-docker-container
 else
 ## make script for your application. Build RIOT-base here!


Should you touch the file again in this PR, removing this comment would make the world a little better!

kaspar030 · 2018-03-28T16:23:54Z

Makefile.include

+	$(Q)DIRS="$(DIRS)" "$(MAKE)" -C $(APPDIR) -f $(RIOTMAKE)/application.inc.mk
+$(BINDIR)/$(APPLICATION_MODULE).a: FORCE  # we do not know here if it needs to be updated
+
+print-size: $(ELFFILE)


please add mentioning "info-buildsize" (which does not cause a rebuild)

I tried something.

kaspar030 · 2018-03-28T16:24:23Z

Makefile.include

+	$(Q)$(SIZE) $<
+
+$(HEXFILE): $(ELFFILE)
+	$(Q)$(OBJCOPY) $(OFLAGS) $< $@


In this PR I only put it to a separate target but keep the old rule.
I wanted to focus on only splitting the link target.

Many boards are using 'HEXFILE' for a '.bin' file so cannot replace it here.

kaspar030 · 2018-03-28T22:15:25Z

Makefile.include

 .PHONY: all link clean flash flash-only term doc debug debug-server reset objdump help info-modules
+.PHONY: print-size
 .PHONY: ..in-docker-container
+# Target can depend on FORCE to force running build command every time


this explanation needs rewrite, too...

Refactor osx-native linker rule into `_LINK` too.

* Add ELFFILE, HEXFILE, print-size targets $(BINDIR)/$(APPLICATION_MODULE).a target builds libraries in RIOT and in DIRS. Uses a FORCE phony target dependency to force rebuilding but still let `make` use the file modification timestamps

kaspar030

ACK.

cladmi · 2018-04-09T15:46:05Z

Thank you for the review, I rebased #8845 which is the next one in the list.

miri64 · 2018-04-10T14:17:44Z

Sadly, this PR introduced a problem :( (see #8910)

Unittests add libraries in 'BASELIBS' which do not have any rules to be built as they are built by 'application.inc.mk' and the DIRS variable. So make complains about missing target for the unittests archives. The bug was introduced by RIOT-OS#8844 Fixes RIOT-OS#8910

Unittests add libraries in 'BASELIBS' which do not have any rules to be built as they are built by 'application.inc.mk' and the DIRS variable. So make complains about missing target for the unittests archives. The fix tells these files are generated when building '$(APPLICATION_MODULE).a'. The bug was introduced by RIOT-OS#8844 Fixes RIOT-OS#8910

Unittests add libraries in 'BASELIBS' which do not have any rules to be built as they are built by 'application.inc.mk', packages and the DIRS variable. So make complains about missing target for the unittests archives. The fix tells these files are generated when building '$(APPLICATION_MODULE).a'. The bug was introduced by RIOT-OS#8844 Fixes RIOT-OS#8910

cladmi added 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 labels Mar 28, 2018

This was referenced Mar 28, 2018

Makefile.include: add %.hex and %.bin rules #8845

Merged

Makefiles: add support to generate both .hex and .bin file and add FLASHFILE variable #8838

Merged

cladmi requested review from jnohlgard, kaspar030 and smlng March 28, 2018 15:36

kaspar030 reviewed Mar 28, 2018

View reviewed changes

cladmi added 2 commits April 9, 2018 16:50

Makefile.include/OSXNATIVE: refactor into _LINK

a83c8d2

Refactor osx-native linker rule into `_LINK` too.

Makefile.include: separate link in subtargets

f64de06

* Add ELFFILE, HEXFILE, print-size targets $(BINDIR)/$(APPLICATION_MODULE).a target builds libraries in RIOT and in DIRS. Uses a FORCE phony target dependency to force rebuilding but still let `make` use the file modification timestamps

cladmi force-pushed the pr/make/split_link_target branch from 6ae3c67 to f64de06 Compare April 9, 2018 14:52

kaspar030 approved these changes Apr 9, 2018

View reviewed changes

kaspar030 merged commit c23eb5a into RIOT-OS:master Apr 9, 2018

cladmi deleted the pr/make/split_link_target branch April 9, 2018 15:26

emmanuelsearch mentioned this pull request Apr 10, 2018

tests: Unittests don't build #8910

Closed

cladmi mentioned this pull request Apr 10, 2018

Makefile.include: fix missing target for libraries #8911

Merged

miri64 mentioned this pull request Jun 4, 2018

netif: use where appropriate #9287

Open

8 tasks

cladmi mentioned this pull request Jul 23, 2018

Makefile.include: Replace PHONY by FORCE #9623

Merged

cladmi added this to the Release 2018.04 milestone Nov 7, 2018

Conversation

cladmi commented Mar 28, 2018

Contribution description

Issues/PRs references

Uh oh!

cladmi commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Apr 9, 2018

Uh oh!

miri64 commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants