Patchwork [3,of,6,v2] linux: add support for custom Mercurial repository

login
register
mail settings
Submitter Thomas De Schampheleire
Date July 24, 2013, 7:23 a.m.
Message ID <702ce86cc56aec78ac12.1374650580@BEANTN0L019720>
Download mbox | patch
Permalink /patch/261298/
State Superseded
Headers show

Comments

Thomas De Schampheleire - July 24, 2013, 7:23 a.m.
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(-)
Arnout Vandecappelle - Aug. 12, 2013, 6:12 a.m.
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
>
Thomas De Schampheleire - Aug. 12, 2013, 10:54 a.m.
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
Arnout Vandecappelle - Aug. 12, 2013, 11:01 a.m.
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
Thomas Petazzoni - Aug. 13, 2013, 9:09 a.m.
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
Arnout Vandecappelle - Aug. 13, 2013, 4:30 p.m.
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
Thomas De Schampheleire - Aug. 14, 2013, 5:06 p.m.
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
Thomas De Schampheleire - Aug. 17, 2013, 8:13 a.m.
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
Arnout Vandecappelle - Aug. 19, 2013, 4:05 p.m.
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
>
Thomas De Schampheleire - Aug. 20, 2013, 8:36 a.m.
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
Arnout Vandecappelle - Aug. 21, 2013, 5:40 a.m.
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
Thomas De Schampheleire - Aug. 27, 2013, 12:29 p.m.
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
Thomas De Schampheleire - Aug. 27, 2013, 2:14 p.m.
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

Patch

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