Fix undefined Makefile variables

[dra27]

The majority of the warnings are coming from the V_ macros from #11844, which uses $ to allow indenting the commands by two spaces. This PR restores the CI check from #10270 for this which was accidentally lost in the multicore merge and also borrows the definition $(SPACE) := which was originally in that PR and silences the warning. The others:

• OC_DEBUG_CPPFLAGS and OC_INSTR_CPPFLAGS - not switched over to the new variable in Merge runtime/Makefile in the root Makefile #11248; these flags were introduced as part of the multicore merge, and their absence will have only prevented debugging statements in amd64.S from ever being enabled.
• OCAMLDEPFLAGS - default not added in Build system: make it possible to choose which ocamldep (and flags) to use #11126

[dra27]

The additional code this enables for libasmrund (see runtime/amd64.S) might mean this warrants a Changes entry.

[nojb]

LGTM! Thanks.

[nojb]

This is wizard-level Makefile hacking :)

[dra27]

It's also possible to do it by having, e.g.

V_CC         = @$(info $(SPACE) CC $@)

but that looks a little odd. More importantly, it's relatively easy to find $ recommended as a way of escaping a space, but rather harder to find the trick necessary to suppress the undefined variable warning, so it seems worth having the slightly strange trick above. Incidentally, that macro is principally used for escaping newlines without adding space (by writing $\ instead of \ at the end of the line.)

[shindere]

In the first commit, you have these two lines: ``` echo Ensuring that all names are prefixed in the runtime ./tools/check-symbol-names runtime/*.a otherlibs/*/lib*.a ``` which come between the call to make and the `if grep -Fq ' warning: undefined variable ' build.log; then` test. Wouldn't it be better to test for undefinedvariables right after the call to make and then only call `tools/check-symbol-names`? And many thanksforthe other fixes which look good to me and for your so prompt response! I am going to approve right now. I'd be happy to have the suggestion above taken into account if it makes sense to you, but the PR is good to go as-is, as far as I am concerned.

[dra27]

Thanks, @shindere! I’ve restructured that block slightly so both tests get done, with a failure in either halting the build only after both have been run (a little like the hygiene checks which try to test as much as possible)

[xavierleroy]

Thanks @dra27 for your quick response and Makefile wizardry. We take Makefile hygiene seriously ! I'm going to merge in trunk and backport to 5.0 (as suggested by your milestone)

[xavierleroy]

Actually, it doesn't backport cleanly to 5.0, because in 5.0 we did not have the nice elided printing of commands. I'm running a quick build of 5.0 to see how many warnings we get from make --warn-undefined-variables.

[xavierleroy]

I'm running a quick build of 5.0 to see how many warnings we get from make --warn-undefined-variables.

Very few:

Makefile:44: warning: undefined variable 'FORCE_INSTRUMENTED_RUNTIME'
Makefile:956: warning: undefined variable 'OC_DEBUG_CPPFLAGS'
Makefile:959: warning: undefined variable 'OC_INSTR_CPPFLAGS'
dynlink_compilerlibs/Makefile:19: warning: undefined variable 'OCAMLDEPFLAGS'

I think we can live with this, esp. if we skip 5.0.1 to go straight to a 5.1.0 release.

[dra27]

No problem to back-port it to 5.0, just to keep CI there clean too (5.1 I think already features some of the more usual incompatibilities in parsetree, etc. that we avoided between 4.14 and 5.0, so we might not be able to having a maintenance release of 5.0, even if we're fully expecting everyone who's on 5.0 to leap on 5.1)