Merge lex/Makefile into the root Makefile

[shindere]

This continues the work started with e.g. #11248 and the yet unmerged #11268.

[dra27]

This is all fine - the comments I have are mostly typos. -safe-string could possibly be removed in a separate PR.

There are two places where pattern rules could add flags - in the case of the toplevel rules, it's quite concise; for adding warning flags back to the lex/ modules it involves some duplication. However, I think your plan is to implement these differently, so those two suggestions are more for discussion as to whether your existing plan for the module-specific flags would cover those patterns?

[dra27]

This could be done more concisely as:

toplevel/topdirs.cmx toplevel/toploop.cmx $(TOPLEVELSTART:.cmo=.cmx): \
  OC_NATIVE_CFLAGS+=-I toplevel/native

and then COMPILE_NATIVE_MODULE, COMPILE_NATIVE_TOPLEVEL_MODULE and TOPLEVELINIT could all be removed

[dra27]

Two typos: reserved and "the user can define"

[dra27]

Three typos: compile-time, contrary and e.g.

[dra27]

FWIW we could use this opportunity to remove -safe-string, as it's only there for compatibility.

[dra27]

Typo: generator

[dra27]

At this point, the additional warnings disabled by the merge could be re-enabled with:

lex/%.cmo: OC_COMMON_CFLAGS+=-w +40+41+66
lex/%.cmi: OC_COMMON_CFLAGS+=-w +40+41+66
lex/%.cmx: OC_COMMON_CFLAGS+=-w +40+41+66

[xavierleroy]

If it was not for pesky bootstrapping questions, ocamllex could be an independent program, separate from the core distribution. For this reason, I think it deserves its own makefile. Doing otherwise will make it harder to separate ocamllex from the core distribution if we decide to do this in the future.

[shindere]

Xavier Leroy (2022/07/12 06:13 -0700):

If it was not for pesky bootstrapping questions, ocamllex could be an independent program, separate from the core distribution. For this reason, I think it deserves its own makefile. Doing otherwise will make it harder to separate ocamllex from the core distribution if we decide to do this in the future.

It's already hard and I don't think this particular PR makes it really harder than it already is. Quite the opposite, I'd say. And given that, currently, the projects are not separated, it kind of makes sense to have the same build system controlling them, uniform ways to control which compiler and flags need to be used to compile them. If, for instance, one wants to compile ocamllex or the compiler itself with something else than the bootstrap compiler, which may be desirable in the context of cross-compilation, it seems better to be able to do so by defining varialbes only once and being sure that the result will be coherent, not by accident but by design and construction.

[shindere]

All checks have passed, so this should be ready for a hopefully final
review, @dra27.

Many thanks for your support, in any case.

[dra27]

I put this through precheck#760 just before the weekend because, well, build system.

This is good to go from my perspective. Given that lex/Makefile is already not standalone¹, I'm not sure that it's made accurately extracting it in future any harder (indeed, perhaps by looking less trivial, it might avoid missing subtle changes like ocaml/num#19).

Footnotes

1. It pulls in Makefile.common, and therefore the whole of the configuration infrastructure ↩

[shindere]

David Allsopp (2022/07/12 02:32 -0700):

This is all fine - the comments I have are mostly typos. -safe-string could possibly be removed in a separate PR.

Yes! That's a good idea! And many thanks for the feedback!

There are two places where pattern rules could add flags - in the case of the toplevel rules, it's quite concise; for adding warning flags back to the lex/ modules it involves some duplication. However, I think your plan is to implement these differently, so those two suggestions are more for discussion as to whether your existing plan for the module-specific flags would cover those patterns?

I am not sure they would. As you may remember, our conclusion was that we should have either module-specific flags and global flags. When we discussed and if I remember correctly, we explicitly decided not to have directory-level flags. We may have to give this point a second thought, though.

══════════════════════════════════════════════════════════════════════════ In [1]Makefile: +COMPILE_NATIVE_TOPLEVEL_MODULE = \ + $(COMPILE_NATIVE_MODULE) -I toplevel/native -c toplevel/topdirs.cmx: toplevel/topdirs.ml - $(CAMLOPT_CMD) $(COMPFLAGS) $(OPTCOMPFLAGS) -I toplevel/native -c $< + $(COMPILE_NATIVE_TOPLEVEL_MODULE) $< $(TOPLEVELINIT:.cmo=.cmx): $(TOPLEVELINIT:.cmo=.ml) \ toplevel/native/topeval.cmx - $(CAMLOPT_CMD) $(COMPFLAGS) $(OPTCOMPFLAGS) -I toplevel/native -c $< + $(COMPILE_NATIVE_TOPLEVEL_MODULE) $< $(TOPLEVELSTART:.cmo=.cmx): $(TOPLEVELSTART:.cmo=.ml) \ toplevel/native/topmain.cmx - $(CAMLOPT_CMD) $(COMPFLAGS) $(OPTCOMPFLAGS) -I toplevel/native -c $< + $(COMPILE_NATIVE_TOPLEVEL_MODULE) $< This could be done more concisely as: toplevel/topdirs.cmx toplevel/toploop.cmx $(TOPLEVELSTART:.cmo=.cmx): \ OC_NATIVE_CFLAGS+=-I toplevel/native and then COMPILE_NATIVE_MODULE, COMPILE_NATIVE_TOPLEVEL_MODULE and TOPLEVELINIT could all be removed

It's a good idea! Thanks! I implemented it and could indeed get rid of the `COMPILE_NATIVE_TOPLEVEL_MODULE` variable, but keeping the `COMPILE_NATIVE_MODULE` variable still seems sensible to me. Am I missing something here? Just to make one thing clear: there is a subtlety about GNU make that I am trying to take into account here. If a command in a recipe uses more than one line, then one has to \ to split hte line and the \ one uses will appear on the command's output, i.e. they will be passed to the shell and the command generated by make will indeed take several lines. On the countrary, if the whole command is assigned to a variable whose definition takes several lines, the \ that terminate the lines of the variable's definition will be processed by make and thus won't appear in the commands that are output. I think, trying to ensure that every output command, at least the simple shell commands that do not involve either loops or conditional constructs is something nice and desirable to have, because it makes it easier to compare outputs when doing a changes in the build system, it aovids noise. In other words, for a variable like `COMPILE_NATIVE_MODULE`, even if it appears only in one recipe, I think it makes sense to keep it for the reason explained above. Once the build system will be extended as required to cope with all the different cases, this variable definition will definitely span over several lines.

══════════════════════════════════════════════════════════════════════════ In [2]Makefile.build_config.in: > +# is for private variables (i.e. reseverd by the compiler's build system), +# the OCAML prefix is used for variables user can define to add their Two typos: reserved and "the user can define"

Fixed I think. Thanks.

══════════════════════════════════════════════════════════════════════════ In [3]Makefile.build_config.in: > +# ZZZ is either CFLAGS (compile-tme flags) or LDFLAGS (link-time flags). +# However, countrary to what is done for C compilers, the flags in the +# CFLAGS category are not passed at link time, so if a flag is needed +# at both stages, like e.e. -g, it should be added to both XXX_YYY_CFLAGS and Three typos: compile-time, contrary and e.g.

Also fixed. Thanks.

══════════════════════════════════════════════════════════════════════════ In [4]Makefile.build_config.in: > +# own flags and the module-name prefix is for flags that apply only +# to one module. +# +# YYY refers to the backend. At the moment, it can take the values +# COMMON for the flags shared by all the backends, BYTECODE or NATIVE +# (other backends may be added in the future). +# +# ZZZ is either CFLAGS (compile-tme flags) or LDFLAGS (link-time flags). +# However, countrary to what is done for C compilers, the flags in the +# CFLAGS category are not passed at link time, so if a flag is needed +# at both stages, like e.e. -g, it should be added to both XXX_YYY_CFLAGS and +# XXX_YYY_LDFLAGS. + +OC_COMMON_CFLAGS = -g -strict-sequence -principal -absname \ + -w +a-4-9-40-41-42-44-45-48-66 -warn-error +a -bin-annot \ + -safe-string -strict-formats FWIW we could use this opportunity to remove -safe-string, as it's only there for compatibility.

Yes. Good idea. To avoid the simultaneous modification of the same fragments, I'll wait until the opened build-system-related PRs are merged to open a new one for that purpose, but I'll do that ASAP and happily since it's a simplification.

══════════════════════════════════════════════════════════════════════════ In [5]Makefile: > @@ -1072,18 +1070,39 @@ libraryopt: partialclean:: $(MAKE) -C stdlib clean -# The lexer and parser generators +# The lexer generators Typo: generator

Sure. Fixed. Thanks.

══════════════════════════════════════════════════════════════════════════ In [6]Makefile: > @@ -1072,18 +1070,39 @@ libraryopt: partialclean:: $(MAKE) -C stdlib clean -# The lexer and parser generators +# The lexer generators + +ocamllex_MODULES = $(addprefix lex/,\ + cset syntax parser lexer table lexgen compact common output outputbis main) + At this point, the additional warnings disabled by the merge could be re-enabled with: lex/%.cmo: OC_COMMON_CFLAGS+=-w +40+41+66 lex/%.cmi: OC_COMMON_CFLAGS+=-w +40+41+66 lex/%.cmx: OC_COMMON_CFLAGS+=-w +40+41+66

That's one possibility, indeed. If I am reading the files correctly, I think warning 41 is disabled both in the root Makefile and in `lex/Makefile`. My understanding is that the only warnings which are disabled in the root Makefile but not in `lex/Makefile` are warnings 40 and 66. I have opened PRs #11498 and #11501 to modify the code so that they don't need to be disabled. If these two PRs can be merged then the discrepandy between the two lists of warnings would disappear.