-
Notifications
You must be signed in to change notification settings - Fork 1
Change clean to a double-colon rule
#3
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
base: main
Are you sure you want to change the base?
Conversation
This commit changes `clean` to a double-colon to allow local project Makefiles to also define a `clean` rule without overriding the top-level one. A stub for a double-colon `clean` rule is placed into the project Makefile as well. See <https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html> Addresses <INCATools/ontology-development-kit#1284>
|
@gouttegd I imagine that this was the right place to submit this PR, since all the templates were removed as of INCATools/ontology-development-kit@51a4220 |
Yes, absolutely! This is where the standard workflows will be maintained from now on. Thanks, will make some tests and decide later, but as I said on the original ticket I am inclined to adopt the idea. |
|
|
||
| .PHONY: clean | ||
| clean:: | ||
| # Add any custom cleaning logic here. |
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 don’t object to putting a dummy rule in the custom Makefile as a way to let users know that they can add their own custom cleaning logic, but I have two issues here:
(1) The rule should be moved out of the Jinja conditional block that opens on line 187 ( {% if project.import_group is defined -%}), because there’s no reason why that rule should only be added when the project defines a import_group.
(2) The “add any custom cleaning logic here” comment should be moved outside of the recipe. Right now, it is part of the recipe, which means that make will display the comment when the target is invoked:
$ make clean
[ ... normal output of the standard clean rule ... ]
# Add any custom cleaning logic here.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.
My mistake on #1. I saw the endif tag and didn't realize it was nested. I was trying to get the clean rule at the end of the file.
|
Having considered this again in the last few days, I'm not sure it's a great idea. Theoretically it makes sense, but it may be a bit too clever and I don't know how good it would be in practice. If someone has already defined a It's also the case that double-colon rules are pretty obscure, and someone editing their project makefile may delete one of the colons thinking it's a typo. Of course, we could document why it's not a typo, but I think there's a better way around this. A simpler solution for extending the Somewhere in the documentation, a section about extending There wouldn't be any way to enforce that someone call What do you think? Happy to put in the requested fixes for this PR, but also to close it and take another stab. |
|
The question then is- why not just create a project specific clean target? To which I would say-- |
That’s true but I am personally not concerned about that. We have always been clear that, the moment you do anything in the custom (project) Makefile, the promise of a smooth update to a future ODK version no longer holds, and you are expected to read the release notes to check whether your custom rules might need updating – which would be the case here.
I like the fact that this is a more “standard” solution – one that does not rely on “double-colon rules”, which I agree is a rather obscure feature that not all Make users might be readily familiar with.
Mildly against this – the project Makefile is not a place to document feature. If we start putting a commented out rule to illustrate how to have your own cleaning rule, why not start putting commented out rules to illustrate how to make custom mirrors, custom import modules, custom components, and so on?
Fully agree.
Feel free to go ahead with the |
This commit changes
cleanto a double-colon to allow local project Makefiles to also define acleanrule without overriding the top-level one. A stub for a double-coloncleanrule is placed into the project Makefile as well.See https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html
Addresses INCATools/ontology-development-kit#1284