Improve "undefined Makefile variables" warning and fix it for GNU make 4.4.1#12843
Improve "undefined Makefile variables" warning and fix it for GNU make 4.4.1#12843shindere merged 4 commits intoocaml:trunkfrom
Conversation
Previously, the CI result warned that undefined variables were detected, but left the user to scroll further up the log to find them. The list is now repeated!
|
cc @shindere |
|
The workaround for flexdll/Makefile is mirrored in ocaml/flexdll#128, but we'll need the fix here regardless. |
You are good at User Experience! We should nerd-snipe you into improving the compiler error messages. Maybe we should add some non-Windows-portable code in the type printer or something. |
|
🙂 Alas, the poor UX from before is also my fault (#12022) |
stdlib/Makefile
Outdated
| ifeq "$(USE_BOOT)" "" | ||
| CAMLC=$(OCAMLRUN) $(COMPILER) | ||
| else | ||
| CAMLC=$(BOOT_OCAMLC) $(CAMLC_FLAGS) |
There was a problem hiding this comment.
This change looks a bit suspicious to me because the name CAMLC_FLAGS suggests that all CAMLC flags are going to go there, but most flag-passing does not use this variable at all.
It is also not clear which variable involved was previously undefined. Can you explain?
(In the previous version I am not sure how overriding CAMLC from the caller works if it is defined unconditionally here on line previously-27. Argh, I shouldn't look at Makefiles...)
There was a problem hiding this comment.
If you compile this Dockerfile:
FROM ocaml/opam:alpine-3.19-opam
RUN git clone https://github.com/ocaml/ocaml.git && git -C ocaml checkout 3908dd368eaf391f671db33804a452ad1868fb3a
WORKDIR ocaml
RUN ./configure && make -j coldstartThe first line of coldstart results in this command:
make -C stdlib OCAMLRUN='$(ROOTDIR)/boot/ocamlrun' CAMLC='$(BOOT_OCAMLC) -use-prims ../runtime/primitives' allNote that $(ROOTDIR) and $(BOOT_OCAMLC) are being passed verbatim - i.e. they get expanded in stdlib/Makefile, not in the root Makefile. If you then add --warn-undefined-variables to that call and run it manually, you get:
$ make --warn-undefined-variables -C stdlib OCAMLRUN='$(ROOTDIR)/boot/ocamlrun' CAMLC='$(BOOT_OCAMLC) -use-prims ../runtime/primitives' all
make: Entering directory '/home/opam/ocaml/stdlib'
../Makefile.common:119: warning: undefined variable 'BOOT_OCAMLC'
make: Nothing to be done for 'all'.
make: Leaving directory '/home/opam/ocaml/stdlib'There was a problem hiding this comment.
The trick I'm doing here is instead of passing the entire CAMLC macro and overriding the runtime (which is what the OCAMLRUN and CAMLC definitions before did), I'm passing enough information to stdlib/Makefile to tell it to do it. It needs to know that it's being called by coldstart and what the additional flags to pass to the compiler should be. Which arguably suggests my name CAMLC_FLAGS is bad (open to suggestions!).
There was a problem hiding this comment.
With apologies for being slow, I still don't understand why BOOT_OCAMLC is missing there. It is defined in Makefile.common, which stdlib/Makefile correctly includes. I went to look at line 119 of Makefile.common, it reads like
ifeq "$(TEST_BOOT_OCAMLC_OPT)" "0"
and TEST_BOOT_OCAMLC_OPT does not seem to contain any call to CAMLC. Assuming there is an off-by-one disagreement in line numbers between make and editors, the line below is
BOOT_OCAMLC = $(ROOTDIR)/boot/ocamlc.opt
which also does not look like it could result in such a warning.
There was a problem hiding this comment.
Sorry, I've been unclear - I was trying to explain how the fix works, not what's presently wrong. I'm reasonably sure what's presently going wrong is a bug in GNU make 4.4.1, at the very least in the reporting of the location. Having diagnosed it as being that specific call, I elected to workaround it, as it blocks being able to run the undefined variable test in CI. I determined that if you removed the $(BOOT_OCAMLC) in make --warn-undefined-variables -C stdlib OCAMLRUN='$(ROOTDIR)/boot/ocamlrun' CAMLC='$(BOOT_OCAMLC) -use-prims ../runtime/primitives' all then the warning didn't trigger, so I therefore constructed a workaround which allowed me not to specify $(BOOT_OCAMLC) in that command-line.
I just tested, and the master branch of GNU make is still doing the same thing. Given that master is also reporting a fresh warning to do with our V_ macros it may be time for a bug report, but I was trying to focus on our Windows CI, and this is blocking it 🙂
|
That's great work as usual!
A few ridiculous comments / questions:
Regarding the first commit: I seem to remember that some failures of
`tools/ci/actions/runner.sh` were not that easy to understand. What I
remember is quite vague, but something along the line that when reading
too quickly the logs, one was under the impression that hte problem was
due to symbols not being defined properly in libraries, because the
`failed` variable was used for several things, I think. If that is still
the case, I think it would be worth trying to improve the situation,
although I am perfectly fine with that not being done in the present PR.
Re:`USE_BOOT`, the following (longer, sorry about that!) alternatives
come to my mind: `USE_BOOT_OCAMLC`, `BOOTSTRAPPING`. I have no
strong opinion on how to rename `CAMLC_FLAGS`, anyt hing works for me.
In `stdlib/Makefile`, on the line that says
CAMLC=$(BOOT_OCAMLC) $(CAMLC_FLAGS)
Can we please have spaces around the equal sign?
In the last commit, I have stumbled over `MSVCC_ROOT`: I thought it
should be `MSVC_ROOT` (one C only before the _), but after checking
flexdll's Makefile I can see that the name is actually correct.
|
GNU make 4.4.1 is triggering an undefined variable warning on the use of '$(BOOT_OCAMLC)' in a definition in the command line. It's not clear if this is intentional or not, but as the whole mechanism will ultimately disappear when stdlib/Makefile is merged into the root Makefile, this provides a viable workaround for the present setup.
When called with MSVC_DETECT=0, flexdll/Makefile does not define MSVCC_ROOT which triggers an undefined Makefile variable warning from make. Fixed upstream, but it needs to be shimmed when called from OCaml, regardless.
9eef65f to
24de0b1
Compare
|
Spaces added. I agree, |
There is no need to pass -use-prims to the stdlib Makefile as it doesn't link any executables, and primitive checking only ever occurs at link-time.
27827b6 to
dd9ae91
Compare
|
Following an offline discussion with @shindere, I realise that the It looks like it was added on a "replace all" basis to any Makefile which used the boot compiler. However, the primitive checking only ever occurs when linking, so it cannot ever be necessary for stdlib/Makefile, which only builds libraries, to have to worry about the primitives list. Removing it both removes the (in a fit of nostaglia, I checked out the 4.04 branch and confirmed that this change could have been applied at the time; a more systematic approach is to begin at |
|
As ever, many thanks for your remarkable and inspiring archeological
work and your generosity insharing the untderstandings you get from
digginginto the past andpresent sources of the compiler.
Just approved the PR and will merge as soon as CI is happy.
|
That's a subtle point, but it is very well taken! Thanks for digging into this. |
(cherry picked from commit b274ea5)
(cherry picked from commit b274ea5)
(cherry picked from commit b274ea5)
(cherry picked from commit b274ea5)
(cherry picked from commit b274ea5)
CI would dutifully report "Undefined Makefile variables detected" but leave the actual list of variables as an exercise for the reader. The list is now displayed too.
GNU make 4.4.1 triggers the warning on the way we presently invoke the stdlib
Makefile. While that's likely to change soon, it's a nuisance now when working on a system which has GNU make 4.4.1 (bleeding-edge Linux, macOS, or Windows).