Very nice fix!

If ocamlc.opt is older than ocamlc then it should not be used silently, because it may be stale (make world.opt, then hack on the compiler, then just make core, then make ocamldoc, the changes to the compiler should be taken into account. In that case I plan to emit a warning and revert to the most-recently-built tool (the bytecode one).

The warning cannot be observed in a cold-build scenario, and it should not be observable when running make world.opt several times, but it could be observed if rebuilding from an existing build (so mostly during development). It will mention the two ways to silence the warning, either to remove the stale native tools, or to touch them to use them.

I have implemented the newer-than check (it's a Make function rather than a shell script to easily make use of Make's $(info ...) printing function, but Make functions are not very pleasant to write or read). A warning such as the following is displayed if, for example, ocamlc is newer than ocamlc.opt:

Warning: we are not using the native binary ocamlc.opt because it is older than the bytecode binary ocamlc; you should silence this warning by either removing ocamlc.opt or rebuilding it (or touch-ing it) if you want it used.

I have implemented the newer-than check (it's a Make function rather than a shell script to easily make use of Make's $(info ...) printing function, but Make functions are not very pleasant to write or read).

I applaud the effort (of using make functions) but dislike the smells of scripting (using eval, passing variables by reference, escaping $). Here is a more functional version that almost looks like Lisp code:

check_not_stale = \
  $(if $(shell test $(ROOTDIR)/$1 -nt $(ROOTDIR)/$2 && echo stale), \
    $(info Warning: we are not using the native binary $2 \
because it is older than the bytecode binary $1; \
you should silence this warning by either removing $2 \
or rebuilding it (or `touch`-ing it) if you want it used.), \
    ok)

choose_best = \
  $(if $(and $(wildcard $(ROOTDIR)/$2), $(strip $(call check_not_stale,$1,$2))), \
    $(ROOTDIR)/$2, \
    $(ROOTDIR)/$1)

BEST_OCAMLC := $(call choose_best,ocamlc,ocamlc.opt)
BEST_OCAMLOPT := $(call choose_best,ocamlopt,ocamlopt.opt)
BEST_OCAMLLEX := $(call choose_best,lex/ocamllex,lex/ocamllex.opt)

Very nice! I pushed a patch with your version. (In the bytecode case we need to prefix with $(CAMLRUN).)

I had thought of doing something like that (without the assignment) but I couldn't find a way that would (1) statically make the choice at Makefile-invocation time (I'm worried of the performance cost of checking at each compiler invocation) and (2) keep using the = assignment that allows other Makefiles to redefine CAMLRUN and get away with it. This is one reason why I used eval, which allows to statically compute the code to run dynamically -- staged programming!

In fact I checked, and CAMLRUN is not redefined anywhere in this style (some other variables use redefinitions and lazy expansions, in particular the CAML{C,OPT} and CAML{C,OPT}_CMD pairing) -- it is sometimes redefined on recursive invocation of Make (to promote, for example) but that should be fine. So we don't need to keep (2) and can use eager assignment with your version.

(cc @shindere this is the other Makefile-oriented PR that I mentioned.)

I addressed the review comments of @shindere -- I have a one-argument instead of two-argument function now. ocamldoc is not using this mechanism because it is not generic enough -- the invocation command for ocamldoc is fairly complex due to dynamic linking.

I think it's good to have a mechanism where these definitions must be explicitly opted into. The alternative would be to have them in Makefile.common but have a variable which can be defined before Makefile.common is included to enable them.

Personally I think having the Makefile.best_binaries as it is better

Personally I think having the Makefile.best_binaries as it is better

It does not really scale...

I'm not sure what sort of scalability you have in mind here, is it the scalability of ls at the root? When we write programs, we tend to consider that it is good style to split distinct part of the codebase in different files, instead of having everything in a single source file.

We could think of moving the sub-Makefiles into tools/ for example, or a new makefiles/ directory (as we used to have in the testsuite).

I don't see the problem. If you forget to include a makefile that provides definitions/variables, then your makefile code fails and you quickly notice the problem. On the other hand, having a monster Makefile that defines everything at once makes it harder to reason about, for example, which definitions are or are not in scope at a given point. (For example, everytime you add a definition there, you have to check all Makefiles including it, which is all makefiles, for naming conflicts.)

Could we try to make faster progress on this PR? Saving 40s to a minute of compiler full-build time seems like an important quality-of-life improvement for developers (and could also help CI response times), it's disappointing to wait for two months to make a decision.

Thanks! (I'm sorry for my sprout of grumpiness now.)

I had just rebased the PR to fix the check-typo issue, so we should wait for CI.

I took no grumpiness 🙂 - feel free to merge when AppVeyor comes back

I presume as you rebased you'd like to keep the commit history?

I just merged, thanks everyone for the feedback.