[U-Boot] [PATCH] Makefile: remove BUILD_TAG from KBUILD_CFLAGS

[Stephen Warren]

On 02/09/2016 10:58 PM, Masahiro Yamada wrote:
> 2016-02-10 3:18 GMT+09:00 Tom Rini <trini at konsulko.com>:
>> On Tue, Feb 09, 2016 at 10:53:48AM -0700, Stephen Warren wrote:
>>> On 02/09/2016 10:43 AM, Tom Rini wrote:
>>>> On Tue, Feb 09, 2016 at 10:26:14AM -0700, Stephen Warren wrote:
>>>>> On 02/09/2016 10:01 AM, Tom Rini wrote:
>>>>>> On Tue, Feb 09, 2016 at 08:11:15AM -0800, James Chargin wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/08/2016 05:32 PM, Stephen Warren wrote:
>>>>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>>>>
>>>>>>>> If BUILD_TAG is part of KBUILD_CFLAGS, then any time the value changes,
>>>>>>>> all files get rebuilt. In a continuous integration environment, the value
>>>>>>>> will change every build. This wastes time assuming that incremental
>>>>>>>> builds would otherwise occur.
>>>>>>>>
>>>>>>>> To solve this, remove BUILD_TAG from KBUILD_FLAGS and add it to the end of
>>>>>>>> "local version".
>>>>>>>>
>>>>>>>> This has other advantages too:
>>>>>>>> - The special case for BUILD_TAG in display_options.c can be removed.
>>>>>>>> - The version printed by the "version" command exactly matches what is
>>>>>>>>    printed at boot.
>>>>>>>>
>>>>>>>> Old sign-on message:
>>>>>>>> U-Boot 2016.03-rc1-00044-g4085db5e767b (Feb ...), Build: bar-bas
>>>>>>>>
>>>>>>>> New sign-on message:
>>>>>>>> U-Boot 2016.03-rc1-00044-g4085db5e767b-bar-baz (Feb ...)
>>>>>>>
>>>>>>> I would urge this not be done. The display of the BUILD_TAG on
>>>>>>> startup is pretty useful in my environment. It's been there for a
>>>>>>> long time and some of my users have grown used to it.
>>>>>>>
>>>>>>> Of all the parts of the sign-on message, I'd rather the git hash go
>>>>>>> away than the BUILD_TAG. None of my users really care about the
>>>>>>> level of detail of the git hash and won't spend the time required to
>>>>>>> use this hash to determine if they have the version they want. (Some
>>>>>>> don't have a repo clone, and don't care to, and so can't easily make
>>>>>>> the correspondence even if they wanted to).
>>>>>>
>>>>>> Yeah, I think this is too widely used of a thing to change.  FWIW, I
>>>>>> really like githashes since it means you can see if $random-binary is
>>>>>> something you have in $vendor-tree or not.  So I think in this case, NAK
>>>>>> on the patch and maybe need to poke Travis-CI on how to or not to tag
>>>>>> things?
>>>>>
>>>>> Do you mean you think people currently rely upon the ", Build: xxx"
>>>>> format in the sign-on message, and the "version" command /not/
>>>>> printing that information?
>>>>
>>>> I mean that the places we construct strings like this are likely relied
>>>> upon by people, yes.
>>>>
>>>>> If so, I'll go back to trying to work out how to get Kbuild to
>>>>> transfer the environment variable into a CONFIG_XXX option so that
>>>>> modifying it only causes a rebuild of the one file that uses the
>>>>> value.
>>>>
>>>> A quick peak and I think that:
>>>> (a) LOCALVERSION needs a little love somewhere to ensure that it in fact
>>>> does not exceed 64 characters (and pushing upstream to the kernel
>>>> anything relevant here)
>>>> (b) Yes, adjusting BUILD_TAG into CONFIG_BUILD_TAG is probably the best
>>>> answer and probably not even that bad.  I don't think we need an
>>>> automagic conversion, just noting it in the release emails and the
>>>> sudden lack of Build: should get people that are using it to update
>>>> their scripts.
>>>
>>> Unfortunately, simply renaming the variable won't work;
>>> Kconfig/Kbuild need to know something about it in order to do their
>>> rebuild-only-on-change magic (and the value must be removed from
>>> CFLAGS too). I was having a hard time getting that to work, but I'll
>>> take another look.
>>
>> Right.  Can't we just make it another string Kconfig like LOCALVERSION,
>> not need to pass -D.... and then it's set via poking the config file by
>> CI, etc?
>
> We are discussing two things together.
>
> [1] avoid full build
> [2] change of the version string format and build-flow
>
> Nobody would complain about fixing [1] only.
> (Feel free to continue discussion if you want [2].)
>
> As Stephen pointed out, KBUILD_CFLAGS is passed to all the C files.
>
> The environment "BUILD_TAG" is only referenced from lib/display_option.c,
> so how about passing the flag to the file only?
>
> Try the following patch:
>
> diff --git a/lib/Makefile b/lib/Makefile

> +CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')

That's a nice simple idea, and should work well. I'll go validate it.

The only disadvantage is that if someone wants to start using BUILD_TAG 
elsewhere, they'd have to manually add it to CFLAGS for that file too. 
If we could import the environment variable into Kconfig automatically 
somehow as CONFIG_BUILD_TAG, that would not be needed; using 
CONFIG_BUILD_TAG would "just work". However, if you think we can live 
with that, I'm all for it.