| Message ID | 322ec0c9af480e9b8a6246d0a2cdb4e308a5900c.1659116941.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/ocaml code and build cleanups | expand |
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@citrix.com> > --- > 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,
> On 3 Aug 2022, at 14:46, Anthony PERARD <anthony.perard@citrix.com> 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@citrix.com> >> --- >> 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
On Wed, Aug 03, 2022 at 03:37:19PM +0000, Edwin Torok wrote: > > On 3 Aug 2022, at 14:46, Anthony PERARD <anthony.perard@citrix.com> 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@citrix.com/ 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,
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) + $(MAKE) -C tools/libs V= + .PHONY: build-stubdom build-stubdom: mini-os-dir build-tools-public-headers $(MAKE) -C stubdom build 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 + +# 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). +# +# 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/*/.)) +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 + +# 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 + +# 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) + +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@) + (LIBRARY_PATH @LIBRARY_PATH@) + ) + (flags (:standard -strict-sequence -strict-formats -principal -w @18)) + (ocamlopt_flags -nodynlink)))
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? 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@citrix.com> --- 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