I don't view a variable of this name as suitable for exporting, the more
that it carries entirely redundant information. The reasons for its
introduction in Linux commit 051f278e9d81 ("kbuild: replace
KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply
to us. Ditch exporting of the variable, replacing uses suitably.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
---
For further reasons (besides the similar redundancy aspect) exporting
VPATH looks also suspicious: Its name being all uppercase makes it a
"non application private" variable, i.e. it or its (pre-existing) value
may have a purpose/use elsewhere. And exporting it looks to be easily
avoidable: Instead of setting it in xen/Makefile, it looks like it could
be set in xen/scripts/Kbuild.include. Thoughts?

--- a/xen/Makefile
+++ b/xen/Makefile
@@ -208,7 +208,7 @@ endif
 objtree := .
 VPATH := $(srctree)
 
-export building_out_of_srctree srctree objtree VPATH
+export srctree objtree VPATH
 
 export XEN_ROOT := $(abs_srctree)/..
 
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -14,7 +14,7 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin)
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
 CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
-ifdef building_out_of_srctree
+ifneq ($(abs_objtree),$(abs_srctree))
 CFLAGS_x86_32 += -I$(objtree)/include
 endif
 CFLAGS_x86_32 += -I$(srctree)/include
--- a/xen/scripts/Makefile.host
+++ b/xen/scripts/Makefile.host
@@ -88,7 +88,7 @@ _hostcxx_flags = $(HOSTCXXFLAGS) $(HOST_
                  $(HOSTCXXFLAGS_$(target-stem).o)
 
 # $(objtree)/$(obj) for including generated headers from checkin source files
-ifdef building_out_of_srctree
+ifneq ($(abs_objtree),$(abs_srctree))
 _hostc_flags   += -I $(objtree)/$(obj)
 _hostcxx_flags += -I $(objtree)/$(obj)
 endif

On Wed, Mar 29, 2023 at 12:22:16PM +0200, Jan Beulich wrote:
> I don't view a variable of this name as suitable for exporting, the more

We could rename it.

> that it carries entirely redundant information. The reasons for its

The patch replace building_out_of_srctree with abs_objtree and
abs_srctree which also carries redundant informations. abs_objtree can
probably be replaced by $(CURDIR), abs_srctree can be
recalculated from $(srctree).

> introduction in Linux commit 051f278e9d81 ("kbuild: replace
> KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply
> to us. Ditch exporting of the variable, replacing uses suitably.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This patch feels like obfuscation of the intended test. Instead of
reading a test for "out_of_tree", we now have to guess why the two paths
are been compared.

Anyway, there isn't that many use outside of the main Makefile, so I
guess the patch is kind of ok:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>


> For further reasons (besides the similar redundancy aspect) exporting
> VPATH looks also suspicious: Its name being all uppercase makes it a
> "non application private" variable, i.e. it or its (pre-existing) value
> may have a purpose/use elsewhere. And exporting it looks to be easily

This sounds like you don't know what VPATH is for, but I'm pretty sure
you do. If there's an pre-existing value, we just ignore it. If VPATH is
used by a program that our Makefile used and that program is intended to
be used by build system then that a bug in that program for not knowing
about makes' VPATH. So I don't think we need to worried about it been
exported.

> avoidable: Instead of setting it in xen/Makefile, it looks like it could
> be set in xen/scripts/Kbuild.include. Thoughts?

I'd rather not make that change unless there's a real issue with
exporting VPATH. We are more likely to introduce a bug than to avoid
one.

Thanks,

-- 
Anthony PERARD

On 04.05.2023 16:49, Anthony PERARD wrote:
> On Wed, Mar 29, 2023 at 12:22:16PM +0200, Jan Beulich wrote:
>> I don't view a variable of this name as suitable for exporting, the more
> 
> We could rename it.
> 
>> that it carries entirely redundant information. The reasons for its
> 
> The patch replace building_out_of_srctree with abs_objtree and
> abs_srctree which also carries redundant informations. abs_objtree can
> probably be replaced by $(CURDIR), abs_srctree can be
> recalculated from $(srctree).
> 
>> introduction in Linux commit 051f278e9d81 ("kbuild: replace
>> KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply
>> to us. Ditch exporting of the variable, replacing uses suitably.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This patch feels like obfuscation of the intended test. Instead of
> reading a test for "out_of_tree", we now have to guess why the two paths
> are been compared.

Hmm, it's quite the other way around for me: I view the variable as
obfuscating, as it hides what it actually expresses (or better based
on what properties it is actually set).

> Anyway, there isn't that many use outside of the main Makefile, so I
> guess the patch is kind of ok:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks.

>> For further reasons (besides the similar redundancy aspect) exporting
>> VPATH looks also suspicious: Its name being all uppercase makes it a
>> "non application private" variable, i.e. it or its (pre-existing) value
>> may have a purpose/use elsewhere. And exporting it looks to be easily
> 
> This sounds like you don't know what VPATH is for, but I'm pretty sure
> you do. If there's an pre-existing value, we just ignore it. If VPATH is
> used by a program that our Makefile used and that program is intended to
> be used by build system then that a bug in that program for not knowing
> about makes' VPATH. So I don't think we need to worried about it been
> exported.

We may use programs from our build system which aren't aware they might
be used that way. No matter that I know what VPATH is for, I consider
its name to violate the shell spec.

>> avoidable: Instead of setting it in xen/Makefile, it looks like it could
>> be set in xen/scripts/Kbuild.include. Thoughts?
> 
> I'd rather not make that change unless there's a real issue with
> exporting VPATH. We are more likely to introduce a bug than to avoid
> one.

Well, okay, it's surely (hopefully) a highly theoretical consideration
anyway.

Jan