[Buildroot] [PATCH 1/1] boot/opensbi: introduce BR2_TARGET_OPENSBI_CUSTOM_MAKEOPTS

[Arnout Vandecappelle]

On 13/05/2022 11:42, Noah Hütter wrote:
> 
> 
> On 5/12/22 23:57, Arnout Vandecappelle wrote:
>>
>>
>> On 12/05/2022 08:50, Noah Hütter wrote:

[snip]
>>>>> +config BR2_TARGET_OPENSBI_CUSTOM_MAKEOPTS
>>>>> +       string "Custom make options"
>>>>> +       help
>>>>> +         List of custom make options passed at build time. Can be
>>>>> +         used for example to pass a BUILD_INFO= value.
>>
>>   In this case, it's not trivial because the use case is not clear. Almost 
>> every package has some random make, cmake, meson or whatever options that you 
>> could pass to it. What makes OpenSBI so special that you need to pass a 
>> freeform list of additional options?
> 
> In my specific case as described in the help, the value to `BUILD_INFO`. 

  Yes, but why is it absolutely necessary to set BUILD_INFO? Are there other 
variables that are going to be need to be set, or is it just the one? In the 
latter case, it would be better to have BR2_TARGET_OPENSBI_BUILD_INFO.


> From 
> looking at other packages I deduced that the _CUSTOM_MAKEOPTS is a common config 
> to packages and added it to OpenSBI. Did I overlook something?

  There's just one that has this, uboot. It was added by this commit:

commit cc151c3993090a52d1fef8532f52d74ee6d924c9
Author: Thomas Petazzoni <thomas.petazzoni at bootlin.com>
Date:   Fri Apr 26 17:19:46 2019

     boot/uboot: add option to pass custom variables to U-Boot build

     U-Boot supports a number of environment variables to pass specific
     information. The following patches were submitted in the past to one
     some specific Config.in option to pass some of these variables:

      - http://patchwork.ozlabs.org/patch/881197/ proposed an option to
        pass a custom EXT_DTB= variable

      - http://patchwork.ozlabs.org/patch/1018245/ proposed an option to
        pass a custom DEVICE_TREE= variable

     Instead of adding one Config.in option for each of those variables,
     let's provide a generic mechanism to pass arbitrary variables during
     U-Boot build step.

  I have to say that that commit message isn't really good enough either, it 
should really have copied the explanations in the two referred patches as to why 
those two variables would need to be used. What we're really looking for is:

- A "proof" that this is for a relevant use case and not just some corner case 
that nobody other than yourself is ever going to need.

- A "design document" that this is really the way to implement this feature, 
that other alternatives are less appropriate (cfr. the "Instead of ... let's 
..." sentence above).

- And all this documented in the commit message, so a few years down the line if 
someone changes it, we remember the reasons why we made those choices.

  Note that it doesn't have to be very wordy, it's possible to cover those bases 
in just a few words. But the gist of it needs to be there.

  Regards,
  Arnout




> 
> Best,
> Noah
> 
>>>>> +
>>>>>   endif
>>>>> diff --git a/boot/opensbi/opensbi.mk b/boot/opensbi/opensbi.mk
>>>>> index 8e055633a8..d007ae1299 100644
>>>>> --- a/boot/opensbi/opensbi.mk
>>>>> +++ b/boot/opensbi/opensbi.mk
>>>>> @@ -31,7 +31,8 @@ BR_NO_CHECK_HASH_FOR += $(OPENSBI_SOURCE)
>>>>>   endif
>>>>>
>>>>>   OPENSBI_MAKE_ENV = \
>>>>> -       CROSS_COMPILE=$(TARGET_CROSS)
>>>>> +       CROSS_COMPILE=$(TARGET_CROSS) \
>>>>> +       $(call qstrip,$(BR2_TARGET_OPENSBI_CUSTOM_MAKEOPTS))
>>>>>
>>>>>   OPENSBI_PLAT = $(call qstrip,$(BR2_TARGET_OPENSBI_PLAT))
>>>>>   ifneq ($(OPENSBI_PLAT),)
>>>>> -- 
>>>>> 2.35.1
>>>>>
>>>>> _______________________________________________
>>>>> buildroot mailing list
>>>>> buildroot at buildroot.org
>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
>>>
>>>
>>> _______________________________________________
>>> buildroot mailing list
>>> buildroot at buildroot.org
>>> https://lists.buildroot.org/mailman/listinfo/buildroot