INCATools / ontology-development-kit

Bootstrap an OBO Library ontology
http://incatools.github.io/ontology-development-kit/
BSD 3-Clause "New" or "Revised" License
214 stars 54 forks source link

Schnapsidee: Favour Make conditionals over shell conditionals #912

Open gouttegd opened 11 months ago

gouttegd commented 11 months ago

The standard ODK-generated Makefile contains several instances where the entire recipe of a rule is enclosed in a shell conditional construct (if... then... fi) based on a Make variable, such as in this example:

(TMPDIR)/pattern_owl_seed.txt: $(PATTERNDIR)/pattern.owl
        if [ $(PAT) = true ]; then $(ROBOT) query --use-graphs true -f csv -i $< --query ../sparql/terms.sparql $@; fi

Since what is tested in the condition is a Make variable, not a shell variable, this kind of construct could be replaced by a Make conditional instead:

ifeq ($(PAT),true)
(TMPDIR)/pattern_owl_seed.txt: $(PATTERNDIR)/pattern.owl
        $(ROBOT) query --use-graphs true -f csv -i $< --query ../sparql/terms.sparql $@
endif

I think this would be less surprising to readers, and easier to debug when things go wrong.

We’d need to be careful about prerequisites, though, because the two above constructs are not strictly equivalent: in the first case (shell conditional), even if the recipe does nothing when PAT is false, the rule is still there and its prerequisites are evaluated (so here, Make will find that it needs to build $(PATTERNDIR)/pattern.owl), whereas in the second case (Make conditional), the entire rule is skipped when PAT is false, including its prerequisites. But we should always be careful about prerequisites anyway, whether we use shell or Make conditionals.

This idea would also apply to the various custom.Makefile that exist in the wild, where constructs similar to those in use in the ODK Makefile are also widely used.

matentzn commented 11 months ago

That does not sound like a Schnapsidee at all :D Yeah, I support this. I think this is much better, and the reason it does not exist is simply that when I first introduced that construct, I did not know the make conditionals were a thing!