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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Aug 03, 2022 at 03:37:19PM +0000, Edwin Torok wrote:
> > 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:
> >> +.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).

That would be fine.

> The toplevel configure can fail due to various missing dependencies, but for 
> OCaml just the configure in tools should be sufficient.

:-), it doesn't looks like the others ./configure have much dependencies
that aren't also needed in tools, so I don't think it make sense to
avoid the toplevel ./configure at this time. We don't really test
running ./configure in only a subsystem so it's probably best to avoid
documenting it. You can always run `./configure --disable-{xen,docs}` to
run the minimum amount of ./configure.

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

Easy:
    ./configure && make -C tools/include && make -C tools/libs
Hopefully, this would be enough, will keep working for a while. But it
might break. (I would try to keep that command above working but who
knows if change would be needed)

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

Non-recursive makefiles, I want that as well!! :-)
I'm working on it:
    [XEN PATCH v3 00/25] Toolstack build system improvement, toward 
non-recursive makefiles
    
https://lore.kernel.org/all/20220624160422.53457-1-anthony.perard@xxxxxxxxxx/
But that's going to take a while. There's till a lot of patches that I
haven't posted yet.

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

I'm not sure, these are just kind of "optimisation" to workaround our
build system. It could easily apply to many subdir in tools/, but
there's no documentation for it. One could run make in most subdir after
just running `./configure && make -C tools/include && make -C
tools/libs`, but I don't think we should document it. As documenting it
makes it harder to make changes if needed.

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

Use the "alias" built-in from your shell, if you want to have -j$(nproc)
added to you `make` commands.

> Would there be another way to achieve this? e.g. a dotfile in user home that 
> make knows about?

There's .bashrc or .zshrc or ..., make doesn't need to know. ;-)

> (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 of $LD_LIBRARY_PATH is fine, it's already used in several places. I
guess "-Wl,-rpath" could be used instead but is probably best to avoid as
not normally needed.

Now about link time, normally we seem to have the full path to the
library we want to link to, or we provide '-L' via pkg-config file.
Then, when a library is links against another one, and the linker wants
to know where this library is, we use "-rpath-link" and I might be
wrong but this probably doesn't add anything in the library.

It seems to me that $LIBRARY_PATH might be useful for external project,
we would want to links against the libraries that aren't yet installed
on the system. But event that isn't needed as we generates
"tools/pkg-config/" which have the needed flags. We use that for
building qemu for example, and I think qemu's configure can make use of
that.

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

I'd mostly like to avoid $(wildcard ) if possible.

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

The comment already speak detecting unclean source tree, but I guess it
probably still run `make clean` even if 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.

The toolstack build system doesn't support out-of-tree build. But my
work on non-recursive makefile would potentially allow that.

Having the dune stuff not been run when running for example `make
build-tools` is probably going to make this kind of things awkward at
first. That is both dune and make buids could be use in parallel (not
necessary at the same time) if the developer doesn't pay attention. I
guess later if could have something like `./configure
--enable-ocaml-dune`, there will be less risk of having the make build
ocaml stuff instead of dune as intended by a developer. Also, this could
allow to test dune build in our CI.

Cheers,

-- 
Anthony PERARD