| Message ID | 702ce86cc56aec78ac12.1374650580@BEANTN0L019720 |
|---|---|
| State | Superseded |
| Headers | show |
On 24/07/13 09:23, Thomas De Schampheleire wrote: [snip] > diff --git a/linux/Config.in b/linux/Config.in > --- a/linux/Config.in > +++ b/linux/Config.in > @@ -52,6 +52,12 @@ config BR2_LINUX_KERNEL_CUSTOM_GIT > This option allows Buildroot to get the Linux kernel source > code from a Git repository. > > +config BR2_LINUX_KERNEL_CUSTOM_HG > + bool "Custom Mercurial repository" > + help > + This option allows Buildroot to get the Linux kernel source > + code from a Mercurial repository. > + > endchoice > > config BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE > @@ -62,24 +68,26 @@ config BR2_LINUX_KERNEL_CUSTOM_TARBALL_L > string "URL of custom kernel tarball" > depends on BR2_LINUX_KERNEL_CUSTOM_TARBALL > > -config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL > - string "URL of custom Git repository" > - depends on BR2_LINUX_KERNEL_CUSTOM_GIT > +if BR2_LINUX_KERNEL_CUSTOM_GIT || BR2_LINUX_KERNEL_CUSTOM_HG > > -config BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION > - string "Custom Git version" > - depends on BR2_LINUX_KERNEL_CUSTOM_GIT > +config BR2_LINUX_KERNEL_CUSTOM_REPO_URL > + string "URL of custom repository" Given that string options can't be propagated from their legacy values, and since the name of an option isn't really that important, I'd keep the _GIT_ names for the mercurial options as well. If we do rename the option symbol names, then I would make sure that it is propagated: default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" + add a comment to the .legacy file so these hacks can be removed eventually. Also, you have to change the symbol names in the defconfigs as well (there are already 10 uses of this symbol). Regards, Arnout > + > +config BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION > + string "Custom repository version" > help > - Git revision to use in the format used by git rev-parse, > + Revision to use in the typical format used by Git/Mercurial > E.G. a sha id, a tag, branch, .. > > +endif > + > config BR2_LINUX_KERNEL_VERSION > string > default "3.10.1" if BR2_LINUX_KERNEL_LATEST_VERSION > default BR2_DEFAULT_KERNEL_HEADERS if BR2_LINUX_KERNEL_SAME_AS_HEADERS > default BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE if BR2_LINUX_KERNEL_CUSTOM_VERSION > default "custom" if BR2_LINUX_KERNEL_CUSTOM_TARBALL > - default $BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION if BR2_LINUX_KERNEL_CUSTOM_GIT > + default $BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION if BR2_LINUX_KERNEL_CUSTOM_HG || BR2_LINUX_KERNEL_CUSTOM_GIT > > # > # Patch selection > diff --git a/linux/linux.mk b/linux/linux.mk > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -14,8 +14,11 @@ LINUX_TARBALL = $(call qstrip,$(BR2_LINU > LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) > LINUX_SOURCE = $(notdir $(LINUX_TARBALL)) > else ifeq ($(BR2_LINUX_KERNEL_CUSTOM_GIT),y) > -LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL)) > +LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_REPO_URL)) > LINUX_SITE_METHOD = git > +else ifeq ($(BR2_LINUX_KERNEL_CUSTOM_HG),y) > +LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_REPO_URL)) > +LINUX_SITE_METHOD = hg > else > LINUX_SOURCE = linux-$(LINUX_VERSION).tar.xz > # In X.Y.Z, get X and Y. We replace dots and dashes by spaces in order > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
Hi Arnout, On Mon, Aug 12, 2013 at 8:12 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 24/07/13 09:23, Thomas De Schampheleire wrote: > [snip] >> diff --git a/linux/Config.in b/linux/Config.in >> --- a/linux/Config.in >> +++ b/linux/Config.in >> @@ -52,6 +52,12 @@ config BR2_LINUX_KERNEL_CUSTOM_GIT >> This option allows Buildroot to get the Linux kernel source >> code from a Git repository. >> >> +config BR2_LINUX_KERNEL_CUSTOM_HG >> + bool "Custom Mercurial repository" >> + help >> + This option allows Buildroot to get the Linux kernel source >> + code from a Mercurial repository. >> + >> endchoice >> >> config BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE >> @@ -62,24 +68,26 @@ config BR2_LINUX_KERNEL_CUSTOM_TARBALL_L >> string "URL of custom kernel tarball" >> depends on BR2_LINUX_KERNEL_CUSTOM_TARBALL >> >> -config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL >> - string "URL of custom Git repository" >> - depends on BR2_LINUX_KERNEL_CUSTOM_GIT >> +if BR2_LINUX_KERNEL_CUSTOM_GIT || BR2_LINUX_KERNEL_CUSTOM_HG >> >> -config BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION >> - string "Custom Git version" >> - depends on BR2_LINUX_KERNEL_CUSTOM_GIT >> +config BR2_LINUX_KERNEL_CUSTOM_REPO_URL >> + string "URL of custom repository" > > Given that string options can't be propagated from their legacy values, > and since the name of an option isn't really that important, I'd keep > the _GIT_ names for the mercurial options as well. > > If we do rename the option symbol names, then I would make sure that it > is propagated: > > default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" > > + add a comment to the .legacy file so these hacks can be removed > eventually. > > Also, you have to change the symbol names in the defconfigs as well > (there are already 10 uses of this symbol). Thanks for your feedback on this series. Regarding the choice of keeping the _GIT_ option names as-is, or changing them with the appropriate legacy handling, I'd like to hear the opinion of others. I understand that the _GIT_ names make the legacy handling easier, but I also feel it's awkward to have _GIT_ in your config file when you're really using Mercurial. It seems like a hack to me. So my preference would be to stick to the renaming as proposed in this patch series, but with the additional default in linux/Config.in (and likewise for uboot) as you suggested, deprecating this default later. In this case I'll also change the defconfigs, as you correctly noted (thanks). Best regards, Thomas
On 12/08/13 12:54, Thomas De Schampheleire wrote: > Hi Arnout, > > On Mon, Aug 12, 2013 at 8:12 AM, Arnout Vandecappelle <arnout@mind.be> wrote: [snip] >> Given that string options can't be propagated from their legacy values, >> and since the name of an option isn't really that important, I'd keep >> the _GIT_ names for the mercurial options as well. >> >> If we do rename the option symbol names, then I would make sure that it >> is propagated: >> >> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >> >> + add a comment to the .legacy file so these hacks can be removed >> eventually. >> >> Also, you have to change the symbol names in the defconfigs as well >> (there are already 10 uses of this symbol). > > Thanks for your feedback on this series. > > Regarding the choice of keeping the _GIT_ option names as-is, or > changing them with the appropriate legacy handling, I'd like to hear > the opinion of others. > > I understand that the _GIT_ names make the legacy handling easier, but > I also feel it's awkward to have _GIT_ in your config file when you're > really using Mercurial. It seems like a hack to me. So my preference > would be to stick to the renaming as proposed in this patch series, > but with the additional default in linux/Config.in (and likewise for > uboot) as you suggested, deprecating this default later. In this case > I'll also change the defconfigs, as you correctly noted (thanks). Sounds OK to me. Regards, Arnout
Dear Arnout Vandecappelle, On Mon, 12 Aug 2013 08:12:13 +0200, Arnout Vandecappelle wrote: > Given that string options can't be propagated from their legacy values, > and since the name of an option isn't really that important, I'd keep > the _GIT_ names for the mercurial options as well. > > If we do rename the option symbol names, then I would make sure that it > is propagated: > > default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" > > + add a comment to the .legacy file so these hacks can be removed > eventually. Hum, I'm not sure how this articulates with the _WRAP mechanism that patch 1/6 is proposing. If we do this: default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" then why would we need the _WRAP thing? Thanks, Thomas
On 13/08/13 11:09, Thomas Petazzoni wrote: > Dear Arnout Vandecappelle, > > On Mon, 12 Aug 2013 08:12:13 +0200, Arnout Vandecappelle wrote: > >> Given that string options can't be propagated from their legacy values, >> and since the name of an option isn't really that important, I'd keep >> the _GIT_ names for the mercurial options as well. >> >> If we do rename the option symbol names, then I would make sure that it >> is propagated: >> >> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >> >> + add a comment to the .legacy file so these hacks can be removed >> eventually. > > Hum, I'm not sure how this articulates with the _WRAP mechanism that > patch 1/6 is proposing. If we do this: > > default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" > > then why would we need the _WRAP thing? The _WRAP thing is needed to make sure there is a user-visible boolean option in the legacy menu, and to make sure you only select BR2_LEGACY when the old option is not empty. However, come to think of it, wouldn't it be enough to keep it as a string option and add select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" ? Although I guess Thomas has tested this already. Looking again at the patch, though, I wonder if it works at all. Aren't the old options that have no symbol definition removed? I think there should still be a config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL string in Config.in.legacy. But then, it will not be possible to modify the string anymore... so there is no escape... Argh, Kconfig... Regards, Arnout
Hi, On Tue, Aug 13, 2013 at 6:30 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 13/08/13 11:09, Thomas Petazzoni wrote: >> >> Dear Arnout Vandecappelle, >> >> On Mon, 12 Aug 2013 08:12:13 +0200, Arnout Vandecappelle wrote: >> >>> Given that string options can't be propagated from their legacy values, >>> and since the name of an option isn't really that important, I'd keep >>> the _GIT_ names for the mercurial options as well. >>> >>> If we do rename the option symbol names, then I would make sure that it >>> is propagated: >>> >>> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>> >>> + add a comment to the .legacy file so these hacks can be removed >>> eventually. >> >> >> Hum, I'm not sure how this articulates with the _WRAP mechanism that >> patch 1/6 is proposing. If we do this: >> >> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >> >> then why would we need the _WRAP thing? > > > The _WRAP thing is needed to make sure there is a user-visible boolean > option in the legacy menu, and to make sure you only select BR2_LEGACY when > the old option is not empty. Correct. Unlike for bool options that can be manipulated from other options, you cannot manipulate a string option from another option, only from the string option itself by adding a default statement, as Arnout proposed in his earlier mail. > > However, come to think of it, wouldn't it be enough to keep it as a string > option and add > > select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" > > ? Although I guess Thomas has tested this already. You mean this, right? config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL string "linux: the git repository URL option has been renamed" select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" I did indeed try this but it doesn't work. Kconfig gives the following message: Config.in.legacy:75:warning: config symbol 'BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL' uses select, but is not boolean or tristate > > Looking again at the patch, though, I wonder if it works at all. Aren't the > old options that have no symbol definition removed? I think there should > still be a > > config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL > string > > in Config.in.legacy. > > But then, it will not be possible to modify the string anymore... so there > is no escape... > > Argh, Kconfig... If you have the following config snippet before applying the patch: BR2_LINUX_KERNEL_CUSTOM_GIT=y BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL="my_repo_url" BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION="my_repo_version" BR2_LINUX_KERNEL_VERSION="my_repo_version" and then apply the patch and run 'make menuconfig' and save without changes, you get: BR2_LINUX_KERNEL_CUSTOM_GIT=y BR2_LINUX_KERNEL_CUSTOM_REPO_URL="" BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="" BR2_LINUX_KERNEL_VERSION="" BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL_WRAP=y BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION_WRAP=y i.e. the wrap thing does work. It's true that the original option BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL is no longer present _after_ saving the config, but the original value _has_ been detected and used to set the _WRAP option. The above behavior is the most you can achieve from the Config.in.legacy file. If you want to automatically propagate the string values, you _have_ to do it from linux/linux.mk. As this is the behavior a user would expect, I will update my patch with that change. Best regards, Thomas
On Wed, Aug 14, 2013 at 7:06 PM, Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> wrote: > Hi, > > On Tue, Aug 13, 2013 at 6:30 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >> On 13/08/13 11:09, Thomas Petazzoni wrote: >>> >>> Dear Arnout Vandecappelle, >>> >>> On Mon, 12 Aug 2013 08:12:13 +0200, Arnout Vandecappelle wrote: >>> >>>> Given that string options can't be propagated from their legacy values, >>>> and since the name of an option isn't really that important, I'd keep >>>> the _GIT_ names for the mercurial options as well. >>>> >>>> If we do rename the option symbol names, then I would make sure that it >>>> is propagated: >>>> >>>> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >>>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>>> >>>> + add a comment to the .legacy file so these hacks can be removed >>>> eventually. >>> >>> >>> Hum, I'm not sure how this articulates with the _WRAP mechanism that >>> patch 1/6 is proposing. If we do this: >>> >>> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>> >>> then why would we need the _WRAP thing? >> >> >> The _WRAP thing is needed to make sure there is a user-visible boolean >> option in the legacy menu, and to make sure you only select BR2_LEGACY when >> the old option is not empty. > > Correct. > Unlike for bool options that can be manipulated from other options, > you cannot manipulate a string option from another option, only from > the string option itself by adding a default statement, as Arnout > proposed in his earlier mail. > >> >> However, come to think of it, wouldn't it be enough to keep it as a string >> option and add >> >> select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >> >> ? Although I guess Thomas has tested this already. > > You mean this, right? > config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL > string "linux: the git repository URL option has been renamed" > select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" > > I did indeed try this but it doesn't work. Kconfig gives the following message: > > Config.in.legacy:75:warning: config symbol > 'BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL' uses select, but is not boolean > or tristate > >> >> Looking again at the patch, though, I wonder if it works at all. Aren't the >> old options that have no symbol definition removed? I think there should >> still be a >> >> config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL >> string >> >> in Config.in.legacy. >> >> But then, it will not be possible to modify the string anymore... so there >> is no escape... >> >> Argh, Kconfig... > > If you have the following config snippet before applying the patch: > BR2_LINUX_KERNEL_CUSTOM_GIT=y > BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL="my_repo_url" > BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION="my_repo_version" > BR2_LINUX_KERNEL_VERSION="my_repo_version" > > and then apply the patch and run 'make menuconfig' and save without > changes, you get: > > BR2_LINUX_KERNEL_CUSTOM_GIT=y > BR2_LINUX_KERNEL_CUSTOM_REPO_URL="" > BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="" > BR2_LINUX_KERNEL_VERSION="" > BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL_WRAP=y > BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION_WRAP=y > > i.e. the wrap thing does work. It's true that the original option > BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL is no longer present _after_ > saving the config, but the original value _has_ been detected and used > to set the _WRAP option. It seems this is not entirely correct: if you were not using CUSTOM_GIT options at all, the legacy options would incorrectly be set as well. I think we'll indeed have to re-introduce the original symbol as string in Config.in.legacy, as Arnout mentioned, but a plain config FOO_STRING string doesn't seem to work. The original symbol value is not stored. I can make it work with a non-hidden symbol: config FOO_STRING string "original string" but this means that the legacy menu would contain the original strings. Is that a problem? If it is, then some help with setting this up the right way would be greatly appreciated. I tried several things but can't find a better solution (except for keeping the original symbol names, which I'd like to avoid). Thanks, Thomas
On 17/08/13 10:13, Thomas De Schampheleire wrote: > On Wed, Aug 14, 2013 at 7:06 PM, Thomas De Schampheleire > <patrickdepinguin+buildroot@gmail.com> wrote: >> Hi, >> >> On Tue, Aug 13, 2013 at 6:30 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >>> On 13/08/13 11:09, Thomas Petazzoni wrote: >>>> >>>> Dear Arnout Vandecappelle, >>>> >>>> On Mon, 12 Aug 2013 08:12:13 +0200, Arnout Vandecappelle wrote: >>>> >>>>> Given that string options can't be propagated from their legacy values, >>>>> and since the name of an option isn't really that important, I'd keep >>>>> the _GIT_ names for the mercurial options as well. >>>>> >>>>> If we do rename the option symbol names, then I would make sure that it >>>>> is propagated: >>>>> >>>>> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >>>>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>>>> >>>>> + add a comment to the .legacy file so these hacks can be removed >>>>> eventually. >>>> >>>> >>>> Hum, I'm not sure how this articulates with the _WRAP mechanism that >>>> patch 1/6 is proposing. If we do this: >>>> >>>> default BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL if >>>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>>> >>>> then why would we need the _WRAP thing? >>> >>> >>> The _WRAP thing is needed to make sure there is a user-visible boolean >>> option in the legacy menu, and to make sure you only select BR2_LEGACY when >>> the old option is not empty. >> >> Correct. >> Unlike for bool options that can be manipulated from other options, >> you cannot manipulate a string option from another option, only from >> the string option itself by adding a default statement, as Arnout >> proposed in his earlier mail. >> >>> >>> However, come to think of it, wouldn't it be enough to keep it as a string >>> option and add >>> >>> select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >>> >>> ? Although I guess Thomas has tested this already. >> >> You mean this, right? >> config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL >> string "linux: the git repository URL option has been renamed" >> select BR2_LEGACY if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" >> >> I did indeed try this but it doesn't work. Kconfig gives the following message: >> >> Config.in.legacy:75:warning: config symbol >> 'BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL' uses select, but is not boolean >> or tristate >> >>> >>> Looking again at the patch, though, I wonder if it works at all. Aren't the >>> old options that have no symbol definition removed? I think there should >>> still be a >>> >>> config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL >>> string >>> >>> in Config.in.legacy. >>> >>> But then, it will not be possible to modify the string anymore... so there >>> is no escape... >>> >>> Argh, Kconfig... >> >> If you have the following config snippet before applying the patch: >> BR2_LINUX_KERNEL_CUSTOM_GIT=y >> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL="my_repo_url" >> BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION="my_repo_version" >> BR2_LINUX_KERNEL_VERSION="my_repo_version" >> >> and then apply the patch and run 'make menuconfig' and save without >> changes, you get: >> >> BR2_LINUX_KERNEL_CUSTOM_GIT=y >> BR2_LINUX_KERNEL_CUSTOM_REPO_URL="" >> BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="" >> BR2_LINUX_KERNEL_VERSION="" >> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL_WRAP=y >> BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION_WRAP=y >> >> i.e. the wrap thing does work. It's true that the original option >> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL is no longer present _after_ >> saving the config, but the original value _has_ been detected and used >> to set the _WRAP option. > > It seems this is not entirely correct: if you were not using > CUSTOM_GIT options at all, the legacy options would incorrectly be set > as well. I think we'll indeed have to re-introduce the original symbol > as string in Config.in.legacy, as Arnout mentioned, but a plain > config FOO_STRING > string > > doesn't seem to work. The original symbol value is not stored. I can > make it work with a non-hidden symbol: > config FOO_STRING > string "original string" > > but this means that the legacy menu would contain the original > strings. Is that a problem? For me, that as such is not much of a problem. In fact, I think you can make the _WRAP option hidden in that case. So to remove the legacy, you'd have to set FOO_STRING to the empty string again. config FOO_STRING string "FOO_STRING has been replaced by BAR_STRING" help ... config FOO_STRING_WRAP bool default y if FOO_STRING != "" select BR2_LEGACY However, in this case it is a simple symbol rename, then maybe we don't need to add anything to the legacy menu. Just add the default to BR2_LINUX_KERNEL_CUSTOM_REPO_URL without defining a symbol for BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL. I'm surprised that that works, though. Regards, Arnout > If it is, then some help with setting this up the right way would be > greatly appreciated. I tried several things but can't find a better > solution (except for keeping the original symbol names, which I'd like > to avoid). > > Thanks, > Thomas >
On Mon, Aug 19, 2013 at 6:05 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 17/08/13 10:13, Thomas De Schampheleire wrote: >> >>> >>> >>> If you have the following config snippet before applying the patch: >>> BR2_LINUX_KERNEL_CUSTOM_GIT=y >>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL="my_repo_url" >>> BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION="my_repo_version" >>> BR2_LINUX_KERNEL_VERSION="my_repo_version" >>> >>> and then apply the patch and run 'make menuconfig' and save without >>> changes, you get: >>> >>> BR2_LINUX_KERNEL_CUSTOM_GIT=y >>> BR2_LINUX_KERNEL_CUSTOM_REPO_URL="" >>> BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="" >>> BR2_LINUX_KERNEL_VERSION="" >>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL_WRAP=y >>> BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION_WRAP=y >>> >>> i.e. the wrap thing does work. It's true that the original option >>> BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL is no longer present _after_ >>> saving the config, but the original value _has_ been detected and used >>> to set the _WRAP option. >> >> >> It seems this is not entirely correct: if you were not using >> CUSTOM_GIT options at all, the legacy options would incorrectly be set >> as well. I think we'll indeed have to re-introduce the original symbol >> as string in Config.in.legacy, as Arnout mentioned, but a plain >> config FOO_STRING >> string >> >> doesn't seem to work. The original symbol value is not stored. I can >> make it work with a non-hidden symbol: >> config FOO_STRING >> string "original string" >> >> but this means that the legacy menu would contain the original >> strings. Is that a problem? > > > For me, that as such is not much of a problem. In fact, I think you can > make the _WRAP option hidden in that case. So to remove the legacy, you'd > have to set FOO_STRING to the empty string again. > > config FOO_STRING > string "FOO_STRING has been replaced by BAR_STRING" > help > ... > > config FOO_STRING_WRAP > bool > default y if FOO_STRING != "" > select BR2_LEGACY > > Thanks a lot for this suggestion. It indeed seems to work, and I like it: the original string values are still visible somewhere, and there is just one line in the legacy menu. > > > However, in this case it is a simple symbol rename, then maybe we don't > need to add anything to the legacy menu. Just add the default to > BR2_LINUX_KERNEL_CUSTOM_REPO_URL without defining a symbol for > BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL. I'm surprised that that works, though. It doesn't, I made a mistake there. You still need the original option somewhere. To come back on the question: should we try and propagate the options from old to new string options or not. The original patch did not do that, the user was responsible for making the change. Arnout responded that he'd like to make it 'just work' for the user, and advocated automatic propagation. I originally agreed to this reasoning, but am now reconsidering. To implement the automatic propagation of string values, we'd have: config FOO_NEW_STRING (in linux/Config.in) default FOO_OLD_STRING if FOO_OLD_STRING != "" config FOO_OLD_STRING (in Config.in.legacy) However, the behavior of this combination is odd: if you start from an old config, update to the newer buildroot, and run make menuconfig, you get the legacy warnings (as expected). In the Kernel menu, the new strings are correctly showing the original values (this is the automatic propagation). In the Legacy menu, the original values are shown too. Everything seems fine up to this point, but there are two scenarios now: 1. to clear the legacy messages, you have to empty the original string in the legacy menu. If you do that, however, the new string in the Kernel menu will also be cleared! If you now save your config, the original string is gone everywhere. 2. if you first save the config (legacy warnings still intact), then reopen menuconfig and clear the legacy warnings, the automatic propagation holds, and now the Kernel menu still contains the original values. This behavior is very confusing and annoying, IMO. A typical user would perform scenario 1, not knowing about the mentioned problem. An additional advantage of not automatic propagating, is that there is just one place that deals with legacy options: Config.in.legacy. To remove support for all legacy options, we could just delete that one file and be done with it. However, to automatically propagate string options, we also have to change additional files (linux/Config.in in this example), which is less clean. Both arguments lead me to advocating not automatically propagating legacy string options. Best regards, Thomas
On 08/20/13 10:36, Thomas De Schampheleire wrote: > To come back on the question: should we try and propagate the options > from old to new string options or not. > The original patch did not do that, the user was responsible for > making the change. > Arnout responded that he'd like to make it 'just work' for the user, > and advocated automatic propagation. > I originally agreed to this reasoning, but am now reconsidering. > To implement the automatic propagation of string values, we'd have: > > config FOO_NEW_STRING (in linux/Config.in) > default FOO_OLD_STRING if FOO_OLD_STRING != "" > > config FOO_OLD_STRING (in Config.in.legacy) > > However, the behavior of this combination is odd: if you start from an > old config, update to the newer buildroot, and run make menuconfig, > you get the legacy warnings (as expected). In the Kernel menu, the new > strings are correctly showing the original values (this is the > automatic propagation). In the Legacy menu, the original values are > shown too. Everything seems fine up to this point, but there are two > scenarios now: > 1. to clear the legacy messages, you have to empty the original string > in the legacy menu. If you do that, however, the new string in the > Kernel menu will also be cleared! If you now save your config, the > original string is gone everywhere. > 2. if you first save the config (legacy warnings still intact), then > reopen menuconfig and clear the legacy warnings, the automatic > propagation holds, and now the Kernel menu still contains the original > values. > > This behavior is very confusing and annoying, IMO. A typical user > would perform scenario 1, not knowing about the mentioned problem. I agree that it is confusing and annoying. However, this is the same behaviour as for boolean options. So if we accept this argument, then also the automatic propagation for boolean options should be removed. So maybe we could make the options with automatic propagation not select BR2_LEGACY. If it is indeed just a symbol rename, then the user doesn't need to be made aware of it, I guess. The disadvantage is that the old options will still appear in saved defconfigs, but that's a minor thing according to me. Another option is warn the user in the comments at the top of Config.in.legacy that they should save and re-run menuconfig. > An additional advantage of not automatic propagating, is that there is > just one place that deals with legacy options: Config.in.legacy. To > remove support for all legacy options, we could just delete that one > file and be done with it. However, to automatically propagate string > options, we also have to change additional files (linux/Config.in in > this example), which is less clean. But I don't think that's a big deal. Config.in.legacy can contain some text to point to the other place where the symbol has to be removed. > Both arguments lead me to advocating not automatically propagating > legacy string options. If that is the case, then simple renames of config symbols should still be avoided, I think. We want it to be really easy for people to upgrade buildroot, so they shouldn't be exposed to the whole legacy stuff unless really needed. Regards, Arnout
All, On Wed, Aug 21, 2013 at 7:40 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 08/20/13 10:36, Thomas De Schampheleire wrote: >> >> To come back on the question: should we try and propagate the options >> from old to new string options or not. >> The original patch did not do that, the user was responsible for >> making the change. >> Arnout responded that he'd like to make it 'just work' for the user, >> and advocated automatic propagation. >> I originally agreed to this reasoning, but am now reconsidering. >> To implement the automatic propagation of string values, we'd have: >> >> config FOO_NEW_STRING (in linux/Config.in) >> default FOO_OLD_STRING if FOO_OLD_STRING != "" >> >> config FOO_OLD_STRING (in Config.in.legacy) >> >> However, the behavior of this combination is odd: if you start from an >> old config, update to the newer buildroot, and run make menuconfig, >> you get the legacy warnings (as expected). In the Kernel menu, the new >> strings are correctly showing the original values (this is the >> automatic propagation). In the Legacy menu, the original values are >> shown too. Everything seems fine up to this point, but there are two >> scenarios now: >> 1. to clear the legacy messages, you have to empty the original string >> in the legacy menu. If you do that, however, the new string in the >> Kernel menu will also be cleared! If you now save your config, the >> original string is gone everywhere. >> 2. if you first save the config (legacy warnings still intact), then >> reopen menuconfig and clear the legacy warnings, the automatic >> propagation holds, and now the Kernel menu still contains the original >> values. > >> >> This behavior is very confusing and annoying, IMO. A typical user >> would perform scenario 1, not knowing about the mentioned problem. > > > I agree that it is confusing and annoying. However, this is the same > behaviour as for boolean options. So if we accept this argument, then also > the automatic propagation for boolean options should be removed. > > So maybe we could make the options with automatic propagation not select > BR2_LEGACY. If it is indeed just a symbol rename, then the user doesn't need > to be made aware of it, I guess. The disadvantage is that the old options > will still appear in saved defconfigs, but that's a minor thing according to > me. > > Another option is warn the user in the comments at the top of > Config.in.legacy that they should save and re-run menuconfig. > > > > >> An additional advantage of not automatic propagating, is that there is >> just one place that deals with legacy options: Config.in.legacy. To >> remove support for all legacy options, we could just delete that one >> file and be done with it. However, to automatically propagate string >> options, we also have to change additional files (linux/Config.in in >> this example), which is less clean. > > > But I don't think that's a big deal. Config.in.legacy can contain some text > to point to the other place where the symbol has to be removed. > > > >> Both arguments lead me to advocating not automatically propagating >> legacy string options. > > > If that is the case, then simple renames of config symbols should still be > avoided, I think. We want it to be really easy for people to upgrade > buildroot, so they shouldn't be exposed to the whole legacy stuff unless > really needed. > What is the input of the buildroot community on this thread? Thanks, Thomas
Hi Arnout, On Wed, Aug 21, 2013 at 7:40 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 08/20/13 10:36, Thomas De Schampheleire wrote: >> >> To come back on the question: should we try and propagate the options >> from old to new string options or not. >> The original patch did not do that, the user was responsible for >> making the change. >> Arnout responded that he'd like to make it 'just work' for the user, >> and advocated automatic propagation. >> I originally agreed to this reasoning, but am now reconsidering. >> To implement the automatic propagation of string values, we'd have: >> >> config FOO_NEW_STRING (in linux/Config.in) >> default FOO_OLD_STRING if FOO_OLD_STRING != "" >> >> config FOO_OLD_STRING (in Config.in.legacy) >> >> However, the behavior of this combination is odd: if you start from an >> old config, update to the newer buildroot, and run make menuconfig, >> you get the legacy warnings (as expected). In the Kernel menu, the new >> strings are correctly showing the original values (this is the >> automatic propagation). In the Legacy menu, the original values are >> shown too. Everything seems fine up to this point, but there are two >> scenarios now: >> 1. to clear the legacy messages, you have to empty the original string >> in the legacy menu. If you do that, however, the new string in the >> Kernel menu will also be cleared! If you now save your config, the >> original string is gone everywhere. >> 2. if you first save the config (legacy warnings still intact), then >> reopen menuconfig and clear the legacy warnings, the automatic >> propagation holds, and now the Kernel menu still contains the original >> values. > >> >> This behavior is very confusing and annoying, IMO. A typical user >> would perform scenario 1, not knowing about the mentioned problem. > > > I agree that it is confusing and annoying. However, this is the same > behaviour as for boolean options. So if we accept this argument, then also > the automatic propagation for boolean options should be removed. > I wasn't aware that this behavior was also there for bool options. I agree that it doesn't make sense to make strings behave 'normal' if it isn't true for bool options (which are in the majority). > So maybe we could make the options with automatic propagation not select > BR2_LEGACY. If it is indeed just a symbol rename, then the user doesn't need > to be made aware of it, I guess. The disadvantage is that the old options > will still appear in saved defconfigs, but that's a minor thing according to > me. I can't make it work in this case. If you just remove 'select BR2_LEGACY' from a legacy bool option, it is still visible in the legacy menu, and users will tend to remove it if they already know how the legacy menu works. Here, you still have to save the config in between to avoid losing an option. If you remove the visibility of the legacy option, I don't get automatic propagation (I may be doing something wrong). > > Another option is warn the user in the comments at the top of > Config.in.legacy that they should save and re-run menuconfig. This is certainly acceptable to me. > > > > >> An additional advantage of not automatic propagating, is that there is >> just one place that deals with legacy options: Config.in.legacy. To >> remove support for all legacy options, we could just delete that one >> file and be done with it. However, to automatically propagate string >> options, we also have to change additional files (linux/Config.in in >> this example), which is less clean. > > > But I don't think that's a big deal. Config.in.legacy can contain some text > to point to the other place where the symbol has to be removed. I will certainly not block this if others prefer the automatic propagation, it's indeed not a showstopper. But I'd really like to hear some other opinions on this. > > > >> Both arguments lead me to advocating not automatically propagating >> legacy string options. > > > If that is the case, then simple renames of config symbols should still be > avoided, I think. We want it to be really easy for people to upgrade > buildroot, so they shouldn't be exposed to the whole legacy stuff unless > really needed. Unnecessary renames should indeed be avoided, but in this case of GIT/HG options I don't feel it's unnecessary. Best regards, Thomas
diff --git a/Config.in.legacy b/Config.in.legacy --- a/Config.in.legacy +++ b/Config.in.legacy @@ -59,6 +59,24 @@ endif ############################################################################### comment "Legacy options removed in 2013.08" +config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL_WRAP + bool "linux: the git repository URL option has been renamed" + default y if BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL != "" + select BR2_LEGACY + help + The option BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL has + been renamed to + BR2_LINUX_KERNEL_CUSTOM_REPO_URL. + +config BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION_WRAP + bool "linux: the git repository version option has been renamed" + default y if BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION != "" + select BR2_LEGACY + help + The option BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION has + been renamed to + BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION. + config BR2_PACKAGE_DOSFSTOOLS_DOSFSCK bool "dosfstools dosfsck renamed to fsck.fat" select BR2_LEGACY diff --git a/linux/Config.in b/linux/Config.in --- a/linux/Config.in +++ b/linux/Config.in @@ -52,6 +52,12 @@ config BR2_LINUX_KERNEL_CUSTOM_GIT This option allows Buildroot to get the Linux kernel source code from a Git repository. +config BR2_LINUX_KERNEL_CUSTOM_HG + bool "Custom Mercurial repository" + help + This option allows Buildroot to get the Linux kernel source + code from a Mercurial repository. + endchoice config BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE @@ -62,24 +68,26 @@ config BR2_LINUX_KERNEL_CUSTOM_TARBALL_L string "URL of custom kernel tarball" depends on BR2_LINUX_KERNEL_CUSTOM_TARBALL -config BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL - string "URL of custom Git repository" - depends on BR2_LINUX_KERNEL_CUSTOM_GIT +if BR2_LINUX_KERNEL_CUSTOM_GIT || BR2_LINUX_KERNEL_CUSTOM_HG -config BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION - string "Custom Git version" - depends on BR2_LINUX_KERNEL_CUSTOM_GIT +config BR2_LINUX_KERNEL_CUSTOM_REPO_URL + string "URL of custom repository" + +config BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION + string "Custom repository version" help - Git revision to use in the format used by git rev-parse, + Revision to use in the typical format used by Git/Mercurial E.G. a sha id, a tag, branch, .. +endif + config BR2_LINUX_KERNEL_VERSION string default "3.10.1" if BR2_LINUX_KERNEL_LATEST_VERSION default BR2_DEFAULT_KERNEL_HEADERS if BR2_LINUX_KERNEL_SAME_AS_HEADERS default BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE if BR2_LINUX_KERNEL_CUSTOM_VERSION default "custom" if BR2_LINUX_KERNEL_CUSTOM_TARBALL - default $BR2_LINUX_KERNEL_CUSTOM_GIT_VERSION if BR2_LINUX_KERNEL_CUSTOM_GIT + default $BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION if BR2_LINUX_KERNEL_CUSTOM_HG || BR2_LINUX_KERNEL_CUSTOM_GIT # # Patch selection diff --git a/linux/linux.mk b/linux/linux.mk --- a/linux/linux.mk +++ b/linux/linux.mk @@ -14,8 +14,11 @@ LINUX_TARBALL = $(call qstrip,$(BR2_LINU LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) LINUX_SOURCE = $(notdir $(LINUX_TARBALL)) else ifeq ($(BR2_LINUX_KERNEL_CUSTOM_GIT),y) -LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_GIT_REPO_URL)) +LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_REPO_URL)) LINUX_SITE_METHOD = git +else ifeq ($(BR2_LINUX_KERNEL_CUSTOM_HG),y) +LINUX_SITE = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_REPO_URL)) +LINUX_SITE_METHOD = hg else LINUX_SOURCE = linux-$(LINUX_VERSION).tar.xz # In X.Y.Z, get X and Y. We replace dots and dashes by spaces in order
Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> --- v2: add Config.in.legacy entries (comment Peter) Config.in.legacy | 18 ++++++++++++++++++ linux/Config.in | 24 ++++++++++++++++-------- linux/linux.mk | 5 ++++- 3 files changed, 38 insertions(+), 9 deletions(-)