Skip to content

Merge tools/Makefile into the root Makefile #11675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 29 commits into from
Dec 13, 2022

Conversation

shindere
Copy link
Contributor

This continues the work started in #11243, #11248, #11268 and #11420.

As these former pull requests, the present one is best reviewed in a
commit-by-commit way.

The PR starts by fixing the build of ocamllex by taking the link flags
into account again -- they got accidentally lost in #11420.

After a few minor commits, a more substantial one adjusts the compilation
flags used to compile and link the programs in the tools/ directory so that
they match those used by the root Makefile. In particular, this
implied the creation of a few interface files which had been forgotten
in #11288, which in a way demonstrates how hard it was/is to e.g. enable
or disable a given warning all over our codebase.

In the same vein, a few commits work on distinguishing the compilation
and link stages for a few tools: rather than building them from their .ml
sources directly, we make sure to first compile them to .cmo files and then
only link them. This is so that the compilation can use the already existing
compilation rules rather than dedicated ones and, that way, make sure
the same flags are passed to the compilers consistently.

This PR also starts to use GNU make's VPATH variable to let make
know where to look for prerequisites. This variable is then
used to compute which directories to pass to the compiler as arguments
of -I. That way, we make sure the compiler and GNU make agree
on where to look for files.

In addition to a few other simplifications, among which there is the
computation of the LN variable during configure rather than during build,
this PR extends the existing build framework with a few build
variables and, more importantly, with a few macros that make linking OCaml
programs more concise and uniform.

Basically, to get a program foo linked, list its libraries in
foo_LIBRARIES, its modules in foo_MODULES and call one of the provided
link macros depending on whether you want to produce both bytecode and
native versions of foo (with OCAML_PROGRAM), only its bytecode
version (with OCAML_BYTECODE_PROGRAM) or only its native versoin
(with, guess?... OCAML_NATIVE_PROGRAM, yes).

Finally, before this PR the cmt and cmit source artifacts of
tools/ocamlmktop_init were not installed as they should have been. The PR
fixes this and installs them.

@shindere shindere force-pushed the merge-tools-makefile branch 3 times, most recently from 526272f to 10c0a41 Compare October 28, 2022 05:06
@shindere shindere force-pushed the merge-tools-makefile branch from 10c0a41 to d5e7932 Compare November 17, 2022 09:33
@shindere shindere force-pushed the merge-tools-makefile branch from d5e7932 to 11de2f1 Compare November 24, 2022 17:19
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As ever, a thorough piece of work, and quite the nettle you've grasped again with this one, @shindere! It's pleasing that a 353 line Makefile is removed with only 209 lines added elsewhere in its place :)

I was concerned - incorrectly - that this might affect Cygwin64 when built in bootstrapped flexdll mode but this is working fine (I think that's due to a combination of your work on utils/Makefile and some of mine on $(MKEXE) et al). I also bumped all the magic numbers, and bootstrapping also seems to be working as expected.

I think you could squash the "Makefile.common: simplify the generic macros" into the previous commit, but it's fine if you want to keep the two separate versions.

Most of my comments are cosmetic or typos - the only major thing which needs fixing is the doubling of commands caused by ocamltoolsopt and ocamltoolsopt.opt both making the same recursive call to make in parallel in the opt.opt target.

In passing:

  • In tools/ocamlmklib.ml, the syslib function is a leftover from ocamlbuild days and the chop_suffix functions appears never to have been used.
  • The spurious inclusion of depend.cmi in the dependencies of ocamldep is a hangover from pre#1078 days

Makefile Outdated

partialclean::
rm -rf ocamlc$(EXE)
rm -rf ocamlc ocamlc.exe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was "wrong" before, but this should just be rm -f (no -r)

Makefile Outdated

partialclean::
rm -f ocamlopt$(EXE)
rm -f ocamlopt ocamlopt.exe ocamlopt.opt ocmalopt.opt.exe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: ocamlopt.opt.exe

Makefile Outdated

partialclean::
rm -f ocamlopt.opt$(EXE)
rm -f ocamlc.opt ocamlc.opt.exe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to move these up with the bytecode version above (as is now the case for ocamlopt / ocamlopt.opt)

Makefile Outdated
@@ -1069,6 +1053,8 @@ partialclean::

# The lexer generator

ocamllex_LIBRARIES =\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray line continuation

Makefile Outdated
compilerlibs/ocamltoplevel.cmxa \
$(TOPLEVELSTART:.cmo=.cmx)
ocamlnat_LIBRARIES = \
$(addprefix compilerlibs/ocaml,common optcomp bytecomp) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reads strangely with 3 of these being prefixed with addprefix and ocamltoplevel not being. In this instance, might it be better just to have the list of 5 fully expanded (i.e. like the previous list, just without the extensions)

Makefile Outdated
@@ -416,37 +407,42 @@ clean::
# The clean target
clean:: partialclean
rm -f configure~ $(PROGRAMS) $(PROGRAMS:=.exe)
rm $(ocamllex_PROGRAMS) $(ocamllex_PROGRAMS:=.exe)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets corrected in a later commit, but this should be rm -f

Makefile Outdated
Comment on lines 1307 to 1303
TOOLS_TO_INSTALL = \
ocamldep ocamlprof ocamlcp ocamloptp ocamlmktop ocamlmklib ocamlobjinfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to have $(TOOLS_TO_INSTALL) defined alongside the $(TOOLS) variable further up. It might also be clearer to put:

TOOLS_TO_INSTALL = ocamldep ocamlprof ocamlcp ..
TOOLS = $(TOOLS_TO_INSTALL) ocamlcmt dumpobj ...

?

Makefile Outdated
otherlibs/str/str
lintapidiff_MODULES = tools/lintapidiff

tools/lintapidiff.opt$(EXE): VPATH += $(ROOTDIR)/otherlibs/str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one doesn't need $(ROOTDIR). It looks like there a few tweaks needed to be able to say make lintapidiff - 5f2f478 seemed to be working for me (at least to get it to build!)

@shindere shindere force-pushed the merge-tools-makefile branch 15 times, most recently from 278f373 to df30414 Compare December 8, 2022 11:28
@shindere
Copy link
Contributor Author

shindere commented Dec 8, 2022 via email

@dra27
Copy link
Member

dra27 commented Dec 9, 2022

All good, thanks @shindere! Two specific replies:

for the issue with the opt and opt.opt targets:

I am unsure how to go about this, then.

I'll pull the revised branch and have a look (apropos our offline discussion)

for the overriding of CAMLC in ocamltex's compilation:

I did add the comment. It's jsut that the definitions apply to all theprerequisites so even to the libraries I think, so I am wondering whether this is quite right. What do you think?

Hmm, it's kind of a dirty "secret" of the build at the moment that compilerlibs is compiled using $(ROOTDIR)/boot/ocamlc but has to be linkable by programs compiled with $(ROOTDIR)/ocamlc. Indeed, at the moment, if you do a build, then, say, touch parsing/location.ml tools/ocamltex.ml ; make tools/ocamltex then ocamlcommon.cma gets incorrectly rebuilt using $(ROOTDIR)/ocamlc instead of $(ROOTDIR)/boot/ocamlc. That can be changed by having an explicit compilerlibs/ocamlcommon.cma: CAMLC = ... in compilerlibs/Makefile.compilerlibs - perhaps that a good idea, therefore, to make the compiler used for the libraries explicit?

I applied this inelegant copy-and-paste diff:

--- a/compilerlibs/Makefile.compilerlibs
+++ b/compilerlibs/Makefile.compilerlibs
@@ -416,6 +416,7 @@ utils/config_%.mli: utils/config.mli

 beforedepend:: utils/config_main.mli utils/config_boot.mli

+compilerlibs/ocamlcommon.cma: CAMLC = $(BOOT_OCAMLC) $(BOOT_STDLIBFLAGS) -use-prims runtime/primitives
 compilerlibs/ocamlcommon.cma: $(COMMON_CMI) $(ALL_CONFIG_CMO) $(COMMON)
        $(CAMLC) -a -linkall -o $@ $(COMMON)
 partialclean::

and then did the touch parsing/location.ml ... from above and it looks like location.cmo and ocamlcommon.cma are correctly produced using $(ROOTDIR)/boot/ocamlc and then ocamltex.cmo and ocamltex using $(ROOTDIR)/ocamlc:

dra@thor:~/ocaml-7$ touch parsing/location.ml tools/ocamltex.ml
dra@thor:~/ocaml-7$ make tools/ocamltex
./boot/ocamlrun ./boot/ocamlc -nostdlib ... -c parsing/location.ml
./boot/ocamlrun ./boot/ocamlc -nostdlib ... -o compilerlibs/ocamlcommon.cma ...
./boot/ocamlrun ./ocamlc -nostdlib ... -c tools/ocamltex.ml
./boot/ocamlrun ./ocamlc -nostdlib ... -o tools/ocamltex ...

It may also be possible to use a pattern rule to apply that a bit more elegantly (I think that might even be stable with the newer prefix-length, rather than order-in-makefile, rules for applying pattern rules?)

(* OCaml *)
(* *)
(* Damien Doligez and Francois Rouaix, INRIA Rocquencourt *)
(* Ported to Caml Special Light by John Malecki *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong copyright?

Makefile Outdated
git ls-files -- 'otherlibs/*/*.mli' 'stdlib/*.mli' |\
grep -Ev internal\|obj\|stdLabels\|moreLabels |\
tools/lintapidiff.opt $(VERSIONS)

# Tools

TOOLS_BYTECODE_TARGETS = $(TOOLS_PROGRAMS) $(TOOLS_MODULES:=.cmo)

TOOLS_NATIVE_TARGETS = $(TOOLS_PROGRAMSS:=.opt) $(TOOLS_MODULES:=.cmx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: extra S at the end of TOOLS_PROGRAMS

The rules were using the LINKFLAGS variable whose definition had been
lost while merging lex/Makefile in the root Makefile.
Remove leftover from commit 3e52e4e

Also remove the no longer used DEF_SYMBOL_PREFIX variable
The compilation of tools/checkstack.c to tools/checkstack.$(O) was
already happening in the root Makefile, so it is more coherent to also
link the executable file in the same Makefile.
@shindere shindere force-pushed the merge-tools-makefile branch from df30414 to 5966b1b Compare December 12, 2022 09:13
The purpose of this commit is to use the same compiler flags to compile the
programs in the tools/ directory than those used in the root Makefile.

In addition to a bit of re-ordering, this means two changes:

 1. Disabling warning 40 in tools. It was enabled before this commit but
    the code didn't trigger it.
 2. Enabling warning 70, which required the creation of a bunch of
    interface files.

Also, the -g flag has been moved from the CAMLC/CAMLOPT variables to
the COMPFLAGS and LINKFLAGS variables and added to the recipe that
builds make_opcodes.

Moreover, the - appearing in the compilation rules
for .cmi[iox] files just before the file to be compiled has been
removed and the flags in these recipes have been re-ordered so that -c
appears at the end of the recipe, just before the name of the file to compile.
@shindere shindere force-pushed the merge-tools-makefile branch from 5966b1b to c4a9171 Compare December 12, 2022 09:31
@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email

@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email

@shindere shindere force-pushed the merge-tools-makefile branch from c4a9171 to 2c4fd25 Compare December 12, 2022 09:35
@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email

@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email


# OCaml programs that are compiled in both bytecode and native code

OCAML_PROGRAMS = ocamlc ocamlopt lex/ocamllex $(TOOLS_PROGRAMS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools/ocamloptp needs adding to this list (as it's used for the clean target) - this is the cause of the CI failure at the moment

@dra27
Copy link
Member

dra27 commented Dec 12, 2022

My understanding is that will no longer be true once the work on the
build system has been completed. At least that this will not have to be
true. Do you also understand things that way, @dra27?

Yes, that's right - I was anticipating that at some point we'll be able to tell trivially whether a separate set of compiler-libs is needed, or not.

Do you mean to apply this to all the bytecode libraries built in
compilerlibs? So something like

compilerlibs/%.cma: CAMLC = $(BOOT_OCAMLC) $(BOOT_STDLIBFLAGS) -use-prims runtime/primitives

Yes (apart from the clean check which is failing in CI) - I just don't know (without trying) how the pattern rules will interact.

By the way: except if we want to add the presently discussed change, the
PR should now be ready for a last review and, maybe, to be merged.

Yes, I agree! I think it might be worth trying to fix this issue here, though - it does at least sit right at the end of the commit series.

@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email

@shindere shindere force-pushed the merge-tools-makefile branch from 2a5cb7c to dc15bb5 Compare December 12, 2022 15:22
@shindere
Copy link
Contributor Author

shindere commented Dec 12, 2022 via email

@shindere shindere force-pushed the merge-tools-makefile branch 2 times, most recently from f2cfdfe to 6623576 Compare December 13, 2022 12:49
This is because the configuration is not available during clenaup
so we cannot rely on it to determine whether ocamloptp etc. have
been built.
@shindere shindere force-pushed the merge-tools-makefile branch from 6623576 to f61b76e Compare December 13, 2022 13:00
Now that each tool specifies on which libraries it depends, there is
no need to wait until all the compiler libs have been built to start
building the tools. The build can thus become more parallel and thus faster.
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making a journey through precheck#807 and assuming that doesn't show up anything, this is good to go with CI!

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mingw32 failure in Jenkins is a sporadic memory model test failure - this is ready to go. A.OUT1!

Footnotes

  1. see last section of https://www.bell-labs.com/usr/dmr/www/odd.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants