Merge ocamltest/Makefile into the root Makefile

[shindere]

This continues the work started earlier.

At the moment, just a draft.

[dra27]

Looking good! There's a few things which may be worth double-checking as we move this Makefile in (the -opaque bits and so forth) and this is possibly "blocked" on a decision for #12467 (which I'm just looking at next).

[shindere]

David Allsopp (2023/08/21 08:04 -0700):

In [2]configure.ac: > @@ -2258,7 +2258,11 @@ AS_CASE([$enable_ocamltest,OCAML__DEVELOPMENT_VERSION], [ocamltest='']) AS_IF([test -n "$ocamltest"], - [AC_CHECK_PROGS([DIFF], [patdiff diff], [none]) + [ocamltest_unix_mod="ocamltest/ocamltest_unix_${ocamltest_unix_impl}.ml" As this is now being done internally, I expect the AC_SUBST for ocamltest_unix_impl can be removed?

Yes absolutely, thanks. Well spotted. Fixed.

In [3]Makefile: > +# because its build involves calling expunge. We thus give its build +# rules explicitly until expunge can hopefully be removed. I don't think there's an actual plan for this (i.e. we'd like not to need to expunge, but no one has a proposal for what we'd do?), so perhaps the second sentence should be omitted?

Well. Does the comment harm? I see it a bit like a reminder that it's our wish, although we cdon't have a concrete plane to make that wish come true now.

In [4]Makefile: > @@ -1279,21 +1297,22 @@ otherlibs/dynlink/dynlink.cma: otherlibraries debugger/%: VPATH += otherlibs/unix otherlibs/dynlink -ocamldebug_COMPILER_MODULES = $(addprefix toplevel/, genprintval topprinters) +ocamldebug_COMPILER_SOURCES = $(addprefix toplevel/, \ + genprintval.mli genprintval.ml topprinters.mli topprinters.ml) FWIW, it does read nicely having each module on a line on its own above, so maybe do that routinely?

So far I have done the change only in one place because I double-checked and in other places where it's not done that way already it's because the whole variable definition fits on one line. For instance, ``` ocamlc_SOURCES = driver/main.mli driver/main.ml ``` It felt a bit pedantic to write this under the form: ``` ocamlc_SOURCES = \ driver/main.mli driver/main.ml ``` (on two lines rather than one)

In [5]Makefile: > # The modules listed in the following variable are packed into ocamldebug.cmo -ocamldebug_DEBUGGER_MODULES = $(addprefix debugger/,\ +ocamldebug_DEBUGGER_SOURCES = $(addprefix debugger/,\ How come these files aren't listed explicitly as the other _SOURCES variables? From a consistency perspective, that feels slightly better - the ocamldebug_DEBUGGER_OBJECTS could presumably filter out .mli before converting the extensions to .cmo?) - e.g. something like ocamldebug_DEBUGGER_OBJECTS = $(patsubst .ml%,.cmo, $(filter-out %.mli, $(ocamldebug_DEBUGGER_SOURCES))?

I think I didn't do such a change right from the beginning because, countrary to the other changes, it does not allow any gain in terms of ability to take advantage of the facilities defined in `Makefile.common`, so basically it makes the code longer but with no other counterpart than coherency. But I now did that change and, why not after all.

In [6]Makefile: > tools/ocamldep$(EXE): OC_BYTECODE_LINKFLAGS += -compat-32 # The profiler ocamlprof_LIBRARIES = -ocamlprof_MODULES = \ - config build_path_prefix_map misc identifiable numbers arg_helper \ - local_store load_path clflags terminfo warnings location longident \ - docstrings syntaxerr ast_helper camlinternalMenhirLib parser \ - lexer pprintast parse ocamlprof - -ocamlcp_ocamloptp_MODULES = \ - config build_path_prefix_map misc profile warnings identifiable numbers \ - arg_helper local_store load_path clflags terminfo location ccomp compenv \ - main_args ocamlcp_common +ocamlprof_SOURCES = \ + config.mli config.ml \ This feels strange that the compiler sources don't have directories - although the proposals with ocamlcommon-private.cma, etc. would do away with these lists completely.

I agree it's odd. I chose the conservative way. Shall I add the directories or leave those untouched until a progres happens on the ocamlcommon-private.cma front? Happy with either way.

In [7]Makefile: > # To help building mixed-mode libraries (OCaml + C) ocamlmklib_LIBRARIES = -ocamlmklib_MODULES = config build_path_prefix_map misc ocamlmklib +ocamlmklib_SOURCES = $(addsuffix .ml,\ + config build_path_prefix_map misc ocamlmklib) There is ocamlmklib.mli

Added, thanks. This variable definition remains odd because it includes compiler modules so a decision remains to be made on whether we leave this the way it is for the time being or make the module lists more explicit also for compiler modules. I'm again fine with either way.

In [8]Makefile: > @@ -1893,9 +1957,9 @@ endif compilerlibs/*.cmxa compilerlibs/*.$(A) \ "$(INSTALL_COMPLIBDIR)" $(INSTALL_DATA) \ - $(ocamlc_MODULES:=.cmx) $(ocamlc_MODULES:=.$(O)) \ - $(ocamlopt_MODULES:=.cmx) $(ocamlopt_MODULES:=.$(O)) \ - $(ocaml_MODULES:=.$(O)) \ + $(ocamlc_CMX_FILES) $(ocamlc_CMO_FILES:.cmo=.$(O)) \ It reads very strangely having ocamlc_CMO_FILES as the source for .o names - wouldn't it be better to use ocamlc_CMX_FILES for this (it'll be the same, it just looks odd).

Yes, okay, good idea, thanks.

Equally, how about having ocamlc_OBJ_FILES?

Do you mean in the general build framework here? That is, adding a new variable for .o/ .obj files and calling it foo_OBJ_FILES in `Makefile.commo`, right?

In [9]Makefile.common: > +# bytecode and native programs. The two next macros, OCAML_BYTECODE_PROGRAM +# (resp. OCAML_NATIVE_PROGRAM) are used to define programs that are +# provided only in bytecode (resp. native) form. "The next two macros, OCAML_BYTECODE_PROGRAM and OCAML_NATIVE_PROGRAM are used to define programs that are provided only in bytecode or native form, respectively. Programs provided in both forms should use OCAML_PROGRAM."

OK I updated the text, thanks.

In [10]Makefile.common: > + $$$$($(basename $(notdir $(1)))_MLI_FILES) \ + $$$$($(basename $(notdir $(1)))_ML_FILES) I wonder if it would be better to do this with _SECONDARY_FILES, rather than explicit adding all sources to beforedepend?

I think it would indeed be better yes, thanks. I did the change, whichresults in slightly more concise and less repetitive code.

In [11]Makefile.common: > LINK_BYTECODE_PROGRAM =\ $(CAMLC) $(OC_COMMON_LINKFLAGS) $(OC_BYTECODE_LINKFLAGS) -define OCAML_BYTECODE_PROGRAM +# The _OCAML_BYTECODE_PROGRAM macro defines a bytecode program but assuming +# that _OCAML_PROGRAM_BASE has already been called. its public counterpart +# does not rely on this assumption and rather does clall _OCAML_PROGRAM_BASE Typo: s/clall/call

Fixed, thanks.

In [12]Makefile: > + +ifeq "$(COMPUTE_DEPS)" "true" +include $(addprefix $(DEPDIR)/, $(ocamltest_C_FILES:.c=.$(D))) +endif + +ocamltest_DEP_FILES = $(addprefix $(DEPDIR)/, $(ocamltest_C_FILES:.c=.$(D))) + +$(ocamltest_DEP_FILES): | $(DEPDIR)/ocamltest + +$(DEPDIR)/ocamltest: + $(MKDIR) $@ + +$(ocamltest_DEP_FILES): $(DEPDIR)/ocamltest/%.$(D): ocamltest/%.c + $(V_CCDEPS)$(DEP_CC) $(OC_CPPFLAGS) $(CPPFLAGS) $< -MT '$*.$(O)' -MF $@ + +beforedepend:: $(ocamltest_MLI_FILES) $(ocamltest_ML_FILES) Isn't this handled by the generic macros now?

It is yes, but I didn't pay attention because this part of the generic macros was commented out. Thanks. Fixed.

In [13]Makefile: > -ocamltest.opt: ocamlc.opt ocamlyacc ocamllex - $(MAKE) -C ocamltest allopt +# Libraries ocamltest depends on + +ocamltest_LIBRARIES = $(addprefix compilerlibs/,ocamlcommon ocamlbytecomp) \ + $(addprefix $(ocamltest_unix_path)/,$(ocamltest_unix_name)) These two variables can be merged as part of this, I think? Either both variables are empty strings or they are combined

Done so, thanks. I also removed the \ ocamltest_` prefix in the name ofthe variable as I believe this has nothing specific to ocamltest any longer and will be usableelseewhere in the build system as themakefiles get merged and the need for the `$(ROOTDIR)` bit vanishes. So the new variable is called `unix_library` and the former `ocamltest_unix_include` variable is replaced by `unix_directory` which avoids having to deal with the `-I` bit explicitly.

In [14]Makefile: > +beforedepend:: $(ocamltest_MLI_FILES) $(ocamltest_ML_FILES) + +ocamltest/%: CAMLC = $(BEST_OCAMLC) $(STDLIBFLAGS) + +ocamltest: ocamltest/ocamltest$(EXE) + +ocamltest/ocamltest$(EXE): OC_BYTECODE_LINKFLAGS += -custom + +ocamltest/ocamltest$(EXE): ocamlc ocamlyacc ocamllex + +ocamltest.opt: ocamltest/ocamltest.opt$(EXE) + +ocamltest/ocamltest.opt$(EXE): ocamlc.opt ocamlyacc ocamllex + +ocamltest/ocamltest_unix.%: \ + OC_COMMON_COMPFLAGS += $(ocamltest_unix_include) -opaque I'm not sure that we still need the -opaque here. In fact, I'm not totally sure why it was originally necessary

Me neither. I thought it was added by you and I think at the time I just trusted it was agood idea. We can remove it if you like?

In [15]Makefile.common: > +#beforedepend:: \ +# $$$$($(basename $(notdir $(1)))_MLI_FILES) \ +# $$$$($(basename $(notdir $(1)))_ML_FILES) Why's this commented out?

Perhaps I had to for debugging at some point and forgot to restore it but it is restored now. With this, we reach the property that the last commit of this PR, which effectively merges ocamltest/Makefile in the root Makefile, does not need to touch `Makefile.common` in any way, which is totally in line with the spirit of the PR.

[dra27]

Ready to merge when precheck#862 concludes!

[shindere]

David Allsopp (2023/08/25 03:03 -0700):

@dra27 approved this pull request. Ready to merge when [precheck#862](https://ci.inria.fr/ocaml/job/precheck/862/) concludes!

Cool! Thanks!

[shindere]

David Allsopp (2023/08/25 05:22 -0700):

Merged #12321 into trunk.

Thanks!