Merge compilerlibs/Makefile.compilerlibs into the root Makefile

[shindere]

This PR continues the work started in #11243, #11248, #11268, #11420,
#11675, #12198 and #12321. The present PR is however slightly different from
the previous ones in the sense that the Makefile it merges was included in
the root Makefile rather than recursively called by it, meaning
that this PR does not change the dependency graph or the number of
recursive invocations of Make.

This PR however introduces a general framework to build libraries, similar
to the one already in place to build programs. It is expected that this
framework can be used also to build ocamldoc's generators, the other
libraries and the standard library itself.

In particular, this framework is flexible enough to deal with libraries
whose bytecode and native versions are built from different lists
of source files, as is the case for the ocamltoplevel library.
This is achieved by listing all the source files that appear in
at least one version of the library in its associated _SOURCES
variable. The framework then filters out the files matching /native/
when building the bytecode library, and vice versa.

Finally, the dependencies on MLI-only modules previously handled through the
GROUP_CMI variables are handled correctly: the libraries are made to
depend on all the .cmi files whose names can be derived from the .mli
files listed in the _SOURCES variable. This works whether the .cmi files
are for interface-only modules or not and it is fine to add regular .cmi
files to the list of dependencies because they were there already
transitively, so adding them does not change the transitive closure
of the dependency grpah.

[dra27]

The addition of the libraries part looks like a very logical continuation of the previous framework for the programs (nicely hinting that this is all on a good track). A few comments - the major contention may be that I think for the compilerlibs it is probably better to stick with listing explicit directories for every file (as Makefile.compilerlibs does at the moment).

[shindere]

Thanks a lot for the helpful and constructive review, as usual! David Allsopp (2023/10/13 10:33 -0700):

@dra27 commented on this pull request. The addition of the libraries part looks like a very logical continuation of the previous framework for the programs (nicely hinting that this is all on a good track).

Thanks. The two frameworks are actually so similar that I did consider introducing one more level of meta-programming to factorise even that but I renounced, at least for the time being, because I was under the impression that it would make things toodifficult to follow, not without any benefit at all though.

A few comments - the major contention may be that I think for the compilerlibs it is probably better to stick with listing explicit directories for every file (as Makefile.compilerlibs does at the moment).

No problem. I was actually happy about this refactoring but defenitely not enough attached to it to insist. For this time, I have implemented each suggestion in a separate commit, with the hope that it will make the reviewing process eaiser and the intention to squash each commit in one of the previous ones, once the review process has been completed.

In Makefile.common: This is identical to _OCAML_PROGRAM_BASE except that it defines an additional _CMI_FILES. That wouldn't hurt for the programs case as well, so could we instead have just the one _OCAML_COMMON_BASE?

Sure, done so. I kept the `_OCAML_PROGRAM_BASE` and `_OCAML_LIBRARY_BASE` macros, though, although they are identical and only call `_OCAML_COMMON_BASE`. I have the feelings that you can never be too pedestrian when doing these things.

In compilerlibs/Makefile.compilerlibs: > + bytelink.mli bytelink.ml \ + bytelibrarian.mli bytelibrarian.ml \ + bytepackager.mli bytepackager.ml) \ +$(addprefix driver/, \ + errors.mli errors.ml \ + compile.mli compile.ml \ + maindriver.mli maindriver.ml) + +intel_SOURCES = \ + x86_ast.mli \ + x86_proc.mli x86_proc.ml \ + x86_dsl.mli x86_dsl.ml \ + x86_gas.mli x86_gas.ml \ + x86_masm.mli x86_masm.ml + +arch_specific_SOURCES = ifeq ($(ARCH),i386) i386 is no more, so this block could just be deleted as part of this

Good idea, thanks. This has been updated in `configure.ac` by removing the `i386` case from the computation of the `arch_specific_SOURCES` variable.

In Makefile: > + +COMPILERLIBS = $(addprefix compilerlibs/, \ + ocamlbytecomp \ + ocamlcommon \ + ocamlmiddleend \ + ocamloptcomp \ + ocamltoplevel) + +# Since the compiler libraries are necessarily compiled with boot/ocamlc, +# make sure they *always are*, even when rebuilding a program compiled +# with ./ocamlc (e.g. ocamltex) + +$(COMPILERLIBS:=.cma): \ + CAMLC = $(BOOT_OCAMLC) $(BOOT_STDLIBFLAGS) -use-prims runtime/primitives + +# FIXME: how about making another target depend on $(ALL_CONFIG_CMO)? The rationale here (sort of) is that config.cmo is part of ocamlcommon?

I am not sure I understand the question, sorry. I think this one will be easier to discuss live.

In Makefile: > + +$(COMPILERLIBS:=.cma): \ + CAMLC = $(BOOT_OCAMLC) $(BOOT_STDLIBFLAGS) -use-prims runtime/primitives + +# FIXME: how about making another target depend on $(ALL_CONFIG_CMO)? +compilerlibs/ocamlcommon.cma: $(ALL_CONFIG_CMO) + +OCAML_LIBRARIES = $(COMPILERLIBS) + +$(foreach LIBRARY, $(OCAML_LIBRARIES),\ + $(eval $(call OCAML_LIBRARY,$(LIBRARY)))) + +# Let the native libraries depend on the native compiler + +$(ocamlcommon_CMX_FILES) $(ocamlbytecomp_CMX_FILES) \ +$(ocamloptcomp_CMX_FILES) $(ocamltoplevel_CMX_FILES): ocamlopt$(EXE) I think it's probably an error that the middle end library wasn't in this list (it will be in practice - I'm sure each module in the middle-end in someone depends on ocamlcommon so were rebuilt regardeless), but it would simplify the rule I think if it were simply that all the .cmx files of all the compilerlibs depend on ocamlopt?

I have replaced the rule with: ``` %.cmx: ocamlopt$(EXE) ``` I am wondering, though, whether it's okay to depend on `ocamlopt$(EXE)` actually. What about `%.cmx: ocamlopt.opt$(EXE)` for instance? (That again may be easier to discuss live.)

In compilerlibs/Makefile.compilerlibs: For this list which has files in multiple places, I think it's better to have each entry with an explicit directory

Done so, only when the files are not all in the same directory, as suggested, for all `_SOURCES` definitions.

In [7]configure.ac: > @@ -1337,6 +1338,11 @@ AS_CASE([$host], [has_native_backend=yes; arch=riscv; model=riscv64; system=linux] ) +AS_CASE([$arch], + [i386|amd64], This only happens for amd64 now.

Yes, taken into account, I think.

[dra27]

A few further comments - the rest we can pick up later

[shindere]

Thanks alot for the quick feedback! David Allsopp (2023/10/18 04:29 -0700):

> @@ -280,6 +270,26 @@ beforedepend:: $$$$($(basename $(notdir $(1)))_SECONDARY_FILES) $(basename $(notdir $(1)))_GENERATED_FILES = \ $$($(basename $(notdir $(1)))_SECONDARY_FILES) \ $$(patsubst %.mly,%.output, $$($(basename $(notdir $(1)))_MLY_FILES)) +endef # _OCAML_LIBRARY_BASE The comment should now be `_OCAML_COMMON_BASE`

Indeed, thanks. Fixed.

I can never remember the rules - does this pattern rule definitely work for adding dependencies if the files also have specific entries in `.depend`? The previous version expressly listed the files.

I think you are right that this does not work -- I tried. I thus built the dependencies on the native compiler into the framework for the time being. Which made me wonder whether we shouldn't go further and introduce, say, `_OCAML_COMMON_NATIVE` to capture all the things that are sharedb between native programs and native libraries.

> -$(addprefix driver/, \ - opterrors.mli opterrors.ml \ - optcompile.mli optcompile.ml \ - optmaindriver.mli optmaindriver.ml) + asmcomp/schedgen.mli asmcomp/schedgen.ml \ + asmcomp/scheduling.mli asmcomp/scheduling.ml \ + asmcomp/branch_relaxation.mli asmcomp/branch_relaxation.ml \ + asmcomp/emitaux.mli asmcomp/emitaux.ml \ + asmcomp/emit.mli asmcomp/emit.ml \ + asmcomp/asmgen.mli asmcomp/asmgen.ml \ + asmcomp/asmlink.mli asmcomp/asmlink.ml \ + asmcomp/asmlibrarian.mli asmcomp/asmlibrarian.ml \ + asmcomp/asmpackager.mli asmcomp/asmpackager.ml \ + driver/opterrors.mli driver/opterrors.ml \ + driver/optcompile.mli driver/optcompile.ml \ + driver/optmaindriver.mli driver/optmaindriver.ml) There's a stray parenthesis at the end of the line which I think it's what's breaking the build temporarily

It's fixed indeed, thanks!

> @@ -1477,16 +1467,21 @@ runtime/%.i.o: runtime/%.S runtime/%_libasmrunpic.o: runtime/%.S $(V_ASM)$(ASPP) $(OC_ASPPFLAGS) $(SHAREDLIB_CFLAGS) -o $@ $< -runtime/domain_state.inc: runtime/caml/domain_state.tbl - $(V_GEN)$(CPP) $< > $@ +runtime/domain_state64.inc: \ This change and the parts which follow appear to be a rebasing error of some kind?

Oops! I confess I didn't proof-read the commits, I should have.

[shindere]

I believe that the current branch addresses the two issues remaining from earlier today:

• Replace the FIXME comment by documentation about why this dependency is there
• Add a commit to ensure that all .cmx file depend on the native compiler so that they get rebuilt if the compiler is updated

As announced, once the branch is good to go I will rewrite the history slightly and basically squash all the commits that were created during the review process into the earlier ones. I intend to keep the current head commit of the branch, though, because (1) it does have a semantics on its own and (2) it touches both Makefile.common and the root Makefile, which is something none of the previous commits in this branch does.

[dra27]

If you rebase onto the same base trunk commit, these can all be addressed while squashing the history

[shindere]

Changes applied and history cleaned up without touching the merge-base. The head commit that introduced the dependency of all CMX files on the native compiler got finally split in two parts, each of them incorporated where it was due.

[dra27]

LGTM!

[shindere]

David Allsopp (2023/10/19 02:54 -0700):

@dra27 approved this pull request. LGTM!

Thanks again for your so supportive and helpful reviews, as usual!

[shindere]

I just direct-pushed db68f24 to fix a bogous line introduced by commit e1c2928 which belongs to this PR.

The problem has been discovered independently by @xavierleroy and myself.

I initially fixed it in PR #12706 but then followed Xavier's suggestion
to push it independently and directly.

Will now rebase #12706 on top of repaired trunk.

[xavierleroy]

I just direct-pushed db68f24 to fix a bogous line introduced by commit e1c2928 which belongs to this PR.

Thanks a lot ! I rebased my PR #12758, which was affected too.