Re: [Buildroot] [PATCH v2 1/6] boot/grub2: add support to build multiple Grub2 configurations in the same build

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Arnout, All,

On 2021-09-21 19:20 +0200, Arnout Vandecappelle spake thusly:
> On 21/09/2021 18:37, Yann E. MORIN wrote:
> >On 2021-09-21 15:28 +0200, Kory Maincent spake thusly:
> [snip]
> >>  ifeq ($(BR2_TARGET_GRUB2_I386_PC),y)
> >[--SNIP removals--]
> >>+GRUB2_IMAGE_i386-pc = $(BINARIES_DIR)/grub.img
> >>+GRUB2_CFG_i386-pc = $(TARGET_DIR)/boot/grub/grub.cfg
> >>+GRUB2_PREFIX_i386-pc = ($(GRUB2_BOOT_PARTITION))/boot/grub
> >>+GRUB2_TARGET_i386-pc = i386
> >>+GRUB2_PLATFORM_i386-pc = pc
> >>+GRUB2_BUILTIN_i386-pc = PC
> >>+GRUB2_TUPLES += i386-pc
> >>+endif
> >
> >Any reason why you have decided not to got with the construct I
> >suggested:
> >     GRUB2_TUPLES-$(BR2_TARGET_GRUB2_I386_PC) += i386-pc
> >
> >and then use $(GRUB2_TUPLES-y) when iterating?
> 
>  Well, it's a bigger diff compared to what you have now. Especially because
> you have to keep the GRUB2_IMAGE_i386-pc etc. part, it's only TUPLES that
> you can use like that.
> 
>  Also, I think the -y approach is good only when you have may options
> (because it is slightly more difficult to parse) - we have 5 options here,
> which is a bit borderline.
> 
>  However, this approach becomes a lot more attractive if we eliminate the
> conditions:

Of course! I should have stated that explicitly, but in my mind it was
obvious that the conditions would disapear, and that'd we'd keep only
the -y assignments.

> GRUB2_IMAGE_i386-pc = $(BINARIES_DIR)/grub.img
> GRUB2_CFG_i386-pc = $(TARGET_DIR)/boot/grub/grub.cfg
> GRUB2_PREFIX_i386-pc = ($(GRUB2_BOOT_PARTITION))/boot/grub
> GRUB2_TARGET_i386-pc = i386
> GRUB2_PLATFORM_i386-pc = pc
> GRUB2_BUILTIN_i386-pc = PC
> GRUB2_TUPLES-$(BR2_TARGET_GRUB2_I386_PC) += i386-pc
> 
> GRUB2_IMAGE_i386-efi = $(BINARIES_DIR)/efi-part/EFI/BOOT/bootia32.efi
> GRUB2_CFG_i386-efi = $(BINARIES_DIR)/efi-part/EFI/BOOT/grub.cfg
> GRUB2_PREFIX_i386-efi = /EFI/BOOT
> GRUB2_TARGET_i386-efi = i386
> GRUB2_PLATFORM_i386-efi = efi
> GRUB2_BUILTIN_i386-efi = EFI
> GRUB2_TUPLES-$(BR2_TARGET_GRUB2_I386_EFI) += i386-efi

Exactly!

> >Note: this is used everywhere in the kernel tree, and we already use
> >that construct in quite a few places, so it would not be totally new
> >and not totally unkown either:
> >
> >     $ git grep -E -- '[_-]\$\([^)]+\)[[:space:]]*\+=' '*.mk'
> >
> >But OK, the multi-conditions work as good if you don't like the -y
> >stuff.
> >
> >>+ifeq ($(BR2_TARGET_GRUB2_I386_EFI),y)
> >>+GRUB2_IMAGE_i386-efi = $(BINARIES_DIR)/efi-part/EFI/BOOT/bootia32.efi
> >>+GRUB2_CFG_i386-efi = $(BINARIES_DIR)/efi-part/EFI/BOOT/grub.cfg
> >>+GRUB2_PREFIX_i386-efi = /EFI/BOOT
> >>+GRUB2_TARGET_i386-efi = i386
> >>+GRUB2_PLATFORM_i386-efi = efi
> >>+GRUB2_BUILTIN_i386-efi = EFI
> >
> >All those GRUB2_BUILTIN_$(tuple) variables are just so that you can do a
> >double indeirection alter on, which is highly unreadable.
> >
> >What about assigning the options directly:
> >
> >     GRUB2_CONFIG_i386-pc = $(GRUB2_BUILTIN_CONFIG_PC)
> >     GRUB2_MODULES_i386-pc = $(GRUB2_BUILTIN_MODULES_PC)
> >     GRUB2_CONFIG_i386-efi = $(GRUB2_BUILTIN_CONFIG_EFI)
> >     GRUB2_MODULES_i386-efi = $(GRUB2_BUILTIN_MODULES_EFI)
> 
>  Still feels over-complicated, but I can't think of anything better. But
> it's definitely an improvement over the BUILTIN_$(tuple).

Agreed, it's not a panacea, but it's still more legible.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@lists.buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot