From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Wed, 10 Feb 2016 09:49:51 -0700 Subject: [U-Boot] [PATCH] Makefile: remove BUILD_TAG from KBUILD_CFLAGS In-Reply-To: References: <1454981534-15925-1-git-send-email-swarren@wwwdotorg.org> <56BA0FA3.6070503@gmail.com> <20160209170130.GL11375@bill-the-cat> <56BA2136.4070306@wwwdotorg.org> <20160209174342.GQ11375@bill-the-cat> <56BA27AC.4070500@wwwdotorg.org> <20160209181849.GS11375@bill-the-cat> Message-ID: <56BB6A2F.9020600@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/09/2016 10:58 PM, Masahiro Yamada wrote: > 2016-02-10 3:18 GMT+09:00 Tom Rini : >> 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 >>>>>>>> >>>>>>>> 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.