| Message ID | 20170517130442.16393-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
>>> 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
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
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.
>>> 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
>>> 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
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 >
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
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(-)