Re: [PATCH v2] selftests: kvm: fix mkdir error when building for unsupported arch

[Muhammad Usama Anjum <Usama.Anjum@collabora.com>]

On 8/19/24 9:33 PM, Sean Christopherson wrote:
> +KVM arch maintainers
> 
> On Mon, Aug 19, 2024, Muhammad Usama Anjum wrote:
>> The tests are built on per architecture basis. When unsupported
>> architecture is specified, it has no tests and TEST_GEN_PROGS is empty.
>> The lib.mk has support for not building anything for such case. But KVM
>> makefile doesn't handle such case correctly. It doesn't check if
>> TEST_GEN_PROGS is empty or not and try to create directory by mkdir.
>> Hence mkdir generates the error.
>>
>> mkdir: missing operand
>> Try 'mkdir --help' for more information.
>>
>> This can be easily fixed by checking if TEST_GEN_PROGS isn't empty
>> before calling mkdir.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>> Changes since v1:
>> - Instead of ignoring error, check TEST_GEN_PROGS's validity first
>> ---
>>  tools/testing/selftests/kvm/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index 48d32c5aa3eb7..9f8ed82ff1d65 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -317,7 +317,9 @@ $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S $(GEN_HDRS)
>>  $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c
>>  	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@
>>  
>> +ifneq ($(strip $(TEST_GEN_PROGS)),)
>>  $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
>> +endif
> This just suppresses an error, it doesn't fix the underlying problem.  E.g. there
> are other weird side effects, such as an above mkdir creating the $(ARCH) directory
> even though it shouldn't exist in the end.
> 
> It's also very opaque, e.g. without a comment or the context of the changelog,
> I'd have no idea what purpose the above serves.
> 
> Rather than bury the effective "is this arch supported" check in the middle of
> the Makefile, what if we wrap the "real" makefile and include it only for
> supported architectures, and provide dummy targets for everything else?
> 
> E.g.
> 
> ---
> # SPDX-License-Identifier: GPL-2.0-only
> top_srcdir = ../../../..
> include $(top_srcdir)/scripts/subarch.include
> ARCH            ?= $(SUBARCH)
> 
> ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64))
> ifeq ($(ARCH),x86)
>         ARCH_DIR := x86_64
> else ifeq ($(ARCH),arm64)
>         ARCH_DIR := aarch64
> else ifeq ($(ARCH),s390)
>         ARCH_DIR := s390x
> else
>         ARCH_DIR := $(ARCH)
> endif
> 
> include Makefile.kvm
> else
> all:
> clean:
> endif
> ---
> 
> And other KVM maintainers, the big question is: if we do the above, would now be
> a decent time to bite the bullet and switch to the kernel's canonical arch paths,
> i.e. arm64, s390, and x86?  I feel like if we're ever going to get away from
> using aarch64, x86_64, and s390x, this is as about a good of an opportunity as
> we're going to get.
> 
> The annoying x86_64=>x86 alias still needs to be handled to avoid breaking explicit
> ARCH=x86_64 builds (which apparently are allowed, *sigh*), but we can ditch ARCH_DIR
> and the KVM selftests dirs match tools' include paths.
> 
> ---
> # SPDX-License-Identifier: GPL-2.0-only
> top_srcdir = ../../../..
> include $(top_srcdir)/scripts/subarch.include
> ARCH            ?= $(SUBARCH)
> 
> ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64))
> # Top-level selftests allows ARCH=x86_64 🙁
> ifeq ($(ARCH),x86_64)
> 	ARCH := x86
> endif
> include Makefile.kvm
> else
> all:
> clean:
> endif
> ---
> 
> If no one objects or has a better idea, I'll post a series to do the above.
I didn't had enough knowledge to attempt a better fix. Thank you.

-- 
BR,
Muhammad Usama Anjum