Cease storing C dependencies in the repository

[dra27]

This is a proof-of-concept (with a few minor decisions to be taken noted in the code) of a discussion on caml-devel. This PR eliminates the need for storing C dependencies in the repository.

** OCaml dependencies are entirely different kettle of 🐟 and should be left out of this discussion, please **

When building for the first time, the only requirement is that generated header files have been built (jumptbl.h, version.h and opnames.h). Detailed dependency information is only required when headers have been edited.

COMPUTE_DEPS in Makefile.build_config controls whether C dependency information should be generated on a per-file basis. This variable is controlled by a new --disable-dependency-generation in configure which is enabled for Git checkouts and disabled for tarballs (i.e. releases).

The Microsoft C compiler (cl) cannot generate dependencies in a consistent way which we can consume, so for a Git checkout configure searches for an additional C compiler in order to compute dependencies. This is obviously not required for a user-build.

As a result, the MSVC port can now safely run make alldepend, since only OCaml dependency information is committed to the repo after this change.

CI does not need to waste time testing the dependency information, because it only tests a single build. A single Travis job has been added which tests the build system code to generate the dependency information (and provides a single make -j run in CI, although Inria's CI also
tests parallel building continuously).

This branch is running through precheck

cc @shindere

[dra27]

I think these are legacy from before the merge of the runtime directories, @shindere?

[shindere]

First of all and that's certainly the most important: thanks a lot for the work. I'm giving the feedback as I go through the diff, sorry I can't comment the code approriately. It feels to me the cleanup of the .gitignroe could be kept for anotehr PR, but that's even more true for the modification of BOOTSTRAP.adoc which does not seem related to this PR, as I understand it. Changes has spurious hunks, it feels. At a higher level: to some extent I understand why you introduced a configure-time option to control whether the dependencies should be computed or not. In a way, there is no reason to couple this with release vs. development mode. However, from a more practical point of view, it feels to me we already have plenty of configure option so I am owndering whether it wouldn't be better, at least for a start, to bind this with release vs. developemnt mode. perhaps we can postpone the decoupling until we are sure there is a real need for that? Unless, of course, you already have one in mind? In the Makefiles, in the include directives that include the dependency files, shouldn't you use a -, i.e. -include rather than include? Re: the creation of the .dep directory: I think there oculd be rules that create them explicitly, see order-only prerequisites: ``` .dep/%.depend: %.c | .dep $(DEP_CC) $(OC_CPPFLAGS) $< | sed -e 's/\.o/.$$(O)/' > $@ .dep: @$(MKDIR) $@ ``` (Not sure whether it is good to silence mkidr, by the way.) Oh and by the way, perhaps `.d` would be enought as a suffix for these files which are in a dedicated directory anyway? And perhaps Makefile.config.in could define a DEPSUFFIX variable to make sure this is coherent accross the build system? In the runtime directory: wouldn't it be simpler to generate dependency files that have the same name than the object file they compute dependencies for? When reading the code it does not look to me that's what is done but maybe I am wrong here?

[dra27]

Thanks, @shindere. I'm confused by your early comments: I haven't touched Changes yet and the only bootstrap file I've touched is tools/ci/inria/bootstrap, or is the merge commit you're looking at mentioning some other stuff?

The clean-up in .gitignore relates to removing what looked like old either dependency information or possibly debugging objects - either way, I was intentionally touching other lines nearby and ensuring that the entries were correct for runtime/.

There is an immediate use for --disable-dependency-generation in the PR itself - all the CI alterations are to use it, as there's no point wasting 10 seconds or so per job generating the C dependencies. One new job in Travis simultaneously tests parallel build and also forces the dependency generation.

-include would fail without error if the files cannot be included, which is not what I want - if dependencies are supposed to be being computed, then you want that to be an error. This is the include (or the $(foreach which generates it) is guarded by a test on $(COMPUTE_DEPS).

The order-only dependency is a good catch (and should have been on my TODO list) - in a first version I called the directory .depend but changed it to ensure that it would never match up with a filename we'd used before).

No opinion on the extension; I'm happy to change it.

In runtime/, foo.c causes .dep/foo-byt.depend and .dep/foo-nat.depend to be generated; the reason for separating them is that the building of these files takes a non-trivial amount of time and if you've configured without the native code compiler then it speeds things up. Changing that wouldn't be particularly simpler - the recipes would simply be concatenated (they're just a refactoring of what was already there).

[shindere]

Sorry for the long silence on this. I just realised after talking to
@dra27 over the phone that I was waiting for feedback about a comment that
had actually never been sent :(

In this comment, I was suggesting to rewrite the rules generating the
dependencies to take advantage of a few gcc options that may avoid
the use of sed scripts.

Among these options are:

• '-MF FILE' which lets you specify in which file to write the
  dependencies. As explained in gcc's documentation, this lets you separate
  the dependencies from other preprocessor output.Although these are not
  supposed to happen, they actually could and it wouln't hurt to use
  this option, I think.

• '-MG', which assumes missing header files will be generated and adds them
  to the list of prerequisites may be of interest, but I am not completely
  sure.

• '-MT target' or '-MQ target' let you specify the name of the target,
  the difference being that '-MQ' quotes special charactgers. Of all the
  optiosn, this is the one that looks most interesting to me because
  it would avoid the use of sed scripts. One could thus get rid of the
  for loops and use pattern rules to generate the dependency files.

[dra27]

No problem, @shindere! Here's some work on this - -MT makes it much neater, indeed - sed is gone. I haven't used -MF because runtime/Makefile embeds multiple rules in the same file (so > followed by >> is sort of neater). Is it definitely worth it?

There's still a question of when to remove .dep in ocamltest/Makefile L287, otherlibs/Makefile.otherlibs.common L132, otherlibs/systhreads/Makefile L114 and runtime/Makefile L211 - do you think these should be deleted on make clean (which is what it currently does) or make distclean?

[shindere]

Many thanks for your prompt response and hard work, @dra27. David Allsopp (2020/04/15 10:21 -0700):

No problem, @shindere! Here's some work on this - `-MT` makes it much neater, indeed - `sed` is gone.

That's great, thanks!

I haven't used `-MF` because `runtime/Makefile` embeds multiple rules in the same file (so `>` followed by `>>` is sort of neater). Is it definitely worth it?

Did you consider having one `.d` file not for each source file but for each object file that needs to be generated? In such a model you could generate the `.d` files with pattern rules and then yes I think the rules would be yet simpler and so yes, to me it seems worth it. What do you think?

There's still a question of when to remove `.dep` in [ocamltest/Makefile L287](https://github.com/ocaml/ocaml/pull/9332/files#diff-b1dca41007b17834a136f6b593489355R287), [otherlibs/Makefile.otherlibs.common L132](https://github.com/ocaml/ocaml/pull/9332/files#diff-a02b2ab4b2b4d8c0db466a3fb40b02e6R132), [otherlibs/systhreads/Makefile L114](https://github.com/ocaml/ocaml/pull/9332/files#diff-3d1592d5afba46794caa00760b9b4e29R114) and [runtime/Makefile L211](https://github.com/ocaml/ocaml/pull/9332/files#diff-fa4cef881ee67fe396ca277ebc75ded1R211) - do you think these should be deleted on `make clean` (which is what it currently does) or `make distclean`?

I have no strong opinion. The two things I can think of: (1) Since it's a configure-time decision whether to compute C dependencies or not, perhaps `distclean`would be a better choice, because that's in general where one removes files that are generated by `configure` (although the dependency files themselves are not generated by `configure`). (2) To avoid making things too complex and ocunter-intuitive for all of us developers, perhaps we would like to handle C and OCaml dependencies in ways as similar as possible. So, if we think that the OCaml dependency files should be removed during `make clean` (assuming they are no longer stored in the repo), then perhaps we should do likewise for C dependencies. What do you think about all this, @dra27 and others?

[dra27]

Yes, I had considered splitting up the .d files in runtime/, but it still wasn't clear to me that it would be much neater... but then I found in the manual that you can include wildcards. This allows one to generate .dep/Makefile (which is a single line and is acting much more like a make cookie than an actual Makefile, which is why it's written with @echo cf. @touch normally) and have that depend on the .d files which are required. That gets rid of all the foreach stuff and does indeed make the rules look less cryptic. runtime/Makefile is still complicated (and, to a lesser extent, otherlibs/systhreads/Makefile) because of building multiple object files from the same C source file, but generating the dependency information has not increased any of the complexity.

GNU make's pattern substitutions don't let you have two wildcards, which is occasionally limiting (so you can't, for example, trim off _% from strings). For this reason, I've altered the C object names to be .b.o instead of _b.o which I don't think makes any material difference anywhere, but does allow $(basename to operate on the filenames. It's also handy because we do have C files where the basename includes _ but of course no C files which include ..

I think arguably the C dependencies should probably be in distclean but, given that you could be working on ocamldep it's clear that you'd expect OCaml dependencies to be blown away in clean, so I think I'll leave the C dependencies being erased in clean. It's largely moot anyway, as C dependencies regenerate very quickly.

Assuming this passes CI and precheck, this is ready for final review.

[shindere]

Thanks a lot, @dra27. I'll do the review ASAP. Are you happy with the history? If not, could you perhaps rewrite it before the review?

[dra27]

I have already rewritten the history, yes - the bulk of the work is in the first commit; the two testsuite changes are separated in order to make the blame explicit (the change is related to testing alterations as part of the first commit, rather than being specifically because of dynamic dependency generation)

[dra27]

Just a note that the precheck run passed, as has Travis (AppVeyor is just finishing up, but it looks to be green too)

[shindere]

David Allsopp (2020/04/16 03:30 -0700):

Yes, I had considered splitting up the `.d` files in `runtime/`, but it still wasn't clear to me that it would be much neater... but then I found in the manual that you can `include` wildcards. This allows one to generate `.dep/Makefile` (which is a single line and is acting much more like a make cookie than an actual Makefile, which is why it's written with ***@***.***` cf. ***@***.***` normally) and have that depend on the `.d` files which are required.

What are you trying to achieve with this? If the .d files depend on the source files they will be re-built if those change, right? So why do you do this?

That gets rid of all the `foreach` stuff and does indeed make the rules look less cryptic.

That's great, thanks.

`runtime/Makefile` is still complicated (and, to a lesser extent, `otherlibs/systhreads/Makefile`) because of building multiple object files from the same C source file, but generating the dependency information has not _increased_ any of the complexity.

I fully agree. To me, the complexity of `runtime/Makefile` is justified given what is done there. Which I wouldn't say about, say, `tools/Makefile` for instance.

GNU make's pattern substitutions don't let you have two wildcards, which is occasionally limiting (so you can't, for example, trim off `_%` from strings). For this reason, I've altered the C object names to be `.b.o` instead of `_b.o` which I don't think makes any material difference anywhere, but does allow `$(basename` to operate on the filenames. It's also handy because we do have C files where the basename includes `_` but of course no C files which include `.`.

That seems reasonable to me, thanks.

I think arguably the C dependencies should probably be in `distclean` but, given that you could be working on `ocamldep` it's clear that you'd expect OCaml dependencies to be blown away in `clean`, so I think I'll leave the C dependencies being erased in `clean`. It's largely moot anyway, as C dependencies regenerate very quickly.

Fine with me.

Assuming this passes CI and [precheck](https://ci.inria.fr/ocaml/job/precheck/352/), this is ready for final review.

Here are a few comments, then: In the commit message: "When building for the first time, the only requirement is that generated header files have been built (jumptbl.h, version.h and opnames.h).": doesn't the use of `-MG` make this sentence obsolete? In `configure.ac`: Since the error message about msvc not supporting the computation of dependencies is on several lines in the source, I think you need to use `m4_normalize` as done in other places so that the message gets formatted properly when output by `configure`. If you don't do that, I think the message will be indented as in the source code, which is not what we want, I think. I think in a previous comment I suggested that you introduce a `DEPDIR` build variable, similarly to what is done with `DEPSUFFIX`, just to enforce consistency in derectory names. Would you mind doing that? Also, regarding `DEPSUFFIX`: assume you'd define it to just `d` rather than `.d`, then in the makefiles you could write `.dep/%.$(DEPSUFFIX)` rather than `.dep/%$(DEPSUFFIX)`, right? And am I correct that this would in turn avoid you having to change the name of generated object files? And again, e.g. in `ocamltest/Makefile` I do not understand why you need to include another file that includes the `.d` files. If the `.d` files are more recent than the files they depend on, make will re-generate them anyway so it really seems to me they can be included directly, can't they? Also, how about factorizing the rule that createes the `.dep` directory by putting it into `Makefile.common`? It seems that the fragment ``` $(wildcard $(ROOTDIR)/runtime/caml/*.h) \ $(wildcard $(ROOTDIR)/runtime/caml/*.tbl) \ $(wildcard *.h) ``` appears several times. Perhaps it could be defined in a `ocaml_headers` variable in `Makefile.common` ? Regarding `runtime/Makefile`: Nitpick: the definitions of `GENERATED_HEADERS` and `DYNAMIC_HEADERS` could use the `addprefix` make function. I am also not very enthousiastic about the names of these variables, which makes the distinction between them unclear to me. How about renaming `DYNAMIC_HEADERS` to, say, `CONFIG_HEADERS` ? Also about your renaming of the generated file where you replace `_` by `.`: not that I don't like the idea in principle, but I thin it is not absolutely necessary and leaving that renaming out would make the patch a bit simpler. It would also simplify the rebasing of #9082. Is the change that introduces the error message "The build has been configured with --disable-native-compiler" actually somehow in relation with the purpose of the PR? Same question for the addition of the `rm -f domain_state*.inc` command. Last suggestion for this file: wouldn't it be better to define one `_depfiles` variable for each target, in a way similary to how the `_OBJECTS` variables are defined, and then one oculd include exactly the relevant dependency files depending on what needs to be built. What do you think, @dra27?

[dra27]

Thanks, @shindere! I've rebased (to fix the conflicts) but put the changes in the 9 most recent commits. Answers to your points:

• The .dep/Makefiles were the result of a mistaken earlier iteration where I thought I could use wildcards, but not variables, in an include statement. They're now gone - the .d files get included directly
• The change from _b.o to .b.o is not in any way related to the way $(DEPSUFFIX) was defined, and I don't think it's possible to keep the _ version without duplicating the rules in runtime/Makefile and otherlibs/systhreads/Makefile. The problem is that you have a series of files which depend on the same C file - e.g. extern_b.o, extern_bd.o, extern_n.o etc. Each one of those depends on its own .d file. All of them depend on extern.c. I can easily generate an empty @ rule which allows make to believe that extern_b.o depends on extern_b.c which depends on extern.c (that's done in COMPILE_C_FILE), but the problem is how I convert extern_b.d in the pattern rule to extern.c - if it's one suffix, then I can use $(subst or whatever with %_b.d, but as far as I'm aware there is no way in make to remove _%.d from a string as that pattern says to make that the strings begin with _ and end with .d and you definitely can't write %_%.d. However, $(basename can simulate what we need, because it removes a generic extension from a filename (if it has one) and returns the filename.
• The commit message you refer to is correct - I'm not talking about generating the dependencies, I'm talking about compilation, which does require those files to exist (I'm explaining why dependency information is not required to build the object files the first time)
• Thanks for the catch in configure.ac - I hadn't spotted that message was wrong!
• I don't think you had suggested $(DEPDIR), but I've done it!
• As I said, $(DEPSUFFIX) has nothing to do with the change in object names, but I have removed the . from it - in fact by changing it to .$(D) it looks more consistent where it appears in rules next to .$(O)
• Those headers variables went through a few iterations - given that the variable with all five headers was only used in one place, I've gone with $(GENERATED_HEADERS) for the headers generated by runtime/Makefile (I avoid the word "compiled" because a compiled header in C/C++ is a Microsoft "thing") and $(CONFIG_HEADERS) for the ones generated by configure. Neither variable includes the other.
• The domain_state*.inc is definitely part of this PR - it gets altered because version.h which was on that line is deleted via $(GENERATED_HEADERS).
• I often develop with --disable-native-compiler because you can quickly test most of the build system with make -j world - I probably then ran make allopt in runtime and found the message very confusing. I can drop it from here, but I'm not assembling an entire just for that! It's part of this PR inasmuch as runtime/Makefile has a lot of native/byte specific dependency stuff to test...
• I initially thought that using the _OBJECTS would be neater, but it doesn't work - the native code ones include the assembly objects, which obviously don't have dependencies or at least C files. I think what you were hoping to achieve shouldn't be done - all dependency files should be generated all the time - configuring with --disable-debug-runtime stops make all from building it, but it shouldn't prevent a core developer from being able to say make -C runtime libcamlrund.a, so you can't predict which dependency information is needed unless you analyse the targets requested. I think that can be exceptionally to make things like make distclean work without configuring, but for actual build targets I think it runs a risk of being brittle and annoying if it ends up wrong? Incidentally, that's why --disable-native-compiler does and should turn off the dependency of the native dependency files - none of those things can actually be compiled if you configured with that turned off, unlike the various runtimes.

[shindere]

Thanks a lot, @dra27, for your in-depth response. I will review again on monday.

[dra27]

No problem - have a good weekend, @shindere!

[shindere]

Thanks! you too!

[shindere]

David Allsopp (2020/04/17 07:03 -0700):

Thanks, @shindere! I've rebased (to fix the conflicts) but put the changes in the 9 most recent commits. Answers to your points:

Thanks, @dra27. Will you cleanup the history once we are done? In particular, will you remove #9453 from your branch? You may push these changes to the original PR branch or perhaps @gasche may pick them up if he likes.

- The `.dep/Makefile`s were the result of a mistaken earlier iteration where I thought I could use wildcards, but not variables, in an `include` statement. They're now gone - the `.d` files get `include`d directly

That's good, thanks!

- The change from `_b.o` to `.b.o` is not in any way related to the way `$(DEPSUFFIX)` was defined, and I don't think it's possible to keep the `_` version without duplicating the rules in `runtime/Makefile` and `otherlibs/systhreads/Makefile`. The problem is that you have a series of files which depend on the same C file - e.g. `extern_b.o`, `extern_bd.o`, `extern_n.o` etc. Each one of those depends on its own `.d` file. *All* of them depend on `extern.c`. I can easily generate an empty `@` rule which allows `make` to believe that `extern_b.o` depends on `extern_b.c` which depends on `extern.c` (that's done in `COMPILE_C_FILE`), but the problem is how I convert `extern_b.d` in the pattern rule to `extern.c` - if it's one suffix, then I can use `$(subst` or whatever with `%_b.d`, but as far as I'm aware there is no way in `make` to remove `_%.d` from a string as that pattern says to `make` that the strings begin with `_` and end with `.d` and you definitely can't write `%_%.d`. However, `$(basename` can simulate what we need, because it removes a generic extension from a filename (if it has one) and returns the filename.

Okay, it makes sense, thanks a lot.

- The commit message you refer to is correct - I'm not talking about generating the dependencies, I'm talking about *compilation*, which does require those files to exist (I'm explaining why dependency information is not required to build the object files the first time)

Okay

- Thanks for the catch in `configure.ac` - I hadn't spotted that message was wrong!

No problem. Is it on purpose that you didn't add a full-stop at the end of the second sentence of the message?

- I don't think you had suggested `$(DEPDIR)`, but I've done it!

Thanks!

- As I said, `$(DEPSUFFIX)` has nothing to do with the change in object names, but I have removed the `.` from it - in fact by changing it to `.$(D)` it looks more consistent where it appears in rules next to `.$(O)`

That's a good idea, thanks.

- Those headers variables went through a few iterations - given that the variable with all five headers was only used in one place, I've gone with `$(GENERATED_HEADERS)` for the headers _generated_ by `runtime/Makefile` (I avoid the word "compiled" because a compiled header in C/C++ is a Microsoft "thing") and `$(CONFIG_HEADERS)` for the ones generated by `configure`. Neither variable includes the other.

Regarding the definition of `RUNTIME_HEADERS`in `Makefile.common`, I may be wrong but it seems to me that the second line should have one more space in its indentation so that the `$ ` signs get aligned. Also, I'd prefer avoiding the use of the `wildcard` funciton when this is not really necessary and liest the filenames explicitly, but I realise this is a matter of taste and if you don't like this idea I'm fine. Similarly, I'd have used `addprefix` in the definition of `CONFIG_HEADERS` but I realise this may go too far so if you like the code as it is, that's fine with me, too.

- I initially thought that using the `_OBJECTS` would be neater, but it doesn't work - the native code ones include the assembly objects, which obviously don't have dependencies or at least C files. I think what you were hoping to achieve shouldn't be done - all dependency files should be generated all the time - configuring with `--disable-debug-runtime` stops `make all` from building it, but it shouldn't prevent a core developer from being able to say `make -C runtime libcamlrund.a`, so you can't predict which dependency information is needed unless you analyse the targets requested. I think that can be exceptionally to make things like `make distclean` work without configuring, but for actual build targets I think it runs a risk of being brittle and annoying if it ends up wrong?

Agreed. The conditional inclusion of dependencies was optional in my mind, anyway. that does not necessariliy mean that introducing variables to track the names of dependency files was a bad idea. ;-)

[dra27]

My only reason to wait on this one was because I was implementing a suggestion I had made in the other PR, rather than giving it a chance to be resolved!

Final rebase done, this should be ready to go!

[gasche]

I have an unrelated question, but: did you check with existing heavy-Windows-users (other than David of course), for example @nojb, that they were happy with the restriction, in the MSCV port, of having another C compiler available on the development machines?

[dra27]

Yes, but only inasmuch as Nicolás was part of the thread on caml-devel which started this PR.

Just to be totally clear (FTR as much as anything else) - this only adds a requirement to a developer of the compiler and one who works on the runtime itself - it doesn't affect you if you distribute OCaml, even if you wish to build OCaml from source on MSVC as part of that. It's also now the case that incremental compilation always works, even on MSVC, it just massively over-rebuilds C files if you don't have the mingw compiler (that wasn't the case in the first version of the code here).

[nojb]

Just to be totally clear (FTR as much as anything else) - this only adds a requirement to a developer of the compiler and one who works on the runtime itself - it doesn't affect you if you distribute OCaml, even if you wish to build OCaml from source on MSVC as part of that. It's also now the case that incremental compilation always works, even on MSVC, it just massively over-rebuilds C files if you don't have the mingw compiler (that wasn't the case in the first version of the code here).

Sorry, I hadn't followed the details, but indeed what is described here seems perfectly OK: there is not actually dependence on the mingw compiler, but without it, you get a "one C file changed -> rebuild all C objects" behavior.

[dra27]

Not quite: without the mingw compiler you get a "one H file changed -> rebuild all C objects". Changing a C file still only recompiles that one C file.

[nojb]

Not quite: without the mingw compiler you get a "one H file changed -> rebuild all C objects". Changing a C file still only recompiles that one C file.

Indeed, that's what I had in mind ("dune behaviour")

[dra27]

Right, 9082 is in, although in the end this didn't conflict much with it. Assuming CI passes, I shall merge.

[shindere]

David Allsopp (2020/04/30 05:54 -0700):

Right, 9082 is in, although in the end this didn't conflict much with it. Assuming CI passes, I shall merge.

Great! Good to have that in before going to vacation. Well staying at home for vacation. Will be back on May 11th.

[dra27]

The term “staycation” got coined for this in the financial crisis when people couldn’t afford to go away!

[shindere]

David Allsopp (2020/04/30 07:15 -0700):

The term “staycation” got coined for this in the financial crisis when people couldn’t afford to go away!

It indeed sounds appropriate and I am guessing that we will have many "opportunities" to make use of it in the coming months.

[gasche]

There is a bug in this PR caused by a missing -MG that breaks alldepend for otherlibs/win32unix. @dra27 and myself hit it while trying to rebase #8753 (the CI test that alldepend is a no-op). @dra27 proposed a fix which is to add -MG in otherlibs/Makefile.otherlibraries.common. He is working on a more general fix (I suggested a DEP_CC_FLAGS variable used consistently.)