| Message ID | CAK7LNATPN1HyVKR4No26xi7+y_xGgoeU=93-M5Rch9w3GVOhww@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/09/2016 10:58 PM, Masahiro Yamada wrote: > 2016-02-10 3:18 GMT+09:00 Tom Rini <trini@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@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.
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