Re: [PATCH v2 1/2] build: don't export building_out_of_srctree

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> 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@xxxxxxxxxx>

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