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

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

[Wei Liu]

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

Since both tools and stubdom build need the public headers, we build
tools/include before stubdom and tools. Remove runes in stubdom and
tools to avoid building tools/include more than once.

The new arrangement ensures tools build gets the correct headers
because XEN_OS is set to host os when building tools/include. As for
stubdom, it explicitly links to the mini-os directory without relying
on XEN_OS so it should fine.

Reported-by: Steven Haigh <netwiz@crc.id.au>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Steven Haigh <netwiz@crc.id.au>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: Julien Grall <Julien.Grall@arm.com>
---
 Makefile         | 10 +++++++---
 stubdom/Makefile |  1 -
 tools/Makefile   |  3 +--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 084588e11e..6c837d7522 100644
--- a/Makefile
+++ b/Makefile
@@ -38,6 +38,10 @@ mini-os-dir-force-update: mini-os-dir
 export XEN_TARGET_ARCH
 export DESTDIR
 
+.PHONY: tools_public_headers
+tools_public_headers:
+	$(MAKE) -C tools/include
+
 # build and install everything into the standard system directories
 .PHONY: install
 install: $(TARGS_INSTALL)
@@ -50,11 +54,11 @@ build-xen:
 	$(MAKE) -C xen build
 
 .PHONY: build-tools
-build-tools:
+build-tools: tools_public_headers
 	$(MAKE) -C tools build
 
 .PHONY: build-stubdom
-build-stubdom: mini-os-dir
+build-stubdom: mini-os-dir tools_public_headers
 	$(MAKE) -C stubdom build
 ifeq (x86_64,$(XEN_TARGET_ARCH))
 	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom pv-grub
@@ -101,7 +105,7 @@ install-tools:
 	$(MAKE) -C tools install
 
 .PHONY: install-stubdom
-install-stubdom: mini-os-dir
+install-stubdom: mini-os-dir tools_public_headers
 	$(MAKE) -C stubdom install
 ifeq (x86_64,$(XEN_TARGET_ARCH))
 	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom install-grub
diff --git a/stubdom/Makefile b/stubdom/Makefile
index aef705dd1e..db01827070 100644
--- a/stubdom/Makefile
+++ b/stubdom/Makefile
@@ -355,7 +355,6 @@ LINK_DIRS := libxc-$(XEN_TARGET_ARCH) xenstore $(foreach dir,$(LINK_LIBS_DIRS),l
 LINK_STAMPS := $(foreach dir,$(LINK_DIRS),$(dir)/stamp)
 
 mk-headers-$(XEN_TARGET_ARCH): $(IOEMU_LINKFARM_TARGET) $(LINK_STAMPS)
-	$(MAKE) -C $(XEN_ROOT)/tools/include
 	mkdir -p include/xen && \
           ln -sf $(wildcard $(XEN_ROOT)/xen/include/public/*.h) include/xen && \
           ln -sf $(addprefix $(XEN_ROOT)/xen/include/public/,arch-x86 hvm io xsm) include/xen && \
diff --git a/tools/Makefile b/tools/Makefile
index 1396d95b50..496428e3a9 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -5,7 +5,6 @@ export PKG_CONFIG_DIR = $(CURDIR)/pkg-config
 include $(XEN_ROOT)/tools/Rules.mk
 
 SUBDIRS-y :=
-SUBDIRS-y += include
 SUBDIRS-y += libs
 SUBDIRS-y += libxc
 SUBDIRS-y += flask
@@ -50,7 +49,7 @@ SUBDIRS-$(OCAML_TOOLS) += ocaml
 endif
 
 ifeq ($(CONFIG_RUMP),y)
-SUBDIRS-y := include libxc xenstore
+SUBDIRS-y := libxc xenstore
 endif
 
 # For the sake of linking, set the sys-root
-- 
2.11.0


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

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

[Ian Jackson]

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@eu.citrix.com>

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Wei Liu]

On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> 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.

IIRC at one point I used '-' in mini-os build system but some version of
make didn't like it. So I stick with '_' since.

> 
> Anyway,
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 

Sadly this patch is still not enough because other targets of
tools/include are not invoked.

I will send out another patch later.

Wei.

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

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

[Ian Jackson]

Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend on public header target"):
> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> > 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.
> 
> IIRC at one point I used '-' in mini-os build system but some version of
> make didn't like it. So I stick with '_' since.

I think you are confused.  You are probably thinking of variable names
which cannot contain -.  (Well, which are troublesome if they do.)

As you can see, our Makefiles are full of target names containing -.

Ian.

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

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

[Wei Liu]

On Wed, May 17, 2017 at 03:20:08PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend on public header target"):
> > On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> > > 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.
> > 
> > IIRC at one point I used '-' in mini-os build system but some version of
> > make didn't like it. So I stick with '_' since.
> 
> I think you are confused.  You are probably thinking of variable names
> which cannot contain -.  (Well, which are troublesome if they do.)
> 
> As you can see, our Makefiles are full of target names containing -.

Ah, yes, indeed. I confused variable names with target names.

> 
> Ian.

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

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

[Jan Beulich]

>>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
> Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
> on public header target"):
>> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
>> > 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.
>> 
>> IIRC at one point I used '-' in mini-os build system but some version of
>> make didn't like it. So I stick with '_' since.
> 
> I think you are confused.  You are probably thinking of variable names
> which cannot contain -.  (Well, which are troublesome if they do.)

Troublesome? I'm pretty sure we use such somewhere in the
hypervisor tree...

Jan


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

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

[Wei Liu]

On Wed, May 17, 2017 at 08:28:40AM -0600, Jan Beulich wrote:
> >>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
> > Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
> > on public header target"):
> >> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> >> > 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.
> >> 
> >> IIRC at one point I used '-' in mini-os build system but some version of
> >> make didn't like it. So I stick with '_' since.
> > 
> > I think you are confused.  You are probably thinking of variable names
> > which cannot contain -.  (Well, which are troublesome if they do.)
> 
> Troublesome? I'm pretty sure we use such somewhere in the
> hypervisor tree...

Then we'd better fix it sooner rather than later. :p

I dig out the patch. And this is in the commit message:

    In the GNU make manual "How to Use Variables" section there is such
    word:

    "However, variable names containing characters other than letters,
    numbers, and underscores should be considered carefully, as in some
    shells they cannot be passed through the environment to a sub-make (see
    Communicating Variables to a Sub-make)."

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

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

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

[Wei Liu]

On Wed, May 17, 2017 at 03:51:11PM +0100, Wei Liu wrote:
> On Wed, May 17, 2017 at 08:28:40AM -0600, Jan Beulich wrote:
> > >>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
> > > Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
> > > on public header target"):
> > >> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> > >> > 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.
> > >> 
> > >> IIRC at one point I used '-' in mini-os build system but some version of
> > >> make didn't like it. So I stick with '_' since.
> > > 
> > > I think you are confused.  You are probably thinking of variable names
> > > which cannot contain -.  (Well, which are troublesome if they do.)
> > 
> > Troublesome? I'm pretty sure we use such somewhere in the
> > hypervisor tree...
> 
> Then we'd better fix it sooner rather than later. :p
> 
> I dig out the patch. And this is in the commit message:
> 
>     In the GNU make manual "How to Use Variables" section there is such
>     word:
> 
>     "However, variable names containing characters other than letters,
>     numbers, and underscores should be considered carefully, as in some
>     shells they cannot be passed through the environment to a sub-make (see
>     Communicating Variables to a Sub-make)."
> 

$ cd xen.git/xen
$ git-ls-files | grep '\(Rules.*\|Makefile.*\|[Cc]onfig.*\)' | xargs \
   grep -nH '[A-Z_]+-.*' # yields nothing

So we're probably fine in xen directory.

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

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

[Jan Beulich]

>>> On 17.05.17 at 16:51, <wei.liu2@citrix.com> wrote:
> On Wed, May 17, 2017 at 08:28:40AM -0600, Jan Beulich wrote:
>> >>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
>> > Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
>> > on public header target"):
>> >> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
>> >> > 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.
>> >> 
>> >> IIRC at one point I used '-' in mini-os build system but some version of
>> >> make didn't like it. So I stick with '_' since.
>> > 
>> > I think you are confused.  You are probably thinking of variable names
>> > which cannot contain -.  (Well, which are troublesome if they do.)
>> 
>> Troublesome? I'm pretty sure we use such somewhere in the
>> hypervisor tree...
> 
> Then we'd better fix it sooner rather than later. :p

I'm afraid I would likely nak any such attempt.

> I dig out the patch. And this is in the commit message:
> 
>     In the GNU make manual "How to Use Variables" section there is such
>     word:
> 
>     "However, variable names containing characters other than letters,
>     numbers, and underscores should be considered carefully, as in some
>     shells they cannot be passed through the environment to a sub-make (see
>     Communicating Variables to a Sub-make)."

Which is fine. I'd never use such names for exported variables.
But for internally used ones they're quite fine (and easier to
type than ones using underscores).

As a side note - even ./Config.mk has such, so all subtrees
effectively use them one way or another.

Jan


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

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

[Jan Beulich]

>>> On 17.05.17 at 17:05, <wei.liu2@citrix.com> wrote:
> On Wed, May 17, 2017 at 03:51:11PM +0100, Wei Liu wrote:
>> On Wed, May 17, 2017 at 08:28:40AM -0600, Jan Beulich wrote:
>> > >>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
>> > > Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
> 
>> > > on public header target"):
>> > >> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
>> > >> > 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.
>> > >> 
>> > >> IIRC at one point I used '-' in mini-os build system but some version of
>> > >> make didn't like it. So I stick with '_' since.
>> > > 
>> > > I think you are confused.  You are probably thinking of variable names
>> > > which cannot contain -.  (Well, which are troublesome if they do.)
>> > 
>> > Troublesome? I'm pretty sure we use such somewhere in the
>> > hypervisor tree...
>> 
>> Then we'd better fix it sooner rather than later. :p
>> 
>> I dig out the patch. And this is in the commit message:
>> 
>>     In the GNU make manual "How to Use Variables" section there is such
>>     word:
>> 
>>     "However, variable names containing characters other than letters,
>>     numbers, and underscores should be considered carefully, as in some
>>     shells they cannot be passed through the environment to a sub-make (see
>>     Communicating Variables to a Sub-make)."
>> 
> 
> $ cd xen.git/xen
> $ git-ls-files | grep '\(Rules.*\|Makefile.*\|[Cc]onfig.*\)' | xargs \
>    grep -nH '[A-Z_]+-.*' # yields nothing

It must have overlooked thinks like

subdir-y += acpi
subdir-y += cpu
subdir-y += genapic

Ah, indeed, you did look for uppercase names only.

Jan


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

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

[Wei Liu]

On Wed, May 17, 2017 at 09:14:17AM -0600, Jan Beulich wrote:
> >>> On 17.05.17 at 17:05, <wei.liu2@citrix.com> wrote:
> > On Wed, May 17, 2017 at 03:51:11PM +0100, Wei Liu wrote:
> >> On Wed, May 17, 2017 at 08:28:40AM -0600, Jan Beulich wrote:
> >> > >>> On 17.05.17 at 16:20, <ian.jackson@eu.citrix.com> wrote:
> >> > > Wei Liu writes ("Re: [PATCH for-4.9] build: stubdom and tools should depend 
> > 
> >> > > on public header target"):
> >> > >> On Wed, May 17, 2017 at 02:16:39PM +0100, Ian Jackson wrote:
> >> > >> > 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.
> >> > >> 
> >> > >> IIRC at one point I used '-' in mini-os build system but some version of
> >> > >> make didn't like it. So I stick with '_' since.
> >> > > 
> >> > > I think you are confused.  You are probably thinking of variable names
> >> > > which cannot contain -.  (Well, which are troublesome if they do.)
> >> > 
> >> > Troublesome? I'm pretty sure we use such somewhere in the
> >> > hypervisor tree...
> >> 
> >> Then we'd better fix it sooner rather than later. :p
> >> 
> >> I dig out the patch. And this is in the commit message:
> >> 
> >>     In the GNU make manual "How to Use Variables" section there is such
> >>     word:
> >> 
> >>     "However, variable names containing characters other than letters,
> >>     numbers, and underscores should be considered carefully, as in some
> >>     shells they cannot be passed through the environment to a sub-make (see
> >>     Communicating Variables to a Sub-make)."
> >> 
> > 
> > $ cd xen.git/xen
> > $ git-ls-files | grep '\(Rules.*\|Makefile.*\|[Cc]onfig.*\)' | xargs \
> >    grep -nH '[A-Z_]+-.*' # yields nothing
> 
> It must have overlooked thinks like
> 
> subdir-y += acpi
> subdir-y += cpu
> subdir-y += genapic
> 
> Ah, indeed, you did look for uppercase names only.
> 

Correct. I only grepped for the ones that are uppercase because those
are likely to be exported to sub-make.

The local variables are fine (subdir-y and co.). I have no objection to
those.

Wei.

> Jan
> 

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