Re: [XEN PATCH v2] build: Fix x86 out-of-tree build without EFI

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.08.2022 16:12, Anthony PERARD wrote:
> On Wed, Aug 17, 2022 at 12:38:36PM +0200, Jan Beulich wrote:
>> On 17.08.2022 11:15, Anthony PERARD wrote:
>>> --- a/xen/common/efi/efi-common.mk
>>> +++ b/xen/common/efi/efi-common.mk
>>> @@ -9,9 +9,9 @@ CFLAGS-y += -iquote $(srcdir)
>>>  # e.g.: It transforms "dir/foo/bar" into successively
>>>  #       "dir foo bar", ".. .. ..", "../../.."
>>>  $(obj)/%.c: $(srctree)/common/efi/%.c FORCE
>>> -   $(Q)test -f $@ || \
>>> -       ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, 
>>> ,$(obj))))/source/common/efi/$(<F) $@
>>> +   $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, 
>>> ,$(obj))))/source/common/efi/$(<F) $@
>>
>> I'm afraid the commit message hasn't made clear to me why this part of
>> the change is (still) needed (or perhaps just wanted). The rest of this
>> lgtm now, thanks.
> 
> There's an explanation in the commit message, quoted here:
>>>  The issue is that in out-of-tree, make might find x86/efi/stub.c via
>>>  VPATH, but as the target needs to be rebuilt due to FORCE, make
>>>  actually avoid changing the source tree and rebuilt the target with
>>>  VPATH ignored, so $@ lead to the build tree where "stub.c" dosen't
>>>  exist yet so a link is made to "common/stub.c".

Hmm, yes, I had guessed that this might be it, but I wasn't able to make
the connection, sorry.

> The problem with `test -f $@` it doesn't test what we think it does. It
> doesn't test for the presence of a regular file in the source tree as
> stated in the original tree.

I didn't think it would to that. $@ is the target of the rule, and the
(pattern) target explicitly points into the build tree, by way of using
$(obj).

> First, `test -f` happily follow symlinks.

Which is of no relevance here, afaict.

> Second, $@ is always going to point to the build dir, as GNU Make will
> try to not make changes to the source tree, if I understand the logic
> correctly.
> 
> Instead of `test -f`, we could probably remove the "FORCE" from the
> prerequisite, but there's still going to be an issue if there's a file
> with the same name in both common and per-arch directory, when the common
> file is newer.

This would be a mistake now, wouldn't it? I did add "(still)" in my earlier
reply for the very reason that it looks to me as if this change might have
been an attempt to address the issue without any renaming.

> So `test -f` needs to go.

I'm sorry to conclude that for now I continue to only see that its removal
does no harm (hence the "(or perhaps just wanted)" in my original reply),
but I still don't see that it's strictly needed. Therefore I'm okay with
the change as is, but I don't view the description as quite clear enough
in this one regard.

Jan