-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
The rule to rebuild the Makefile.include(s) should be removed at some point.
2029aca
to
2ba8683
Compare
Squashed and rebased. |
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: