Skip to content

make: introduce $(CLEAN)#11829

Merged
fjmolinas merged 2 commits intoRIOT-OS:masterfrom
kaspar030:make_add_clean_variable
Jul 12, 2019
Merged

make: introduce $(CLEAN)#11829
fjmolinas merged 2 commits intoRIOT-OS:masterfrom
kaspar030:make_add_clean_variable

Conversation

@kaspar030
Copy link
Contributor

Contribution description

Currently, some targets are serialized before "clean" by conditionally
adding a dependency.

Make does allow ordering using "|" syntax in prerequisites, but for
"clean" that would always trigger the clean target.
By only conditionally setting $(CLEAN) to clean, that can be
circumvented.

The new CLEAN allows any recipe to be run after "clean" by specifying "|
$(CLEAN)" as prerequisite.

Testing procedure

Try make clean all -j and make all -j, ensuring both still work as expected (both with existing and non-existing bin/.

Issues/PRs references

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Jul 12, 2019
@kaspar030 kaspar030 requested review from cladmi and fjmolinas July 12, 2019 09:25
@cladmi
Copy link
Contributor

cladmi commented Jul 12, 2019

Makes sense.

Two changes in one commit though.

Makefile.include Outdated
#
# all: | $(CLEAN)
#
CLEAN=$(filter clean, $(MAKECMDGOALS))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not bash, we use spaces around equals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, fixed

@kaspar030 kaspar030 force-pushed the make_add_clean_variable branch from 94985c8 to a0d653b Compare July 12, 2019 10:35
@kaspar030
Copy link
Contributor Author

Two changes in one commit though.

split in two.

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 12, 2019
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, in #11818 you said:

Found an issue with the key generation targets not being properly serialized before "clean".
Split out a new "CLEAN" variable into #11821

Can you specify that scenario to reproduce and see this fixes it?

Otherwise I tested that make all -j and make clean all -j work as expected.

Makefile.include Outdated
BUILDRELPATH ?= $(patsubst $(RIOTPROJECT)/%,%,$(CURDIR)/)

# Set CLEAN to "clean" if that target was requested.
# Allowis recipes to be run after cleaning, without triggering it implicitly:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Allows

@kaspar030
Copy link
Contributor Author

Can you specify that scenario to reproduce and see this fixes it?

Without this fix, SUIT_KEY_DIR='$(BINDIR)' make clean all -j in #11818 fails (most of the times), as "clean" and "genkey" happens at the same time.

@fjmolinas
Copy link
Contributor

@kaspar030 Thanks, verified this fixes the use case in #11818. Please squash directly once typo is fixed.

Currently, some targets are serialized before "clean" by conditionally
adding a dependency.

Make does allow ordering using "|" syntax in prerequisites, but for
"clean" that would always trigger the clean target.
By only conditionally setting $(CLEAN) to clean, that can be
circumvented.

The new CLEAN allows any recipe to be run after "clean" by specifying "|
$(CLEAN)" as prerequisite.
@kaspar030 kaspar030 force-pushed the make_add_clean_variable branch from a0d653b to 4e051f8 Compare July 12, 2019 13:22
@kaspar030
Copy link
Contributor Author

Please squash directly once typo is fixed.

done!

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@cladmi
Copy link
Contributor

cladmi commented Jul 12, 2019

Note for more advanced | usages.

This correctly works because the target after | is a .PHONY target.
When trying to do this with a | $(BINDIR)/. target for example and a directory creation rule, the handling was more problematic with the -j clean all.

@fjmolinas fjmolinas merged commit 220ab41 into RIOT-OS:master Jul 12, 2019
@kaspar030 kaspar030 deleted the make_add_clean_variable branch July 12, 2019 19:25
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants