Fix undefined Makefile variables#12022
Conversation
Wrong CPPFLAGS used when assembling with ASPP.
|
The additional code this enables for libasmrund (see runtime/amd64.S) might mean this warrants a Changes entry. |
| # $(SPACE) contains a single space | ||
| SPACE := $(EMPTY) $(EMPTY) | ||
| # $( ) suppresses warning from the alignments in the V_ macros below | ||
| $(SPACE) := |
There was a problem hiding this comment.
This is wizard-level Makefile hacking :)
There was a problem hiding this comment.
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.)
|
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.
|
Rather than just reporting un-prefixed symbol names, also report undefined variables.
|
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) |
|
Thanks @dra27 for your quick response and Makefile wizardry. We take Makefile hygiene seriously ! I'm going to merge in trunk |
|
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 |
Very few: I think we can live with this, esp. if we skip 5.0.1 to go straight to a 5.1.0 release. |
|
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) |
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_CPPFLAGSandOC_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 inamd64.Sfrom ever being enabled.OCAMLDEPFLAGS- default not added in Build system: make it possible to choose which ocamldep (and flags) to use #11126