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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.

> 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.

> +     $(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.

> 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.

> +# 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?

> +# 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" ?

> +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".

> +
> +# 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.)
)
> +
> +# 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.

> +
> +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?

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

Thanks,

-- 
Anthony PERARD