Makefile.include: replace $(USEPKG:%=$(BINDIR)/%.a) target

[cladmi]

Contribution description

Replace the per package archive file target by phony targets.

This allows at the same time to correctly describe PSEUDOMODULES packages as they should not be file targets and prepares for build system changes where some packages could produce objects
instead of archives for the 'ld -r' PR.

Having an explicit file target here was not adding anything.

Testing procedure

If all applications build without problem even in parallel mode and the test application commit also build, everything should be ok.

Issues/PRs references

• Mentioned the problem of packages generating objects in a comment in ld -r PR make: Use ld -r for intermediate linking of each module #8711
• Noticed the PSEUDOMODULES issue when working on wolfssl Wolfssl pkg with GNRC support #9894

[cladmi]

The test application also works in master, as nothing uses the file, but the file does not exist after building in master despite being a file target:

$ make -C examples/hello-world/  RIOT_CI_BUILD=1
make: Entering directory '/home/cladmi/git/work/RIOT/examples/hello-world'
make[1]: Nothing to be done for 'Makefile.include'.
Building application "hello-world" for "native" with MCU "native".

make[1]: Nothing to be done for 'prepare'.
make[1]: Nothing to be done for 'all'.
   text    data     bss     dec     hex filename
  22949     568   47716   71233   11641 /home/cladmi/git/work/RIOT/examples/hello-world/bin/native/hello-world.elf
make: Leaving directory '/home/cladmi/git/work/RIOT/examples/hello-world'

$ ls examples/hello-world/bin/native/fake_pkg.a
ls: cannot access 'examples/hello-world/bin/native/fake_pkg.a': No such file or directory

[jnohlgard]

See inline comment

[jnohlgard]

@cladmi Does this correctly detect when a file is modified in the pkg?
This is an order-only prerequisite which is also a .PHONY, which is unfamiliar territory for me. We need to be certain that this does not cause make to assume that the target is up to date when the pkg files have been modified.

[cladmi]

@gebart For me order-only .PHONY prerequisite is something you execute everytime but you still check the file timestamp.
I used an 'order-only' because building it should not invalidate the other files timestamp, it is not its role here.

My train of thoughts here is:

• ELFFILE is rebuilt if files in $(BASELIBS) changed (but also depend on FORCE, so it is rebuilt everytime to ensure it is rebuilt if we change one of the variables)
• These lines say that "Files in $(BASELIBS) are generated by executing the $(BINDIR)/$(APPLICATION_MODULE).a and pkg-build targets.
• The target $(BINDIR)/$(APPLICATION_MODULE).a, that in fact generate most of the targets in $(BASELIBS), currently depends on FORCE to rebuild it everytime
• The target pkg-build is .PHONY so rebuilt everytime

So now, they are all always executed.

I just put it order-only as for me, it is the same behavior as before, execute it every time (there was $(USEPKG:%=$(BINDIR)/%.a): $(BUILDDEPS) FORCE before) but still consider the generated files timestamps.

Currently it also does not really change anything as $(BUILDDEPS) contains .PHONY targets so forces rebuilding anyway. I may be able to change it to an order-only target, just a bit worried of the make clean all -j case.

[jnohlgard]

What I am confused about is how make treats the indirect dependencies via the order only prereq. Does make treat the .a file as needing a rebuild, or do we introduce a race here?
I know that the elf is always relinked in the current implementation of the build system, but that is not really what we want to do when nothing has changed, so it is necessary that the dependencies are correct or we are likely to introduce hidden problems that will bite us in the future.

[cladmi]

I tested on a smaller makefile and you are right, make does not always re-evaluate the .a files timestamp after the order-only .PHONY target.
At least, it does not if there is only one target in the list of targets, if there is more than one, it does re-evaluate them after executing the order-only .PHONY target.

It is even lazier than what I thought, which makes sense, it does not re-evaluate the .a files timestamp even if it depends on a target that modifies another file than the target name.

So currently it only works because there are more than one target in _SUBMAKE_LIBS and if not it would because of the FORCE dependencies everywhere.

I will replace with a normal dependency here as the order-only does not currently add anything and only introduces confusion.

Testing

Details of testing showing the state in master, (except without FORCE everywhere and only one target):

.PHONY: all clean FORCE

all: app.elf

app.elf: pkg-sub.a pkg.a applications.a
        cat $^ > $@

pkg-sub.a: pkg.a applications.a

pkg.a: FORCE
        echo '1' > pkg-sub.a

applications.a: FORCE
        echo '2' > pkg.a
        test -f $@ || echo '3' > $@

clean:
        rm -f *.a *.elf

Now if you do

$ make clean
rm -f *.a *.elf
$ make
echo '1' > pkg-sub.a
echo '2' > pkg.a
test -f applications.a || echo '3' > applications.a
cat pkg-sub.a pkg.a applications.a > app.elf
$ make
echo '1' > pkg-sub.a
echo '2' > pkg.a
test -f applications.a || echo '3' > applications.a
$ make
echo '1' > pkg-sub.a
echo '2' > pkg.a
test -f applications.a || echo '3' > applications.a
cat pkg-sub.a pkg.a applications.a > app.elf
$ make
echo '1' > pkg-sub.a
echo '2' > pkg.a
test -f applications.a || echo '3' > applications.a

We have an applications.a target that does not change the file if it is up to date and also create the pkg.a archive. The pkg.a creates another one. And the final application is not linked half of the times.

And yes we have the case in our build system with for example the nordic_softdevice_ble archive that is not built by the $(USEPKG:%=$(BINDIR)/%.a) target.

$ make --no-print-directory BOARD=nrf52dk -C examples/gnrc_networking ${PWD}/examples/gnrc_networking/bin/nrf52dk/nordic_softdevice_ble.a
/home/cladmi/git/work/RIOT/Makefile.dep:87: nordic_softdevice_ble: Disabling router modules: gnrc_ipv6_router_default gnrc_rpl auto_init_gnrc_rpl netstats_rpl                                                                                                                           
make[1]: Nothing to be done for 'Makefile.include'.
/home/cladmi/git/work/RIOT/dist/tools/dlcache/dlcache.sh: getting "https://developer.nordicsemi.com/nRF5_IoT_SDK/nRF5_IoT_SDK_v0.9.x/nrf5_iot_sdk_3288530.zip" from cache                                                                                                                
rm -rf /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src
mkdir -p /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src
rm /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/ble/common/ble_conn_params.c
cat /home/cladmi/git/work/RIOT/pkg/nordic_softdevice_ble/Makefile.module > /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/softdevice/common/softdevice_handler/Makefile                                                        
echo "MODULE=ble_common" > /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/ble/common/Makefile                                                                                                                                  
cat /home/cladmi/git/work/RIOT/pkg/nordic_softdevice_ble/Makefile.module >> /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/ble/common/Makefile                                                                                 
cat /home/cladmi/git/work/RIOT/pkg/nordic_softdevice_ble/Makefile.module > /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/iot/ble_ipsp/Makefile                                                                                
touch /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/.extracted
"make" -C /home/cladmi/git/work/RIOT/pkg/nordic_softdevice_ble
cp /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/iot/ble_6lowpan/lib/ble_6lowpan.a /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/nrf52dk/ble_6lowpan.a                                                              
cp /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/pkg/nrf52dk/nordic_softdevice_ble/src/components/softdevice/s1xx_iot/s1xx-iot-prototype3_nrf52_softdevice.hex /home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/nrf52dk/softdevice.hex                                  

$ ls ${PWD}/examples/gnrc_networking/bin/nrf52dk/nordic_softdevice_ble.a
ls: cannot access '/home/cladmi/git/work/RIOT/examples/gnrc_networking/bin/nrf52dk/nordic_softdevice_ble.a': No such file or directory

It is build by having $(RIOTBASE)/pkg/nordic_softdevice_ble/src in DIRS.

[jnohlgard]

Is this really the right way to go?
A file depending on a phony target means that make will always consider the file out of date and force a rebuild. I think we should work towards correcting the dependencies so that in the future we won't need the FORCE targets and the elf does not need to be relinked when nothing has changed.

[cladmi]

@gebart I know, that is in my TODO pipeline to remove the FORCE and I started the process some time ago. It just takes many steps. Before they were all .PHONY targets and not even separated.

I explain below why I go with this only.

It's not perfect and would gladly adapt or take yours if you have a better solution.

---

The problems with the current rule, is that it is wrong that each $(BINDIR)/pkgname.a is built by going in the package directory:

• One counter example is nordic_softdevice_ble which is built by the $(APPLICATION_MODULE)/.a target. And because of this, make would not reconsider the file timestamp even if updated as it is not by the target that said it changes it.
• it also prevents having packages that are pseudomodules which may be required for wolfssl if you only want to use the crypto libraries and not the ssl stack.

Keeping this wrong rule is for me worst than having it defined as a .PHONY target.
One is false, the other "only" forces rebuilding.

I replaced the order-only target by a normal .PHONY target because you advised to do it and that indeed in the current state it was not adding anything except confusion.

For me this current change is required for the ld -r PR because even if separating between REALPKGS LIBPKGS for this rule it would still be wrong that all packages archives and modules are all build in there, so I would not go in this direction of making it more complex but still wrong.

---

To give some history context, in march there were no file target to build the libraries, the elffile, the binfile, all were just a "bash script" block f64de06#diff-ebbb3165b3276a049759e5cb4e04d6a4
So make could never consider that a file may be up to date and should not be rebuilt.

[cladmi]

My real way to go later that I thing about, is to define a variable for each module MODULE_SRC_DIR_module_name that can then be used directly in Makefile.include to go in the correct directory to build instead of doing the recursive application_dir -> sys -> net/gnrc -> netif make hierarchy. But I did not push it yet as there were higher priority blocking issues.

Even add some MODULE_MAKEFILE_PATH_module_name to handle packages where the makefile is stored somewhere else than in the source directory.

And to remove the FORCE, do somehow the same thing as the CFLAGS file and lazy save all the variables that should trigger a rebuild #9634.

[jnohlgard]

Thanks for the detailed explanation. I understand that this is a step on the way to a more robust build environment. ACK

[jnohlgard]

please squash

[cladmi]

Thanks for the thorough review, I really appreciate that you challenged it.
It showed the limits and that I did not explained correctly all the justifications.

I will try to summarize in the commit message.

[cladmi]

I will try to push it on top of the ld -r PR before merging to verify it does not break this as it was the goal of this PR.

[cladmi]

I tried to summarize the explanation in the last commit, please tell if you want me to adapt it.

[jnohlgard]

Looks fine, please squash

[cladmi]

For me they are split in independent commits and do some cleanup before doing the final "big" change, so would rather keep them, they all correctly compile tests/lwip.

However I can remove the "NEEDS SQUASHING" label that I set because I had a "TEST REMOVE ME" commit.

[cladmi]

I will just push them on the ld -r PR and see if it works. Please wait for my input before merging.

[cladmi]

I can also change pkg-build to be on the left of $(BUILDDEPS) now that it is not anymore an order-only dependency.

[cladmi]

I amended the last commit with the following change to be more close to what was before.

diff --git a/Makefile.include b/Makefile.include
index 9f0352f86..dc336ef57 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -430,7 +430,7 @@ endif # RIOTNOLINK
 $(ELFFILE): $(BASELIBS)
        $(Q)$(_LINK) -o $@

-$(BINDIR)/$(APPLICATION_MODULE).a: $(BUILDDEPS) pkg-build
+$(BINDIR)/$(APPLICATION_MODULE).a: pkg-build $(BUILDDEPS)
        $(Q)DIRS="$(DIRS)" "$(MAKE)" -C $(APPDIR) -f $(RIOTMAKE)/application.inc.mk
 $(BINDIR)/$(APPLICATION_MODULE).a: FORCE

[jnohlgard]

👍

[cladmi]

Thanks for reviewing.