Build system: make it possible to choose which ocamldep (and flags) to use

[shindere]

This PR standardizes the invocations of ocamldep.

It is now possilbe to choose which ocmaldep is used to compute dependencies
like this:

make alldepend OCAMLDEP=/usr/bin/ocamldep

Independently of which ocamldep is called, it is now also possible to
specify additional flags which will be passed to the invoked ocamldep in
adition to those passed by the build system (currently only -slash). It is
guaranteed that the additional flags will really be passed in addition to
the build system defined ones (rather than replacing them). It is also
guaranteed that the behaviour will be consistent accross all the
invocations of ocamldep.

The more general context of this PR is to make it possible and easier to use
the user-chosen tools during the build of the compiler, rather than having to
rely on in-tree tools because this behaviour is hard-coded in the build
system itself.

It is already possible to choose the runtime one wants to use and
one possible next step is to be able to use an installed version of ocamlc
as a bootstrap compiler, rather than having to rely on boot/ocamlc.
Similarly, one may want to be able to easily add user-chosen
compiler flags to those defined by the build system when compiling
the compiler, for instance to build an instrumented version of
the compiler itself.

Also, this PR fixes the Makefile of the memory model test which was
incorrectly invoking ocamldep directly, assuming there is already
an installed one available, which is not necessarily the case -- @maranget
may be interested in having a look.

[dra27]

One important change, and a couple of suggestions, but this looks nice to me! I've also verified locally that make alldepend still works (with the correction to stdlib/Makefile).

Given that the flags are added, perhaps this is a good point to add -no-slash to ocamldep (so that one can cancel the option using OCAMLDEPFLAGS):

diff --git a/driver/compenv.ml b/driver/compenv.ml
index 5e247c9941..ce74352e56 100644
--- a/driver/compenv.ml
+++ b/driver/compenv.ml
@@ -260,6 +260,7 @@ let read_one_param ppf position name v =
   | "verbose" -> set "verbose" [ verbose ] v
   | "nopervasives" -> set "nopervasives" [ nopervasives ] v
   | "slash" -> set "slash" [ force_slash ] v (* for ocamldep *)
+  | "no-slash" -> clear "no-slash" [ force_slash ] v (* for ocamldep *)
   | "keep-docs" -> set "keep-docs" [ Clflags.keep_docs ] v
   | "keep-locs" -> set "keep-locs" [ Clflags.keep_locs ] v

diff --git a/driver/makedepend.ml b/driver/makedepend.ml
index c05abe22ef..ff332f4b72 100644
--- a/driver/makedepend.ml
+++ b/driver/makedepend.ml
@@ -637,6 +637,8 @@ let run_main argv =
         " Generate dependencies for native plugin files (.cmxs targets)";
       "-slash", Arg.Set Clflags.force_slash,
         " (Windows) Use forward slash / instead of backslash \\ in file paths";
+      "-no-slash", Arg.Clear Clflags.force_slash,
+        " (Windows) Preserve any backslash \\ in file paths";
       "-sort", Arg.Set sort_files,
         " Sort files according to their dependencies";
       "-version", Arg.Unit print_version,

[dra27]

Suggested change

	# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS
	# variables defined in Makefile, so it can only be invoked from the
	# main Makefile
	# This rule depends on the OCAMLDEP_CMD variable defined in Makefile.common,
	# so it can only be invoked from the main Makefile

[dra27]

Can this one be refactored with $(addprefix to be consistent with the INCLUDES_DEP? So

INCLUDES_NODEP = $(addprefix -I $(ROOTDIR)/,\
  stdlib compilerlibs otherlibs/str otherlibs/dynlink otherlibs/dynlink/native otherlibs/$(UNIXLIB))

[dra27]

This should be BOOT_OCAMLDEP (there's a stray capital C before DEP)

[dra27]

Nice!

[shindere]

Many thanks for the detailed feedback, @dra27. Once again it has been sad to notice how the text version of the e-mails sent by GitHub is broken, compared to the HTML one. Would really be nice to get this acted upon. David Allsopp (2022/03/22 09:21 -0700):

@dra27 commented on this pull request. One important change, and a couple of suggestions, but this looks nice to me! I've also verified locally that make alldepend still works (with the correction to stdlib/Makefile).

I did check but missed that. Thanks.

Given that the flags are added, perhaps this is a good point to add -no-slash to ocamldep (so that one can cancel the option using OCAMLDEPFLAGS):

[...] Yes! Good idea, thanks. I added a separate commit for that and credited you for the change in the commit message.

In [1]Makefile.menhir: > +# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS +# variables defined in Makefile, so it can only be invoked from the +# main Makefile � Suggested change -# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS -# variables defined in Makefile, so it can only be invoked from the -# main Makefile +# This rule depends on the OCAMLDEP_CMD variable defined in Makefile.common, +# so it can only be invoked from the main Makefile

Fixed. I also clarified the comment a bit and moved it above the rule it talks about.

══════════════════════════════════════════════════════════════════════════ In [2]ocamldoc/Makefile: > +INCLUDE_DIRS = $(addprefix $(ROOTDIR)/,\ + utils parsing typing driver bytecomp toplevel) +INCLUDES_DEP = $(addprefix -I ,$(INCLUDE_DIRS)) INCLUDES_NODEP=\ Can this one be refactored with $(addprefix to be consistent with the INCLUDES_DEP? So INCLUDES_NODEP = $(addprefix -I $(ROOTDIR)/,\ stdlib compilerlibs otherlibs/str otherlibs/dynlink otherlibs/dynlink/native otherlibs/$(UNIXLIB))

Done. Thanks.

══════════════════════════════════════════════════════════════════════════ In [3]stdlib/Makefile: > @@ -14,6 +14,10 @@ #************************************************************************** ROOTDIR = .. +# NOTE: it is important that the OCAMLDEP variable is defined *before* +# Makefile.common gets included, so that its local definition here +# take precedence over its general shared definitions in Makefile.common. +OCAMLDEP ?= $(BOOT_OCAMLCDEP) This should be BOOT_OCAMLDEP (there's a stray capital C before DEP)

Absolutely. Thanks. Fixed.

[sabine]

Suggested change

	(and flags) to use when computiing depencies for the compiler.
	(and flags) to use when computing depencies for the compiler.

[dra27]

This looks good to go to me, thanks - @Octachron, what is the glaciated state of trunk (vis-à-vis the addition of -no-slash to ocamldep?)

[shindere]

David Allsopp (2022/03/28 02:59 -0700):

@dra27 approved this pull request. This looks good to go to me, thanks - @Octachron, what is the glaciated state of trunk (vis-à-vis the addition of `-no-slash` to ocamldep?)

Many thanks for the approval!

[Octachron]

Concerning the freeze, I would propose to circumvent the issue: ocamldep is not the compiler, and it should be fine to update tools in the compiler distribution with backward compatible features even during the freeze.

[dbuenzli]

Aside, my understanding of the sequential glaciation was that it was over when multicore got merged. Hence I recently pinged a few stdlib PRs, sorry about that.

[shindere]

That was my understanding too. My belief was/is is that trunk is not frozen now, although we perhaps try to keep the set of new features minimal as far as the language and runtime are ocncerned.

[Octachron]

My understanding is that trunk will be frozen until the moment that the 5.0 branch is cut. I am aiming to cut the branch as soon as the remaining issues in #11013 are resolved.

[shindere]

Florian Angeletti (2022/03/29 11:43 +0000):

My understanding is that trunk will be frozen until the moment that the 5.0 branch is cut. I am aiming to cut the branch as soon as the remaining issues in #11013 are resolved.

Ah, okay. I got a few PRs merged but perhaps because they fixed something. So can we merge this one or should it wait?

[Octachron]

Let's merge, like I said this is mostly a new ocamldep feature (and arguably a fix) and not a change to the compiler itself.

[shindere]

Florian Angeletti (2022/03/29 06:52 -0700):

Let's merge, like I said this is mostly a new ocamldep feature (and arguably a fix) and not a change to the compiler itself.

Thanks. Done so, as can be seen.

[shindere]

sabine (2022/03/23 01:33 -0700):

@sabine commented on this pull request. > @@ -189,6 +189,10 @@ Working version - #11092: Build native-code compilers on OpenBSD/aarch64. (Christopher Zimmermann, review by Anil Madhavapeddy) +- #11126: Build system: make it possible to choose which ocamldep + (and flags) to use when computiing depencies for the compiler. ```suggestion (and flags) to use when computing depencies for the compiler. ```

Many thanks for your attention, @sabine. Fixed and added a mention of the -no-slash option to the Changes entry.