Merge testsuite sub-makefiles in the root Makefile and harden build system

[shindere]

This PR merges testsuite/{lib,tools}/Makefile into the root Makefile and does two additional perhaps more interesting things:

1. If ocamltest is enabled, all the support libraries and tools required to run certain tests are built, too. This means that any test can then be run by ocamltest directly without having to necessarily go through make to ensure that the required libraries and tools are built. This fulfulls a wish expressed some time ago by @Octachron and I am pleased to offer this modest gift to him. @gasche may also like this, and the coming item, too.

2. The PR makes it impossible to build ocamltest if it has not been enabled at configure time and gives an explicit error message explaining what is going on. This is an attempt to address Test failure when glibc has frame pointers #12587, more precisely a problem it revealed, see e.g. Test failure when glibc has frame pointers #12587 (comment) since if ocamltest is disabled at configuration time, this PR makes is then completely impossible to build it, even manually.

[dra27]

A few suggestions here and there, with a couple of other thoughts:

• Given the integration into the main Makefile, could we just move expect, codegen, testing and "lib" (which could be given a more descriptive name) to tools/ (in the same way that stripdebug, cmpbyt and other parts of the build system also live in tools)?
• I fully support hardening the correct running of the testsuite, but I'm not overly keen on this approach. It is relatively easy to end up working on a tree where ocamltest wasn't built by default, and it's very pedantic and time-consuming to force the user to reconfigure the tree. I would prefer it to be that the test for, say, diff and everything is always done, but is only an error if ocamltest is explicitly requested and, consequently, that attempting to build ocamltest is only an error if it impossible to build.

[dra27]

I don't think this needs to be here - isn't testsuite/tools/expect built as part of BYTECODE_OPTIONAL_TOOLS?

[dra27]

Similarly, isn't this part of the optional tools variable?

[dra27]

As a further optimisation (I think) codegen is only needed if the native compiler is being built

[shindere]

David Allsopp (2023/11/11 06:07 -0800):

* Given the integration into the main Makefile, could we just move expect, codegen, testing and "lib" (which could be given a more descriptive name) to tools/ (in the same way that stripdebug, cmpbyt and other parts of the build system also live in tools)?

This had actually already been discussed in the past, although I can't remember whether the discussion took place online or off-line. At that time, the conclusion was that we wanted to keep the tools and libs related to tests separated from the rest but that decision can of course always be revisited.

* I fully support hardening the correct running of the testsuite, but I'm not overly keen on this approach. It is relatively to end up working on a tree where ocamltest wasn't built by default, and it's very pedantic and time-consuming to force the user to reconfigure the tree. I would prefer it to be that the test for, say, diff and everything is always done, but is only an error if ocamltest is explicitly requested and, consequently, that attempting to build ocamltest is only an error if it impossible to build.

David and I discussed this point intensively offline. My understanding of the conclusion is that, for the time being, the fact that there is an error hinting the user on what to do is already an improvement over te previous situation. Ideally, though, in the case of ocamltest (and only in that case), it seems desirable to make it possible to build it although it has been disabled at configure time (provided it's actually possible to do so), so that workflows like `./configure && make -- make test` work in any case. That means that if ocamltest is enabled then it will be build during the main build, but if it's not, then it will be build as part of `make test`. At the moment this is non-trivial to implement so we think it's best to wait until `testsuite/Makefile` gets merged into the root Makefile too to implement this feature. I'll let @dra27 comment on whether this summary of our discussion is accurate.

In testsuite/lib/testing.ml: > let failure_test f x s = test_raises_this_failure s f x;; -let any_failure_test = test_raises_some_failure;; +(* let any_failure_test = test_raises_some_failure;; *) Instead of commenting this out, why not expose it in the .mli?

I did so to mark that this bit of code is currently unused. Given this explanation, what do you prefer? Shall I leave it as it is or expose it?

In testsuite/lib/lib.mli: > @@ -0,0 +1,57 @@ +(**************************************************************************) +(* *) +(* OCaml *) +(* *) +(* Sebastien Hinderer, Tarides *) FWIW I'd use the copyright header of lib.ml, given that the file is largely identical

OK, Copyright adjusted

In Makefile: > @@ -1874,6 +1903,11 @@ partialclean:: rm -rf $(ocamltest_GENERATED_FILES) rm -f ocamltest/ocamltest.html rm -f $(addprefix testsuite/lib/*.,cm* o obj a lib) + rm -f $(addprefix testsuite/tools/*.,cm* o obj a lib) + rm -f testsuite/tools/codegen testsuite/tools/codegen.exe + rm -f testsuite/tools/expect testsuite/tools/expect.exe + rm -f testsuite/tools/lexcmm.ml # Documentation The "# Documentation" looks like an editing artefact?

Indeed! Might be the one a few lines down which get copied I don't know how. Fixed.

In Makefile: > @@ -1832,7 +1833,7 @@ $(ocamltest_DEP_FILES): $(DEPDIR)/ocamltest/%.$(D): ocam ltest/%.c ocamltest/%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS) ocamltest: ocamltest/ocamltest$(EXE) \ - testsuite/lib/lib.cmo testsuite/lib/testing.cma + testsuite/lib/lib.cmo testsuite/lib/testing.cma testsuite/tools/expect$(EXE) I don't think this needs to be here - isn't testsuite/tools/expect built as part of BYTECODE_OPTIONAL_TOOLS?

You are right that there is some redundancy here that needs to be addressed, many thanks for having spotted it. I am not sure yet how to best address it though, i.e. whether I should remove the dependency, or rather adjust `configure.ac`. One interesting thing to notice about `codegen` is that, at least for the time being, it's a bytecode tool but that is used only for native tests. Its situation thus looks a bit odd to me. Wouldn't it make things a bit clearer if we would build it as a native tool only, and then only when the native compiler is enabled? Then of course the question of how to build it remains. Ot me, having the tools as dependencies of ocamltest looks slightly more meaningful than adding them to generic list of tools. What's your take?

In Makefile: > ocamltest/ocamltest$(EXE): OC_BYTECODE_LINKFLAGS += -custom ocamltest/ocamltest$(EXE): ocamlc ocamlyacc ocamllex -ocamltest.opt: ocamltest/ocamltest.opt$(EXE) testsuite/lib/testing.cmxa +ocamltest.opt: ocamltest/ocamltest.opt$(EXE) \ + testsuite/lib/testing.cmxa $(asmgen_OBJECT) testsuite/tools/codegen$(EXE) Similarly, isn't this part of the optional tools variable?

I see the response as a matter of taste, as I tried to explain above, but let's see.

In [6]configure.ac: > @@ -2314,7 +2319,8 @@ AS_CASE([$enable_ocamltest,OCAML__DEVELOPMENT_VERSION], ocamltest_target=ocamltest ocamltest_opt_target=ocamltest.opt ocamltest='ocamltest' - ], + testsuite_tools='testsuite/tools/codegen testsuite/tools/expect' As a further optimisation (I think) codegen is only needed if the native compiler is being built

Yes, I think it's what I tried to do by adding `testsuite/tools/codegen$(EXE)` as a dependency of `ocamltest.opt` in the root Makefile but perhaps that's not clean enough?

[shindere]

Giving it a second thought, I believe that the code in this PR is actually correct as it is.

More specifically, regarding the discussions on where we should list the
libraries and tools to be built, the lists in configure.ac and the
dependencies in Makefile accomplish two different purposes. The fact that
the libraries and tools are listed in configre.ac is useful so that the
macros for building programs and libraries get called. But these macros
being called is not enough per se for the corresponding libraries and tools
to be actually built. The build will become effective only if the libraries
and tools appear as prerequisites of other rules, which is what is done in
the Makefile.

[shindere]

I just pushed an update that takes into account @dra27's remark about
the Testing library module.

With this, I hope I have addressed all the review comments done so far.

[dra27]

LGTM!

[shindere]

Thanks! And thanks for the careful and supportive review, as usual!