On Wed, Jun 07, 2023 at 02:33:14PM +0800, Zhangjin Wu wrote:
ifneq ($(findstring riscv,$(ARCH)),) KARCH = riscv else KARCH = $(ARCH) endif(...)
At least it suggests what it's going to be used for instead of just being marked as "special" (something the underscore does).
Yeah.
but the new method mentioned here differs, it split the whole Makefile to two 'parts', the before part accept something like ARCH=riscv32, ARCH=riscv64, ARCH=riscv, the after part use the ARCH=riscv, this avoid touch the targets context:
We don't care about touching *code*. What is important is that it scales and is understandable, thus maintainable. Code that has many exceptions or requires a lot of head scratching to figure what's being done is a pain to maintain and nobody wants to take the risk to touch it. That was exactly the purpose of the enumeration of per-target args, flags etc in the makefile: nobody needs to be expert in multiple areas to touch their own area. If we face a showstopper, we need to address it, and not work around it for the sake of touching less context.
Get it clearly.
... variable assignments before this line ... +# Some architectures share the same arch/<ARCH>/ source code tree among the <ARCH>xyz variants +# Top-level kernel Makefile only accepts ARCH=<ARCH>, override <ARCH>xyz variants to make kernel happy +ARCHS := riscv +_ARCH := $(strip $(foreach arch, $(ARCHS), $(if $(findstring x$(arch),x$(ARCH)),$(arch)))) +ifneq ($(_ARCH),) +override ARCH := $(_ARCH) +endif +So actually this is a perfect example of head scratching for me. I suspect it would replace x86_64 with x86 if x86 would be placed there for example, though it would not change anything for i386. Maybe for s390/s390x, arm/arm64 or ppc/ppc64 etc it would act similarly while these are different archs. Thus this seems to be trying to generalize a rule around a single particular case.
It is true, we did worry about when users wrongly add new ARCH in the list, a generic way is very hard before we really use them.
Probably that instead this particular case should be addressed explicitly until we find a generalization (if ever) to other archs:
ifeq($(ARCH),riscv32) override ARCH := riscv else ifeq($(ARCH),riscv64) override ARCH := riscv endif endif
Or maybe even better you can decide to remap arch names explicitly:
# use KARCH_from=to to rename ARCH from $from to $to past this point. KARCH_riscv32 := riscv KARCH_riscv64 := riscv ... ifneq($(KARCH_$(ARCH)),) override ARCH := $(KARCH_$(ARCH)) endif
This did inspire me a lot, so, what about simply go back to the KARCH method without any overriding:
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 4a3a105e1fdf..bde635b083f4 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -14,6 +14,12 @@ include $(srctree)/scripts/subarch.include ARCH = $(SUBARCH) endif
+# kernel supported ARCH names by architecture +KARCH_riscv32 = riscv +KARCH_riscv64 = riscv +KARCH_riscv = riscv +KARCH = $(or $(KARCH_$(ARCH)),$(ARCH)) + # kernel image names by architecture IMAGE_i386 = arch/x86/boot/bzImage IMAGE_x86_64 = arch/x86/boot/bzImage @@ -21,6 +27,8 @@ IMAGE_x86 = arch/x86/boot/bzImage IMAGE_arm64 = arch/arm64/boot/Image IMAGE_arm = arch/arm/boot/zImage IMAGE_mips = vmlinuz
And this:
@@ -117,7 +132,7 @@ sysroot: sysroot/$(ARCH)/include sysroot/$(ARCH)/include: $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot $(QUIET_MKDIR)mkdir -p sysroot - $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone + $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(KARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone $(Q)mv sysroot/sysroot sysroot/$(ARCH)
nolibc-test: nolibc-test.c sysroot/$(ARCH)/include @@ -141,10 +156,10 @@ initramfs: nolibc-test $(Q)cp nolibc-test initramfs/init
defconfig: - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare + $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
kernel: initramfs - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs + $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
It is almost consistent with the original Makefile now.
I do like this method more than the override method now, the override method may break the maintainability a lot especially that the developers may be hard to know which ARCH value it is when he touch a line of the Makefile.
And this does deserve an explicit note in the makefile that anything using $(ARCH) using a macro will see the renamed arch while anything using it as a variable before that line will see the original one.
If you want to avoid the '=' vs ':=' mess you can even keep a copy of the original ARCH at the beginning of the makefile:
# keep a copy of the arch name requested by the user, for use later # when the original form is preferred over the kernel's arch name. USER_ARCH = $(ARCH)
Yeah, a copy is good for the override case.
Willy, Which one do you prefer?
The most explicit ones like above. Generally speaking when you try to add support for your own arch here, you look there for similar ones, where commands are called, and read in reverse mode till the beginning, hoping to understand the transformations. I think the current ones and the proposed ones above are self-explanatory. Anything doing too much magic renaming or doing too much hard-coded automatic stuff can quickly obfuscate the principle and make things more complicated. I already despise "override" because it messes up with macros, but I agree it can sometimes have some value. If you dup it into ORIG_ARCH or USER_ARCH, and modify the few lines overriding arch in an explicit manner, I think it would preserve its maintainability.
Agree, let's give up the 'override' stuff.
What do you think ?
So, let's go with the KARCH method if you agree too.
Best regards, Zhangjin
thanks, Willy