make: `make clean all` does not make sense and should be removed

[jcarrano]

Description

What does make clean all mean? How can one create a target and clean it at the same time? The answer is that it does not make sense. This ill-defined command is used extensively in RIOT.

It could seem (if one has only a superficial understanding of make) that make clean all means make clean followed by make all. Well, that is not how make works, and if it behaves like that in certain circumstances it is only accidental.

What make does is execute all recipes needed to make the required targets, in any order, as long as dependencies are satisfied. In the case of parallel make ("-j") recipe execution may be simultaneous.

The reason make clean all works at all is because of hacks that make all depend on clean. It is a hack because under normal circumstances all does not depend on clean.

Why is make clean all even used

See also: this comment

Normally, one would type make clean followed by make all. The problem is that in the current implementation, the overhead of starting make is too big. This is caused mainly by the extensive use of immediate assignments (:=) combined with $(shell ...) which means there are lots of commands run on each time make is invoked.

On a big CI build, this overhead adds up and cleaning ends up taking considerable time. For this reason a hack was devised to make it possible to run both targets in a single invocation.

Steps to reproduce the issue

This issue is hard to reproduce, as it involves parallelism (and non-deterministic behaviour). For some examples, see

• examples/{javascript/lua}: correctly declare rules to embed scripts. #10456 Removing a (seemingly) unrelated rule caused builds to start failing.
• Makefile.include: remove rule to remake PKGs' Makefile.include #10344 This is the rule that got removed.
• Makefile.include: building in docker should be done after clean  #10461 It also complicates docker integration.
• mcuboot.mk: Add mcuboot targets to BUILDDEPS. #10492 I thought I fixed it, but it turns out it is necessary to add conditional dependencies on "clean" for some targets.

Steps to fix

Removing all the code needed for make clean all, and banning it's usage will make our build system simpler and saner.

For this we must attack the root cause: high overhead, caused by unnecessary evaluations of variables containing costly shell invocations. The good news is that that overhead can be fixed.

On the other hand, in the case where those variables are needed, they should be evaluated only once.

Another cause of unnecessary evaluations is the indiscriminate use of "export" statements (see #10440 for an example).

[jnohlgard]

What do we gain from removing the hack which makes all (and flash) depend on clean?
What is you use case?
The hack is convenient since it let's us make a clean build when developing and maybe messing with dependencies etc. It also redu es maintenance burden because we do not need to write answers to users asking why their link failed because of missing files after a clean build. Many other projects also support running make clean all, so it is kind of counterproductive to remove something that is a helpful feature and not particularly intrusive, even if it is a hack.

What are the hacks other than this part of makefile.include?

https://github.com/RIOT-OS/RIOT/blob/master/Makefile.include#L663-L668

Some of the shell invocations will be very difficult to move or delete. One big problem with using only make, as opposed to using a makefile generator, such as CMake or GNU autotools, is that basic toolchain checks need to be performed for each build instead of only once, testing the existence of various tools, building application dependency information etc. We can try to streamline this as good as possible, but without any persistent state cache between runs there will always be significant overhead for each make run. Both clean and all require a full dependency calculation to know which files are used, unless clean is modified into the simpler and more dangerous rm -f *.o.

[kaspar030]

For this we must attack the root cause: high overhead, caused by unnecessary evaluations of variables containing costly shell invocations.

Whatever makes "make clean; make all" faster will also make "make clean all" faster.
If "clean all" wasn't possible, and someone would PR a hack that allows it, promising 50% improvement in build times, we'd happily merge it.

Removing all the code needed for make clean all, and banning it's usage will make our build system simpler and saner.

Maybe slightly, but also much slower.

The root issue is that our recursive make actually requires a clean in order to produce consistent output.

[jcarrano]

If "clean all" wasn't possible, and someone would PR a hack that allows it, promising 50% improvement in build times, we'd happily merge it.

And that's how you bury yourself deeper and deeper into the tar pit.

[cladmi]

Just found out while testing #10344 that the clean all in parallel ack does not currently handle the docker integration.

[kaspar030]

parallel ack

Whats a parallel ack?

[cladmi]

The clean all is a hack when building in parallel only. Without parallel build there is no issue with doing clean all.

[cladmi]

The hack is convenient since it let's us make a clean build when developing and maybe messing with dependencies etc. It also redu es maintenance burden because we do not need to write answers to users asking why their link failed because of missing files after a clean build. Many other projects also support running make clean all, so it is kind of counterproductive to remove something that is a helpful feature and not particularly intrusive, even if it is a hack.

The main goal is that only make all is required even on a dirty build directory.
This way, no-one ever runs make clean all.
We are getting the infra to allow debugging it.

Some of the shell invocations will be very difficult to move or delete. One big problem with using only make, as opposed to using a makefile generator, such as CMake or GNU autotools, is that basic toolchain checks need to be performed for each build instead of only once, testing the existence of various tools, building application dependency information etc.

The main issues is that in many places, a immediate evaluation is done.
So even when doing make clean, your gcc version is verified and also check if it supports -fno-delete-null-pointer-checks.
The solution is what I did in your PR getting PORT using the list-ttys, and we are working towards migrating these.
This also causes bugs right now with INCLUDES being evaluated immediately in newlib.mk that can use wrong CFLAGS_FPU value.

It is just a matter of only using things when needed, so declaring them with deferred variables and not export everything every time even when not needed, as this forces evaluation.
The next step that the value is cached between two builds is a further optimization, but also in our targets.

The main problem is that as some point, it as become the default to export everything even if not needed and use := for variable by default.
So there is a lot of thing to fix.

We can try to streamline this as good as possible, but without any persistent state cache between runs there will always be significant overhead for each make run.

It is in our goals and will soon have the infrastructure to work on it. There is the introduction on a common build directory that is a first step towards this.

[cladmi]

To be precise, the hack is not the one you listed as it is the one when building for a different RIOT version (is it even used and tested ?), but it is this one:

RIOT/Makefile.include

Lines 484 to 487 in 29acd40

	# The `clean` needs to be serialized before everything else.
	ifneq (, $(filter clean, $(MAKECMDGOALS)))
	all $(BASELIBS) $(USEPKG:%=$(RIOTPKG)/%/Makefile.include) $(BUILDDEPS): clean
	endif

Where all has no reason to be there as it does nothing for the problem.

[cladmi]

Things that do not handle doing clean all -j:

• examples/{javascript/lua}: correctly declare rules to embed scripts. #10456 file as a dependency from RIOTBIULD_CONFIG_HEADER (merged)
• Makefile.include: building in docker should be done after clean  #10461 docker integration

[jcarrano]

I updated the list with #10492.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.