Skip to content

Conversation

@ptgolden
Copy link

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

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>
@ptgolden
Copy link
Author

@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

@gouttegd gouttegd self-requested a review January 15, 2026 19:29
@gouttegd
Copy link
Collaborator

I imagine that this was the right place to submit this PR

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.
Copy link
Collaborator

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.

Copy link
Author

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.

@ptgolden
Copy link
Author

ptgolden commented Jan 18, 2026

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 clean: rule in their project makefile, then the next update will raise an arcane error message, since single-colon rules and double-colon rules with the same name cannot both be defined. (This could be added in the upgrading documentation if this PR is merged).

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 clean target may be to rename the base target to odk_clean. By default, an alias target to clean could be added:

.PHONY: clean
clean: odk_clean

Somewhere in the documentation, a section about extending clean could be added. (Also maybe extending, help, which is already possible by echoing the defined help text).

There wouldn't be any way to enforce that someone call odk_clean in their overriding clean, but it would at least give them the option to do so without copying the existing target. There could even be a commented out overriding clean target in the project makefile template to document this.

What do you think? Happy to put in the requested fixes for this PR, but also to close it and take another stab.

@ptgolden
Copy link
Author

The question then is- why not just create a project specific clean target? To which I would say-- make clean is such a common idiom that it would make sense to have that target clean everything, rather than have to do, e.g. make clean uberon_clean

@gouttegd
Copy link
Collaborator

If someone has already defined a clean: rule in their project makefile, then the next update will raise an arcane error message

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.

A simpler solution for extending the clean target may be to rename the base target to odk_clean.

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.

There could even be a commented out overriding clean target in the project makefile template to document this.

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?

The question then is- why not just create a project specific clean target? To which I would say-- make clean is such a common idiom that it would make sense to have that target clean everything, rather than have to do, e.g. make clean uberon_clean.

Fully agree.

What do you think? Happy to put in the requested fixes for this PR, but also to close it and take another stab.

Feel free to go ahead with the odk_clean idea. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants