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

[Masahiro Yamada]

Hi.



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/Makefile b/Makefile
index a46c1ae..77cb8fa 100644
--- a/Makefile
+++ b/Makefile
@@ -562,10 +562,6 @@ else
 KBUILD_CFLAGS  += -O2
 endif

-ifdef BUILD_TAG
-KBUILD_CFLAGS += -DBUILD_TAG='"$(BUILD_TAG)"'
-endif
-
 KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
 KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks)

diff --git a/lib/Makefile b/lib/Makefile
index dd36f25..9325faa 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -67,6 +67,9 @@ obj-$(CONFIG_ADDR_MAP) += addr_map.o
 obj-y += hashtable.o
 obj-y += errno.o
 obj-y += display_options.o
+
+CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')
+
 obj-$(CONFIG_BCH) += bch.o
 obj-y += crc32.o
 obj-y += ctype.o





-- 
Best Regards
Masahiro Yamada