Further simplifications to the flexlink bootstrap

[dra27]

OCaml has been able to bootstrap FlexDLL at the same as the compiler since 4.03 (#388). It got quite a big overhaul in 4.13 (#10135). This PR picks up some of the "for the future" comments from that latter PR, and in particular removes a lot of the "magic" about the bootstrap.

The fundamental complexity arises from the twin facts that linking ocamlrun.exe (a C program) requires flexlink.exe (an OCaml program) and that the compiler expects to find flexlink.exe in PATH (just like it does for the C compiler). The solution to the first part works by building a temporary runtime which doesn't support shared libraries, as this version of ocamlrun.exe can be compiled with the standard linker. #388 then threw this temporary runtime away where #10135 acts a little more intelligently and introduced boot/ocamlruns.exe which is used during the build to call a bytecode-compiled version of flexlink.exe.

The solution to the second part works by a hack in the Config module which reads the OCAML_FLEXLINK environment variable and, if it is defined, redefines mkexe, mkdll and mkmaindll values in terms of that environment variable instead of just "flexlink". This allows the build system both to use a qualified path and also to be able to specify a bytecode runtime (i.e. to replace calls to flexlink in those variables to calls with C:\Devel\ocaml\boot\ocamlruns.exe C:\Devel\ocaml\boot\flexlink.byte.exe).

This environment variable is also set when running the testsuite, again so that the compiler invocations in the tests use the tree-compiled version of flexlink. However, ocamltest also itself makes use of mkexe and mkdll and so it too has a mechanism to allow those values to be overridden. Thus the testsuite logic overrides three environment variables before ever calling ocamltest.

It's a bit of a mess, but if one winds back to the "state" of the build system in OCaml 4.03 (Windows and Unix had separated build systems; neither ocamltest nor even Makefile.common had been invented yet), it made sense at the time to do it this way.

However, it's now a bit of a wart - especially as OCAML_FLEXLINK is an exposed environment variable in the compiler itself.

This PR still involves an environment variable trick, but it's considerably simpler and, thanks to Makefile.common now being read throughout the build system, only has to be done once. When bootstrapping flexdll, we create the directory byte/bin and ensure that make exports it at the start of PATH. There's a small piece of trickery to ensure that the recursive invocations of make don't add it more than once. Beyond that, it gets plumbed in as a dependency at all the required points. In fact, two directories get added to PATH - opt/bin gives a place from which to plumb in flexlink.opt. This means that, by natural shell invocation, as soon as the faster flexlink.opt is available, it gets executed instead.

Now, instead of having to plumb an override value all around the build system, the compilers and ocamltest simply execute flexlink as normal, and the fastest one in the tree is picked up automatically.

The second commit is a smaller simplification. #10135 preserved the separate boot/ocamlruns (no DLL support) and boot/ocamlrun (with DLL support) distinction, but this is unnecessary. It is impossible to use shared libraries with the boot artefacts (indeed, #11149 made this a hard error, on purpose). The build system therefore does not need to care if boot/ocamlrun does or does not support shared libraries, so the distinction (and the copying of an extra runtime on Windows) is eliminated.

The longer-term for flexlink is twofold. The first part of the plan is to stop having flexlink pretend to be a linker - i.e. to encode build steps to produce the required descriptor objects instead of the final binary and then include those objects in a normal linker invocation. This will massively simplify the linking story, as at present ocamlopt (and ocamlc) and the build system have to worry about whether the "linker" is actually a C compiler, actually a linker or actually a program pretending to be a linker and definitely not a C compiler. The second part of the plan would be to allow that step to be available as a library which could be compiled directly into compiler-libs, thus removing the installation need for flexlink completely. However, that's a chunk of work, and it requires a lot of testing of opam packages, etc. to ensure things don't break - I think the simplification in this PR is a good simplification for now and hopefully the amount which gets deleted as a result of this change (in utils/config.generated.ml.in and testsuite/Makefile) rather speaks for itself.

There were two alternate mechanisms for doing this, and I mention them in order to explain why they weren't adopted:

• The compiler's ocaml_compiler_internal_params mechanism could have been used to feed the alternate flexlink command. There are three problems with this. Firstly, ocaml_compiler_internal_params is at present read from the wrong place (separate fix for that at some point soon). Secondly, it doesn't simplify much - there's still a lot of processing needed in both Config and in the testsuite. Thirdly, it becomes very fiddly to tie this change in Config to affect only the compiler (i.e. so that users of compiler-libs don't suddenly start trying ocaml_compiler_internal_params files). Basically, it seemed tempting, but the reality is a bit of a mess.
• Add an additional command line flag instead of the environment variable (i.e. something like -ld to complement -cc). This was similarly tempting because it's quite explicit. For OCaml's build system, it doesn't even require a new flag, as we never use ocamlc or ocamlopt to compile and link simultaneously, which means we can just override -cc at link time. However, that means that the testsuite has to be aware of this - including tests which explicitly call the compiler. At this point, we simply trade one internal mess for a very developer-visible and confusing mess in the testsuite (especially as the mistakes would only be visible in Windows testing).
• The PATH trick presented here seems therefore the best compromise - manipulating PATH this way is a properly supported GNU make thing to do (it even has Windows handling for it, dealing with both the alternate separator and the different case of the environment variable internally...) and it means that the compiler is tested in exactly the way that it would be run.

[shindere]

Not much to say here because the review has been reviewed in a quite in-depth way off-line. It will require a rebase though to resolve the conflict in Makefile.

[dra27]

Thanks, @shindere - and rebased.

[shindere]

Note to all: this PR has been reviewed offline quite deeply already so it shouldn't be any surprise that there are not toomany comments here. Here are a few remarks, though: The `FLEXDLL_C_SOURCES` variable is used only once so perhaps it would be as simple to replace its only use by its definition? Regarding this line in `Makefile`: ``` $(BYTE_BINDIR)/flexlink$(EXE): flexlink.byte$(EXE) | $(BYTE_BINDIR) ``` One question and one suggestion: Question: given that the recipe starts by copying `boot/ocamlrun$(EXE)` to the target, would it be worth adding `boot/ocamlrun$(EXE)` as a dependency of the rule? Suggestion: use `$<` in the `cat` command: ``` cat $< >> $@ ``` Also, currently the root `Makefile` contains two instances of: ``` ifeq "$(BOOTSTRAPPING_FLEXDLL)" "false" ``` Given that these two instances have `else` branches and that there already are three instances where `BOOTSTRAPPING_FLEXDLL` is compared to `true` I'd like to suggest that the two remaining instances are written the same way, i.e. to invert the `ifeq` and `else` branches. Beware that the second instance finishes with a comment, after the `endif`, which will need to be fixed if this suggestion is taken into account. Suggesiton: ``` # The recipe for runtime/ocamlruns$(EXE) directly produces runtime/primitives ``` How about replacing `directly` by `also`? In the `flexlink.byte$(EXE):` target, I am curious on why it is necessary to define `OCAMLOPT` whereas at the same time we say that `NATDYNLINK=false`? I realise this line has not been added by this PR but I am taking the opportunity to ask the question. The command that installs `flexlink.byte$(EXE)` could be simplified by not repeating `flexlink.byte$(EXE)`, as is already done for the `opt` version, actually. I'll finish with two requests that are merely a matter of taste. It is confusing for me when a line's indentation is more than 40 characters because, since I can see only the first 40 characters at once, the line looks empty to me until I move the braille window to the right. So, would it be possible to avoid such indentations, please? this PR does introduce two of those: one in the definition of the prerequisites of the `flexlink.opt$(EXE):` target in the root Makefile and one in the definition of `ROOT_FROM` in `Makefile.common`. I realise this is to preserve vertical alignment but could we please change the layout of these so that the indentend lines start earlier, for instance: ``` flexlink.opt$(EXE): \ $(FLEXDLL_SOURCES) | $(BYTE_BINDIR)/flexlink$(EXE) $(OPT_BINDIR) ``` And: ``` ROOT_FROM = \ $(subst $(SPACE),/,$(patsubst %,..,$(subst /, ,$(strip \ $(patsubst %/,%,$(1)))))) ``` (not so sure about the second proposal, though) And finally, at the risk of repeating myself, let me say how much I like this PR. I reviewed it once more and it's very obvious that it simplifies many areas of the configure and build system.

[dra27]

$(BYTE_BINDIR)/flexlink$(EXE): flexlink.byte$(EXE) | $(BYTE_BINDIR)

One question and one suggestion:

Question: given that the recipe starts by copying boot/ocamlrun$(EXE) to the
target, would it be worth adding boot/ocamlrun$(EXE) as a dependency of the
rule?

That's done - but, having done that:

Suggestion: use $< in the cat command:

        cat $< >> $@

I think this then isn't quite so good because you there are two dependencies and they both need referring to individually. In the absence of $> to mean the last dependency, the most automatic-variable-based solution was:

$(BYTE_BINDIR)/flexlink$(EXE): \
  boot/ocamlrun$(EXE) flexlink.byte$(EXE) | $(BYTE_BINDIR)
	rm -f $@
	cp $< $@
	cat $(filter-out $<, $^) >> $@

and I worried for @gasche's health with such a recipe!

In the flexlink.byte$(EXE): target, I am curious on why it is
necessary to define OCAMLOPT whereas at the same time we say that
NATDYNLINK=false? I realise this line has not been added by this PR
but I am taking the opportunity to ask the question.

It's entirely non-obvious, and all to do with "hacking" flexdll's build system into the tree. flexdll's Makefile is not designed to build a bytecode version of flexlink.exe so it gets tricked into doing so by overriding OCAMLOPT. However, for linking in its resource file, flexlink does have to worry about how to specify the linker flag (because the compiler may also call flexlink!) and it uses NATDYNLINK as an indicator for that (cf. this fragment):

ifeq ($(NATDYNLINK), false)
#when ocaml is not built with flexlink i.e. -no-shared-libs
LINKFLAGS = -cclib "$(RES)"
else
LINKFLAGS = -cclib "-link $(RES)"
endif

And finally, at the risk of repeating myself, let me say how much
I like this PR. I reviewed it once more and it's very obvious that it
simplifies many areas of the configure and build system.

Thank you! I think that after a mere 14 releases of the compiler, it's now probably at its most optimal, short of actually pulling the sources into the tree...

[shindere]

Many thanks for the clarification, @dra27. As discussed, this is now going through [precheck](https://ci.inria.fr/ocaml/job/precheck/849/). If precheck is happy then I think this can be merged, thus approving.

[shindere]

Precheck seems happy but a rebase is now necessary to resolve the conflict
in Changes.