I can't remember what is in place for canonicalising the dependency files (I know you've done some things with this before?). I wonder if it might want to be restricted to checking the OCaml dependencies only, but leave C dependencies alone?

Overall, it's a good idea, though!

Checking only OCaml dependencies: I think that would be nice, but I don't know how to do that without splitting the single per-directory .depend file in two (.depend-ml and .depend-c), and I feel a bit too lazy to do this before it proves an issue.

I'm just doing some checking on that - incidentally, I don't think your code as it stands can possibly work, because make alldepend requires a configured and built tree? I think this has to be done in the way as the check-all-arches - at the end of a build step.

I initially started with a check that is part of Build, but in fact at least the OCaml part works from the .ml/.mli files alone, and is careful to generate the parts that have to be generated first (see beforedepend).

I should definitely configure first, and I forgot that. I will experiment on my local machine to ensure that ./configure ; make alldepend can work, and update this PR.

Quick testing suggests that I need to coldstart for obscure primitive reasons.

The reason why I moved the check out of the main Build test is that I expect this check to fail on a semi-regular basis, and I want to make sure that it is very clear to users what is failing. I can print a lot of contextual information (and I do), but having a separate check at the Travis level will always be a much better user interface.

OK, testing df63138 on CentOS 7 and Ubuntu 18.04. They both find the following actual missing dependency:

diff --git a/otherlibs/dynlink/.depend b/otherlibs/dynlink/.depend
index bbad580..0a3555b 100644
--- a/otherlibs/dynlink/.depend
+++ b/otherlibs/dynlink/.depend
@@ -14,7 +14,8 @@ dynlink_common.cmi : \
 dynlink_platform_intf.cmo : \
     dynlink_types.cmi \
     dynlink_platform_intf.cmi
-dynlink_platform_intf.cmi :
+dynlink_platform_intf.cmi : \
+    dynlink_types.cmi
 dynlink_types.cmo : \
     dynlink_types.cmi
 dynlink_types.cmi :

but Ubuntu also generates this:

dra@bionic:~/ocaml$ git diff --stat
 otherlibs/dynlink/.depend |   3 ++-
 otherlibs/unix/.depend    |  24 ++++++++++++------------
 runtime/.depend           | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------
 3 files changed, 94 insertions(+), 93 deletions(-)

and the extra changes are all C dependencies which have been reordered.

So it seems that we do need to separately check for C and OCaml dependencies...

I decided to close the PR for now, because I don't have the time to make it work correctly (by separating differences in OCaml and C dependencies somehow).

This branch should work if #9332 is accepted

#9332 will be merged once CI comes back - this PR I think should then "just work" (modulo a rebase) and add considerable value

Thanks for reviving this one. Indeed, it would be nice to have this if we get rid of C dependencies.

I am less available to deal with this right now, so please feel free to adopt this PR if there are further changes to be made and you want to avoid delays on my end.

Ahem... @dra27 I just saw that you rebased the PR now that #9332 was merged... after rebasing the PR now that #9332 was merged. Apologies for the awkward superposition.

I am trying to test the rebased patchset at home and I encountered two issues:

1. some make depend rule relied on ocamlc being built through BEST_OCAMLDEP, while I tried in check-depend to require only minimal dependencies (in particular not ocamlc which is a bit slow to build, especially if we don't use parallelism)
2. I am now getting an include-time error for otherlibs/unix/cst2constr.h and I don't know where it is coming from.

I fixed (1) by pushing a change to BEST_OCAMLDEP to use the bootstrap compiler instead of ocamlc. Looking at whether the second issue is a real issue.

The cst2constr.h failure I observe in fact comes from win32unix, not unix. This header file is not copied from unix by the Makefile and yet it is included by getaddrinfo.c.

I tried to push a fix for the cst2constr.h failure I was observing. I don't know if this is a real issue and, in that case, why it was not caught earlier.

Sorry @gasche, I didn't realise you'd start working on it given your comment - I've been fixing this in my own fork temporarily.

I took 3db9aab and have set this build running - https://travis-ci.org/github/dra27/ocaml/builds/681862234

The simpler fix for the win32unix problem is twofold: firstly there's no reason for -MG to be missing in otherlibs/Makefile.otherlibs.common (that actually fixes the error) and also it's better to run this test, somewhat counterintuitively, with --disable-dependency-generation!

Can I force push dra27@d8c8b5f here?

https://travis-ci.org/github/dra27/ocaml/jobs/681862241 succeeded in 1:11, which I think is OK for this test

Hm.

• Yes, (you're too kind to point it out explicitly but) I'm at fault here for working on this on a whim after saying I wouldn't. Thank you for looking at this as I suggested you should, and apologies for the confusion. I don't want to work on this, but I'm doing it anyway. Aarh!

• It feels like a problem to me that the new C dependency generation stuff would explode right on the first build I did after it. (Admittedly a sort of exotic build, trying to run alldepend after only the minimal dependencies.) I'm not fond of your solution which is to just disable it, because I expect that other users will also run into the same issues in other situations. If it is easy to get a build failure by running make coldstart ocamlyacc; make alldepend, I think we should fix it rather than configure it away.

I haven't just disabled - I've fixed it and disabled it. There was one missing flag in one Makefile for a quite complicated piece of work - please shoot me!

This test does not need to generate C file dependencies, so I put --disable-dependency-generation to speed it up - like all the other tests on Travis bar the one which does it to make sure it's still working.

Thanks! I found the fix you mention in your branch, I looked at the gcc documentation and I understand what MG does now.

This is a bug in #9332; in particular, make core; make alldepend is currently broken in trunk right now. I'm a bit nervous at the fact that other places use $(DEP_CC) but not -MG; namely ocamltest. Could we have a DEP_CC_FLAGS variable that is used consistently?

I think we should move this conversation to #9322, fix it now, and go back to the present PR later.

Yes, I'm already working on it

Just to confirm: I am now convinced that I should stop working on it again, and will try to stick to it. Please feel free to force-push.

(Thanks for the explanation on the configuration change.)

Hopefully this will now have CI greenness by the morning! (it's passed Travis on my fork, again)

Huzzah, modulo the Changes test, we have green. I committed a dummy version with .depend sabotaged with corresponding Travis log here.

@gasche - I've put the no-change-entry-needed label on, but no problem if you want to put an entry in, obviously. I'm happy with the BEST_OCAMLDEP bit - can I suggest we merge it, as it's useful on trunk, and let Sebastien retrospectively review it when he's "back"?

Indeed, I merged.

Hopefully that’s an end of - or at least big reduction in - random unexplained parallel build failures!

I'm getting increasingly skeptical about this; I observe many parallel build failures when doing incremental builds (typically interactions between stdlib, .cma and plain compilation units), and I think that most of them are due to incorrect Makefile dependencies rather than incorrect module dependency information.

What about that leaves you skeptical that this at least improves things?

Oh, I think the check is a good idea. I'm skeptical that it will fix a large proportion of the parallel build failures I see in my personal usage.

Ah well, onwards and upwards, then! 🙂