Re: [Xen-devel] [PATCH for-4.9] build: stubdom and tools should depend on public header target

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH for-4.9] build: stubdom and tools should depend on 
public header target"):
> Build can fail if stubdom build is run before tools build because:
> 
> 1. tools/include build uses relative path and depends on XEN_OS
> 2. stubdom needs tools/include to be built, at which time XEN_OS is
>    mini-os and corresponding symlinks are created
> 3. libraries inside tools needs tools/include to be built, at which
>    time XEN_OS is the host os name, but symlinks won't be created
>    because they are already there
> 4. libraries get the wrong headers and fail to build

The new code in the Makefiles LGTM.  I have only one nit, which is
that style for Makefile targets seems to be to use `-' rather than `_'
as a word separator.

Anyway,

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

I would like to put on record here an observation I made IRL:

When doing recursive make, to have make -j work properly, it is in
general necessary for the call graph between Makefiles be a tree.[1]
This is because   $(MAKE) -jN -C DIRECTORY  is not safely reentrant,
and there is no deduplication or locking in make.

For programmer sanity, the Makefile call graph should be a subgraph of
the directory graph.

What your patch does is lift the $(MAKE) -C tools/include invocation
into the common parent of the two invocation Makefiles, adding
appropriate dependencies.  This is the correct approach, as we
discussed IRL.


I did a grep and found the following problems:

docs/Makefile:VERSION           := $(shell $(MAKE) -C $(XEN_ROOT)/xen 
--no-print-directory xenversion)
tools/flask/policy/Makefile.common:POLICY_FILENAME = 
$(FLASK_BUILD_DIR)/xenpolicy-$(shell $(MAKE) -C $(XEN_ROOT)/xen xenversion 
--no-print-directory)

This is probably provably correct despite violating the rule.[1]
We should probably add some comments to xen/Makefile explain tha
the `xenversion' target must be reentrant.

stubdom/Makefile:       $(MAKE) -C $(XEN_ROOT)/tools/include

This is fixed by your patch.

tools/debugger/gdbsx/xg/Makefile:       $(MAKE) -C ../../../include
stubdom/Makefile:       $(MAKE) DESTDIR= -C $(XEN_ROOT)/tools 
qemu-xen-traditional-dir-find

These look broken.  Maybe qemu-xen-traditional-dir-find is reentrant.

xen/xsm/flask/Makefile: $(MAKE) -f 
$(XEN_ROOT)/tools/flask/policy/Makefile.common -C 
$(XEN_ROOT)/tools/flask/policy FLASK_BUILD_DIR=$(FLASK_BUILD_DIR)

This seems to be a violation of the directory tree subgraph principle,
but may be fine.  I haven't investigated.

stubdom/Makefile:       cd $@/build; CC=${CC} $(CMAKE) .. 
-DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) 
$(TARGET_CFLAGS) -Wno-declaration-after-statement"

This rune matched my   git-grep 'MAKE.*\.\.'  but actually it is
reinvoking the very same Makefile using CMAKE.  I assume that the
author knew what they were doing.  The problem is in any case local.

Ian.


[1] It can be possible to prove that a particular non-tree Makefile
call graph is correct, depending on exactly what the targets do, but
this is tricky.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel