Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile.include: Replace PHONY by FORCE #9623

Merged
merged 5 commits into from Jul 25, 2018

Conversation

jcarrano
Copy link
Contributor

Contribution description

Makefile.include abuses PHONY targets. These should be used for non-file targets, but they are currently being used to force re-running of the recipe.

This PR replaces PHONY targets by FORCE and clarifies when they should be used.

References

See the GNU Make manual:

@jcarrano jcarrano requested a review from cladmi July 23, 2018 15:12
@jcarrano jcarrano added the Area: build system Area: Build system label Jul 23, 2018
@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 23, 2018
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

I agree with the technical changes. I also checked that target that previously depended on $(RIOTBUILD_CONFIG_HEADER) depend on FORCE so will be rebuilt as before.

However, I do not really like the new documentation. See inline.

Makefile.include Outdated
# For targets that are not a file, and only for that, use .PHONY. For more
# information, see:
# https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html
# https://www.gnu.org/software/make/manual/html_node/Force-Targets.html
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation does not unfortunately say that FORCE should be use for file targets. I already found that when adding FORCE in this file: #8844

Even if it was not that great, my previous documentation was mentioning the justification with the file timestamp being taken into account with FORCE contrary to .PHONY.
I find it important because it is not clear in the documentation and it explain why you should use FORCE.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be more precise on my thought, you changed to "how it should be used", I was more writing the "why it should be used", and unfortunately the official documentation does not say why. And I think both should be kept so I would prefer if you had a merge of both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The make manual says: "A phony target should not be a prerequisite of a real target file". I take that to mean that if you make a target depend on a phony prerequisite, then that's equivalent to forcing the phony rule and the target's rule to be run anytime. This is what we are doing by defining FORCE as phony and making everything depend on force.

If the file is updated each time the rule runs, it should not change anything. What FORCE gives us is the possibility of choosing not to update the file and then it will not trigger an update in it's dependents.

Some other considerations: you cannot make a pattern rule phony, but you can make it FORCE.

Should I clarify all that in the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the file is updated each time the rule runs, it should not change anything. What FORCE gives us is the possibility of choosing not to update the file and then it will not trigger an update in it's dependents.

That what I explained with the previous documentation. So just keep it I guess and maybe clarify it.

No need to explain everything, keep it simple. Adding your part with "file targets -> FORCE" and "non file -> .PHONY" would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some other considerations: you cannot make a pattern rule phony, but you can make it FORCE.

I correct myself, what the manual says is "The implicit rule search (see Implicit Rules) is skipped for .PHONY targets."

Makefile.include Outdated
# into account.
#
# FORCE is useful for goals that may keep outputs unchanged (for example, if it
# depends on environment or configuration variables). If the goal were PHONY, it
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, replace PHONY by .PHONY but otherwise I like it this way.

Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK please squash the documentation commits

@jcarrano
Copy link
Contributor Author

Squashed and rebased.

@cladmi cladmi merged commit 0fe5ec1 into RIOT-OS:master Jul 25, 2018
@cladmi cladmi mentioned this pull request Jul 25, 2018
1 task
@cladmi cladmi added this to the Release 2018.10 milestone Aug 11, 2018
@jcarrano jcarrano deleted the fix-force-usage branch November 8, 2018 15:20
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants