Re: [PATCH v1 4/7] tools/ocaml: Makefile to drive dune

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 3 Aug 2022, at 14:46, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> 
> On Fri, Jul 29, 2022 at 06:53:27PM +0100, Edwin Török wrote:
>> create a separate Makefile that can be used to drive dune.
>> 
>> Usage:
>> `make -f Makefile.dune`
>> 
>> There are some files that need to be created by the Makefile based
>> build system (such as all the C code in $(XEN_ROOT)/tools/libs),
>> and those need to exist before dune runs.
>> 
>> Although it'd be possible to automatically call the necessary makefile
>> rules from dune, it wouldn't work reliably:
>> * dune uses sandboxing by default (only files declared or known as
>>  dependencies are visible to individual build commands,
>>  symlinks/hardlinks are used by dune to implement this)
>> * the dune builds always run in a _build subdir, and calling the
>>  makefiles from there would get the wrong XEN_ROOT set
>> * running the make command in the source tree would work, but dune still
>>  wouldn't immediately see the build dependencies since they wouldn't
>>  have been copied/linked under _build
>> 
>> The approach here is to:
>> * use the Makefile to build C-only prerequisites (i.e. most of Xen)
>> * use Dune only to build the OCaml parts once the C prerequisites exist
>> * dune has dependencies declared on the C bits, so if they are missing
>>  you will get an error about a missing rule to create them instead of a
>>  cryptic compilation error
>> * dune is still optional - the old Makefile based buildsystem is still
>>  there for now
>> * use dune exclusively for new code going forward (e.g. OCaml test-suites)
>> 
>> The workspace file needs to be generated by make because this currently
>> cannot be generated by dune, and it doesn't support including external
>> files. But could be generated by configure?
> 
> Potentially, but ./configure doesn't have access to the list of
> xen libraries, so I'm not sure it would be a good idea.

ok I'll remove it from the commit message.

> 
>> LD_LIBRARY_PATH needs to be set, because even with -Wl,-rpath
>> executables wouldn't be able to run using the just-built libraries,
>> unless we'd also link all the transitive dependencies of libs.
>> 
>> No functional change.
>> 
>> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
>> ---
>> Makefile                          |  5 ++
>> tools/ocaml/Makefile.dune         | 88 +++++++++++++++++++++++++++++++
>> tools/ocaml/dune-workspace.dev.in |  2 +
>> tools/ocaml/dune-workspace.in     | 18 +++++++
>> 4 files changed, 113 insertions(+)
>> create mode 100644 tools/ocaml/Makefile.dune
>> create mode 100644 tools/ocaml/dune-workspace.dev.in
>> create mode 100644 tools/ocaml/dune-workspace.in
> 
> You've added dune-workspace* to .gitignore in the previous patch, should
> the addition be done in this patch instead? (Also feel free to create
> "tools/ocaml/.gitignore".
> 
>> diff --git a/Makefile b/Makefile
>> index b93b22c752..ddb33c3555 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -68,6 +68,11 @@ build-tools-oxenstored: build-tools-public-headers
>>      $(MAKE) -s -C tools/libs
>>      $(MAKE) -C tools/ocaml build-tools-oxenstored
>> 
>> +.PHONY: build-tools-oxenstored-prepare
>> +build-tools-oxenstored-prepare: build-tools-public-headers
>> +    test -f tools/config.status || (cd tools && ./configure 
>> --with-xenstored=oxenstored)
> 
> No, do not run ./configure from the makefile. ./configure needs to be
> run before running make.


Perhaps I can add the necessary instructions to a README in tools/ocaml instead 
of this Makefile rule (which was here for documentation/convenience reasons).
The toplevel configure can fail due to various missing dependencies, but for 
OCaml just the configure in tools should be sufficient.

> 
>> +    $(MAKE) -C tools/libs V=
> 
> No, do not start a build of the libraries from the root make file. If a
> user were to run `make build-tools-oxenstored-prepare build-tools`, we
> would end up with 2 make running `make -C tools/libs` concurrently which
> isn't going to end well.


I'd like a single command to build everything needed related to oxenstored, 
without necessarily building the rest of Xen (which could either take a long 
time, or fail due to missing dependencies).
Ideally Xen wouldn't use recursive invocations of make, but just one single 
makefile that is aware of all source files (and you could then refer to 
objects/libraries in another directory as dependencies)
and what I'd like to do could be achieved by simply asking 'make' to build 
tools/ocaml/xenstored/oxenstored and let it figure out the minimal set of code 
that needs to be built for that.
However such a change would be quite invasive to the build system (and there 
probably was a reason to use recursive makefiles, they might have some 
advantage I'm not aware of).
Where do you recommend to put this rule instead, should it be in `tools/ocaml`? 
(although in that case it'd have to do a make invocation in `../libs` which 
isn't necessarily nicer)

Or should it be a shell script instead that invokes all the necessary make 
rules with the right flags, e.g. tools/ocaml/dev-build.sh?
 (and in that case there'd be no risk of the user running multiple make rules 
if the script itself takes no parameters).

> 
>> diff --git a/tools/ocaml/Makefile.dune b/tools/ocaml/Makefile.dune
>> new file mode 100644
>> index 0000000000..eca9cac0ca
>> --- /dev/null
>> +++ b/tools/ocaml/Makefile.dune
>> @@ -0,0 +1,88 @@
>> +XEN_ROOT = $(CURDIR)/../..
>> +all: dune-all-check
>> +
>> +# Dune by default uses all available CPUs. Make doesn't.
>> +# Query the available CPUs and use all available for any of the make rules 
>> we call out to.
>> +# -O is also needed with parallel make such that the build error and the 
>> build command causing
>> +#  the error are close together and not interspersed with other output
>> +NPROC=$(shell getconf _NPROCESSORS_ONLN)
>> +MAKEN=$(MAKE) -j$(NPROC) -O
> 
> Please, don't change those options, I don't think these options belong
> to a Makefile.


this Makefile is not (yet) linked to the toplevel makefiles, it is only used it 
you explicitly
'make -f Makefile.dune', in which case it saves some typing if you don't have 
to specify -j every time.
Would there be another way to achieve this? e.g. a dotfile in user home that 
make knows about?
(I think rpmbuild can be configured that way to use the correct -j flag, but 
I'm not aware whether Make can).
Perhaps the above dev-build.sh would be the solution here too (move the 
settings that don't belong into a makefile into a convenience shell script)

> 
>> +# We want to link and use the Xen libraries built locally
>> +# without installing them system-wide
>> +# (the system-wide one installed from packages will likely be too old and 
>> not match the locally
>> +# built one anyway).
>> +#
>> +# Set LIBRARY_PATH and LD_LIBRARY_PATH so that the linker
>> +# finds the proper libraries and the various dune commands
>> +# work (e.g. running tests, utop, etc.).
>> +#
>> +# The Makefile based buildsystem would use -Wl,-rpath-link= here,
>> +# but that only works during linking, not runtime.
>> +# There is a -Wl, -rpath= that can be used, but that only works
>> +# for libraries linked directly to the main executable:
>> +# the dependencies of those libraries won't get found on the rpath
>> +# (the rpath of the executable is apparently not used during that search).
> 
> That's why you do -Lpath -Wl,-rpath=path. Would the files generated in
> tools/pkg-config/ would be useful for dune?
> 
> LD_LIBRARY_PATH is kind of expected to run binaries, but how is
> LIBRARY_PATH used, and by which process?

I think LIBRARY_PATH is used by gcc/ld to find the just built libraries. I can 
try without them, but ISTR linking failing.
I could use the rpath flags, but then my binary would end up with rpaths 
inside, which isn't necessarily what I want
(although I think that is what happens currently). The overriden 
rpath/library_path/ld_library_path is only needed on the development machine,
not when deployed onto a box with the rest of the libraries installed into the 
correct places.

I think distributions would typically remove all the rpath handling code 
anyway, so the only user of rpaths left would be developers,
where setting LD_LIBRARY_PATH/LIBRARY_PATH is less intrusive than modifying all 
the build rules to add the rpaths.
I can try to see whether there is a non-intrusive way of adding rpaths, perhaps 
including a certain file wherever linker flags are specified,
which could be initially empty, but could contain rpaths when needed (or other 
compiler/linker flags).
Then at least rpath handling would be done in only one place (and only one 
place to immediately undo in the patchqueue).

> 
>> +# Use environment variables, because that way we don't make any permanent 
>> alternations (rpath)
>> +# to the executable, so once installed system-wide it won't refer to build 
>> paths anymore.
>> +#
>> +# Dune cannot be used to generate this file: the env-vars stanza doesn't 
>> support %{read:}, :include,
>> +# and dune-workspace doesn't support (include) stanzas.
>> +# So for now generate it from this Makefile
>> +# Cannot start with comment, so add auto-generated comment at the end
>> +LIB_DIRS=$(abspath $(wildcard ../libs/*/.))
> 
> Do you need all those libs? Can't you instead list the library needed
> or use the value listed in "tools/libs/uselibs.mk" ?

Indeed, I think some Xen source paths changed since this patch was originally 
written, an explicit list is probably a better choice now,
since there are a lot of libs there that are not necessarily needed (e.g. 
xenlight)

> 
>> +LIBRARY_PATH=$(subst $(eval) ,:,$(LIB_DIRS))
>> +../dune-workspace ../dune-workspace.dev: dune-workspace.in 
>> dune-workspace.dev.in Makefile.dune
>> +    @( sed -e "s|@LIBRARY_PATH@|$(LIBRARY_PATH)|" <$< \
>> +    && echo "; DO NOT EDIT: autogenerated from ocaml/dune-workspace.in") 
>> >../dune-workspace
>> +    @cat ../dune-workspace dune-workspace.dev.in >../dune-workspace.dev
>> +
>> +# for location of various libs which moves between Xen versions
>> +include $(XEN_ROOT)/tools/Rules.mk
>> +
>> +XEN_DEPS=$(XEN_libxenctrl)/libxenctrl.so
>> +XEN_DEPS+=$(XEN_libxenevtchn)/libxenevtchn.so
>> +XEN_DEPS+=$(XEN_libxenguest)/libxenguest.so
>> +
>> +# Cannot be generated from dune
>> +# Tell the user how to generate them
>> +../include/xen/xen.h ../config.status $(XEN_DEPS):
>> +    echo "Missing C headers or libraries" >&2
>> +    echo "Run make -C $(XEN_ROOT) build-tools-oxenstored-prepare 
>> -j$$(nproc)" >&2
>> +    exit 1
>> +
>> +# dune would refuse to run if there are build artifacts in the source 
>> directory
>> +# if we detect anything then run make clean to ensure these are removed
>> +# don't always call 'make clean' because it takes ~1.6s
>> +.PHONY: dune-pre
>> +dune-pre: ../config.status | ../include/xen/xen.h ../dune-workspace 
>> $(XEN_DEPS)
>> +    $(MAKEN) clean -s
> 
> I think it would be much better to tell the user to run clean themself,
> like we do in the hypervisor tree when trying to do an out-of-tree
> build. See rule "outputmakefile" in "xen/Makefile".


I could attempt to detect an unclean tree and abort the build instead with a 
message saying to run 'make clean'.
However detecting an unclean tree isn't necessarily trivial (although I think 
dune itself would detect and abort the build, so perhaps I can reuse that,
I'll have to do some experiments).
Does Xen support out-of-tree builds btw? That might be another option in 
maintaining a clean source tree without build artifacts.

> 
>> +
>> +# Convenience targets
>> +dune-syntax-check: dune-pre
>> +    dune build @check
>> +
>> +dune-all-check: dune-pre ../dune-workspace.dev
>> +    # Test build with multiple compiler versions
>> +    # requires opam switches for each to be already installed
>> +    dune build --workspace=../dune-workspace.dev @check @install @runtest
>> +
>> +check: dune-pre
>> +    dune runtest --no-buffer
>> +
>> +# approximatively equivalent to Dune 3.0 --release mode
>> +dune-oxenstored: dune-pre
>> +    dune build --root .. --ignore-promoted-rules --no-config \
>> +           --profile release --always-show-command-line \
>> +           --promote-install-files --default-target @install
>> +
>> +-include $(XEN_ROOT)/config/Paths.mk
> 
> I think make should fail if "Paths.mk" doesn't exist, could you remove
> the dash ? (Also, at this point, "Paths.mk" should already exist because
> Rules.mk checks that ./configure has run.)

Ok 

> )
>> +
>> +# skip doc, it'd install an extra LICENSE file that is already installed by 
>> other rules
>> +INSTALL_SECTIONS=bin,etc,lib,sbin
>> +dune-install: dune-oxenstored
>> +    dune install --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell 
>> ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) 
>> --docdir=$(docdir) --sections=$(INSTALL_SECTIONS)
> 
> Each option here could be on there own line, for clarity.

Ok

> 
>> +
>> +dune-uninstall: dune-oxenstored
>> +    dune uninstall --destdir=$(DESTDIR) --prefix=$(prefix) --libdir=$(shell 
>> ocamlfind printconf destdir) --mandir=$(mandir) --etcdir=$(sysconfdir) 
>> --docdir=$(docdir)
>> diff --git a/tools/ocaml/dune-workspace.dev.in 
>> b/tools/ocaml/dune-workspace.dev.in
>> new file mode 100644
>> index 0000000000..2ca831a048
>> --- /dev/null
>> +++ b/tools/ocaml/dune-workspace.dev.in
>> @@ -0,0 +1,2 @@
>> +(context default)
>> +(context (opam (switch 4.02.3) (profile release)))
>> diff --git a/tools/ocaml/dune-workspace.in b/tools/ocaml/dune-workspace.in
>> new file mode 100644
>> index 0000000000..c963a6e599
>> --- /dev/null
>> +++ b/tools/ocaml/dune-workspace.in
>> @@ -0,0 +1,18 @@
>> +(lang dune 2.1)
>> +
>> +(env
>> +  ; we need to support older compilers so don't make deprecation warnings 
>> fatal
>> + (dev
>> +  (flags (:standard -w -3))
>> +   (env-vars
>> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)
>> +    (LIBRARY_PATH @LIBRARY_PATH@)
>> +   ))
>> + (release
>> +  (env-vars
>> +   (OCAMLRUNPARAM b)
>> +    (LD_LIBRARY_PATH @LIBRARY_PATH@)
> 
> Shouldn't this line (and the next) been aligned with the previous one?

Yes

> 
>> +    (LIBRARY_PATH @LIBRARY_PATH@)
>> +  )
>> +  (flags (:standard -strict-sequence -strict-formats -principal -w @18))
>> +  (ocamlopt_flags -nodynlink)))


Thanks for the feedback, I'll think about how to best address some of these 
points (see above for some initial thoughts) and send out a V2 when ready.

Best regards,
--Edwin