* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 15:07 [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available Romain Izard
@ 2016-04-19 17:30 ` Thomas Petazzoni
2016-04-19 17:45 ` Yann E. MORIN
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 17:30 UTC (permalink / raw)
To: buildroot
Hello,
On Tue, 19 Apr 2016 17:07:14 +0200, Romain Izard wrote:
> +$(2)_KCONFIG_RULES = \
> + $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
> + sed 's/^\([_0-9a-zA-Z]*config:\).*/\1/ p; d')
Does this actually work? Does this gets expanded/evaluated *before* the
package source code is extracted?
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 15:07 [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available Romain Izard
2016-04-19 17:30 ` Thomas Petazzoni
@ 2016-04-19 17:45 ` Yann E. MORIN
2016-04-19 20:35 ` Yann E. MORIN
2016-04-19 20:35 ` Thomas Petazzoni
2016-04-19 20:57 ` Yann E. MORIN
3 siblings, 1 reply; 7+ messages in thread
From: Yann E. MORIN @ 2016-04-19 17:45 UTC (permalink / raw)
To: buildroot
Romain, All,
On 2016-04-19 17:07 +0200, Romain Izard spake thusly:
> Using 'yes "" | make oldconfig' to regenerate a .config file from an
> existing defconfig does not work reliably in all cases. Specifically, it
> does not work well with tristate choice entries.
>
> The correct way to do it is to use 'make olddefconfig', but this target
> is not supported in projects that use an old version of kconfig. As
> most projects do not use tristate entries, there is no need for them to
> upgrade.
>
> 'oldnoconfig' is an alias for 'olddefconfig', but it also is the only
> name for this target between Linux versions 2.6.36 and 3.6.
>
> Check which targets are supported by the current project, and use the
> appropriate method to regenerate the .config file.
>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
> Changelog:
> v1: New patch
> v2: Use oldnoconfig instead of olddefconfig
> Fix silent mode
> Change the pattern in both places where it is used
> Use a macro to avoid redundancy
> v3: Do not use $(call ...)
> Use the $(2)_KCONFIG_MAKE variable
> v4: No change
> v5: Detect available methods and use the most recent one
>
> package/pkg-kconfig.mk | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> index 4033915..e14b33f 100644
> --- a/package/pkg-kconfig.mk
> +++ b/package/pkg-kconfig.mk
> @@ -60,18 +60,35 @@ $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
> $(2)_KCONFIG_MAKE = \
> $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) $$($(2)_KCONFIG_OPTS)
>
> +$(2)_KCONFIG_RULES = \
> + $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
> + sed 's/^\([_0-9a-zA-Z]*config:\).*/\1/ p; d')
NAK, because this will be executed at the time the package rules are
exapnded (i.e. during the call to $(eval $(kconfig-package)), so would
not work for a build from scratch...
(but double-dollar make that assertion difficult to asses... I'd have to
double-check).
Besides...
> +# The correct way to regenerate a .config file is to use 'make olddefconfig'.
> +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel
> +# versions 2.6.36 and 3.6, and remains as an alias in later versions.
> +# In older versions, and in some other projects that use kconfig, the target is
> +# not supported at all, and we use 'yes "" | make oldconfig' as a fallback
> +# only, as this can fail in complex cases.
> +define $(2)_REGEN_DOT_CONFIG
> + $$(if $$(filter olddefconfig:,$$($(2)_KCONFIG_RULES)),
> + $$(Q)$$($(2)_KCONFIG_MAKE) olddefconfig,
> + $$(if $$(filter oldnoconfig:,$$($(2)_KCONFIG_RULES)),
> + $$(Q)$$($(2)_KCONFIG_MAKE) oldnoconfig,
> + $$(Q)(yes "" | $$($(2)_KCONFIG_MAKE) oldconfig)))
> +endef
... this would get expand twice, so the shell construct would be run
twice, whereas doing it all in shell as I suggested would only run the
needed checks: if olddefconfig is supported, the check for oldnoconfig
would no even run; in your case, both tests would be run, always.
But yeah, my proposal had a flaw; even my fix on that proposal had a
flaw: the output of the grep should not be >/dev/null, since we want to
check whether it isempty or not...
Regards,
Yann E. MORIN.
> # The specified source configuration file and any additional configuration file
> # fragments are merged together to .config, after the package has been patched.
> # Since the file could be a defconfig file it needs to be expanded to a
> -# full .config first. We use 'make oldconfig' because this can be safely
> -# done even when the package does not support defconfigs.
> +# full .config first.
> $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
> $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
> $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
> cp $$($(2)_KCONFIG_FILE) $$(@))
> $$(Q)support/kconfig/merge_config.sh -m -O $$(@D) \
> $$(@) $$($(2)_KCONFIG_FRAGMENT_FILES)
> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> + $$($(2)_REGEN_DOT_CONFIG)
>
> # If _KCONFIG_FILE or _KCONFIG_FRAGMENT_FILES exists, this dependency is
> # already implied, but if we only have a _KCONFIG_DEFCONFIG we have to add
> @@ -82,7 +99,7 @@ $$($(2)_DIR)/.config: | $(1)-patch
> # The exact rules are specified by the package .mk file.
> define $(2)_FIXUP_DOT_CONFIG
> $$($(2)_KCONFIG_FIXUP_CMDS)
> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> + $$($(2)_REGEN_DOT_CONFIG)
> $$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done
> endef
>
> --
> 2.5.0
>
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 17:45 ` Yann E. MORIN
@ 2016-04-19 20:35 ` Yann E. MORIN
2016-04-19 22:24 ` Arnout Vandecappelle
0 siblings, 1 reply; 7+ messages in thread
From: Yann E. MORIN @ 2016-04-19 20:35 UTC (permalink / raw)
To: buildroot
Romain, All,
On 2016-04-19 19:45 +0200, Yann E. MORIN spake thusly:
> On 2016-04-19 17:07 +0200, Romain Izard spake thusly:
> > Using 'yes "" | make oldconfig' to regenerate a .config file from an
> > existing defconfig does not work reliably in all cases. Specifically, it
> > does not work well with tristate choice entries.
> >
> > The correct way to do it is to use 'make olddefconfig', but this target
> > is not supported in projects that use an old version of kconfig. As
> > most projects do not use tristate entries, there is no need for them to
> > upgrade.
> >
> > 'oldnoconfig' is an alias for 'olddefconfig', but it also is the only
> > name for this target between Linux versions 2.6.36 and 3.6.
> >
> > Check which targets are supported by the current project, and use the
> > appropriate method to regenerate the .config file.
> >
> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> > ---
> > Changelog:
> > v1: New patch
> > v2: Use oldnoconfig instead of olddefconfig
> > Fix silent mode
> > Change the pattern in both places where it is used
> > Use a macro to avoid redundancy
> > v3: Do not use $(call ...)
> > Use the $(2)_KCONFIG_MAKE variable
> > v4: No change
> > v5: Detect available methods and use the most recent one
> >
> > package/pkg-kconfig.mk | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> > index 4033915..e14b33f 100644
> > --- a/package/pkg-kconfig.mk
> > +++ b/package/pkg-kconfig.mk
> > @@ -60,18 +60,35 @@ $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
> > $(2)_KCONFIG_MAKE = \
> > $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) $$($(2)_KCONFIG_OPTS)
> >
> > +$(2)_KCONFIG_RULES = \
> > + $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
> > + sed 's/^\([_0-9a-zA-Z]*config:\).*/\1/ p; d')
>
> NAK, because this will be executed at the time the package rules are
> exapnded (i.e. during the call to $(eval $(kconfig-package)), so would
> not work for a build from scratch...
>
> (but double-dollar make that assertion difficult to asses... I'd have to
> double-check).
OK, so I withdraw that comment: it *does* work. And the second shell
eval is only called when needed! :-)
Still, I'm not too fond of it being written in Makefile. Especially the
part below...
> Besides...
>
> > +# The correct way to regenerate a .config file is to use 'make olddefconfig'.
> > +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel
> > +# versions 2.6.36 and 3.6, and remains as an alias in later versions.
> > +# In older versions, and in some other projects that use kconfig, the target is
> > +# not supported at all, and we use 'yes "" | make oldconfig' as a fallback
> > +# only, as this can fail in complex cases.
> > +define $(2)_REGEN_DOT_CONFIG
> > + $$(if $$(filter olddefconfig:,$$($(2)_KCONFIG_RULES)),
> > + $$(Q)$$($(2)_KCONFIG_MAKE) olddefconfig,
> > + $$(if $$(filter oldnoconfig:,$$($(2)_KCONFIG_RULES)),
> > + $$(Q)$$($(2)_KCONFIG_MAKE) oldnoconfig,
> > + $$(Q)(yes "" | $$($(2)_KCONFIG_MAKE) oldconfig)))
> > +endef
I am not usually one to argue against writting Makefile code in a
Makefile. However, for this case, I think writting a shell rule is much
more readable.
And I don't see how your solution would be inherently different from
mine: both use 'make -pqn' to get the list of targets and grep/sed the
output to decide if the requested target exists...
By the way, if your sed expression would have moved the colon ':'
outside the ()-expression, you would not have had to repeqt the colon in
the filter match. Also, using extended regexps would allow using
character classes.
$(2)_KCONFIG_RULES = \
$$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
sed -r '/^([[:alnum:]]*config):.*/!d; s//\1/;')
Again, I think a shell construct is either to read.
Regards,
Yann E. MORIN.
> > # The specified source configuration file and any additional configuration file
> > # fragments are merged together to .config, after the package has been patched.
> > # Since the file could be a defconfig file it needs to be expanded to a
> > -# full .config first. We use 'make oldconfig' because this can be safely
> > -# done even when the package does not support defconfigs.
> > +# full .config first.
> > $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
> > $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
> > $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
> > cp $$($(2)_KCONFIG_FILE) $$(@))
> > $$(Q)support/kconfig/merge_config.sh -m -O $$(@D) \
> > $$(@) $$($(2)_KCONFIG_FRAGMENT_FILES)
> > - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> > + $$($(2)_REGEN_DOT_CONFIG)
> >
> > # If _KCONFIG_FILE or _KCONFIG_FRAGMENT_FILES exists, this dependency is
> > # already implied, but if we only have a _KCONFIG_DEFCONFIG we have to add
> > @@ -82,7 +99,7 @@ $$($(2)_DIR)/.config: | $(1)-patch
> > # The exact rules are specified by the package .mk file.
> > define $(2)_FIXUP_DOT_CONFIG
> > $$($(2)_KCONFIG_FIXUP_CMDS)
> > - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> > + $$($(2)_REGEN_DOT_CONFIG)
> > $$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done
> > endef
> >
> > --
> > 2.5.0
> >
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
> | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
> '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 7+ messages in thread* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 20:35 ` Yann E. MORIN
@ 2016-04-19 22:24 ` Arnout Vandecappelle
0 siblings, 0 replies; 7+ messages in thread
From: Arnout Vandecappelle @ 2016-04-19 22:24 UTC (permalink / raw)
To: buildroot
On 04/19/16 22:35, Yann E. MORIN wrote:
> Romain, All,
>
> On 2016-04-19 19:45 +0200, Yann E. MORIN spake thusly:
>> On 2016-04-19 17:07 +0200, Romain Izard spake thusly:
>>> Using 'yes "" | make oldconfig' to regenerate a .config file from an
>>> existing defconfig does not work reliably in all cases. Specifically, it
>>> does not work well with tristate choice entries.
>>>
>>> The correct way to do it is to use 'make olddefconfig', but this target
>>> is not supported in projects that use an old version of kconfig. As
>>> most projects do not use tristate entries, there is no need for them to
>>> upgrade.
>>>
>>> 'oldnoconfig' is an alias for 'olddefconfig', but it also is the only
>>> name for this target between Linux versions 2.6.36 and 3.6.
>>>
>>> Check which targets are supported by the current project, and use the
>>> appropriate method to regenerate the .config file.
>>>
>>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>>> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>>> ---
>>> Changelog:
>>> v1: New patch
>>> v2: Use oldnoconfig instead of olddefconfig
>>> Fix silent mode
>>> Change the pattern in both places where it is used
>>> Use a macro to avoid redundancy
>>> v3: Do not use $(call ...)
>>> Use the $(2)_KCONFIG_MAKE variable
>>> v4: No change
>>> v5: Detect available methods and use the most recent one
>>>
>>> package/pkg-kconfig.mk | 18 ++++++++++++++----
>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
>>> index 4033915..e14b33f 100644
>>> --- a/package/pkg-kconfig.mk
>>> +++ b/package/pkg-kconfig.mk
>>> @@ -60,18 +60,35 @@ $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
>>> $(2)_KCONFIG_MAKE = \
>>> $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) $$($(2)_KCONFIG_OPTS)
>>>
>>> +$(2)_KCONFIG_RULES = \
>>> + $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
>>> + sed 's/^\([_0-9a-zA-Z]*config:\).*/\1/ p; d')
>>
>> NAK, because this will be executed at the time the package rules are
>> exapnded (i.e. during the call to $(eval $(kconfig-package)), so would
>> not work for a build from scratch...
>>
>> (but double-dollar make that assertion difficult to asses... I'd have to
>> double-check).
>
> OK, so I withdraw that comment: it *does* work. And the second shell
> eval is only called when needed! :-)
>
> Still, I'm not too fond of it being written in Makefile. Especially the
> part below...
>
>> Besides...
>>
>>> +# The correct way to regenerate a .config file is to use 'make olddefconfig'.
>>> +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel
>>> +# versions 2.6.36 and 3.6, and remains as an alias in later versions.
>>> +# In older versions, and in some other projects that use kconfig, the target is
>>> +# not supported at all, and we use 'yes "" | make oldconfig' as a fallback
>>> +# only, as this can fail in complex cases.
>>> +define $(2)_REGEN_DOT_CONFIG
>>> + $$(if $$(filter olddefconfig:,$$($(2)_KCONFIG_RULES)),
>>> + $$(Q)$$($(2)_KCONFIG_MAKE) olddefconfig,
>>> + $$(if $$(filter oldnoconfig:,$$($(2)_KCONFIG_RULES)),
>>> + $$(Q)$$($(2)_KCONFIG_MAKE) oldnoconfig,
>>> + $$(Q)(yes "" | $$($(2)_KCONFIG_MAKE) oldconfig)))
>>> +endef
>
> I am not usually one to argue against writting Makefile code in a
> Makefile. However, for this case, I think writting a shell rule is much
> more readable.
+1 to that.
And as Thomas said: my Rev-by no longer applies.
Regards,
Arnout
>
> And I don't see how your solution would be inherently different from
> mine: both use 'make -pqn' to get the list of targets and grep/sed the
> output to decide if the requested target exists...
>
> By the way, if your sed expression would have moved the colon ':'
> outside the ()-expression, you would not have had to repeqt the colon in
> the filter match. Also, using extended regexps would allow using
> character classes.
>
> $(2)_KCONFIG_RULES = \
> $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
> sed -r '/^([[:alnum:]]*config):.*/!d; s//\1/;')
>
> Again, I think a shell construct is either to read.
>
> Regards,
> Yann E. MORIN.
>
>>> # The specified source configuration file and any additional configuration file
>>> # fragments are merged together to .config, after the package has been patched.
>>> # Since the file could be a defconfig file it needs to be expanded to a
>>> -# full .config first. We use 'make oldconfig' because this can be safely
>>> -# done even when the package does not support defconfigs.
>>> +# full .config first.
>>> $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
>>> $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
>>> $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
>>> cp $$($(2)_KCONFIG_FILE) $$(@))
>>> $$(Q)support/kconfig/merge_config.sh -m -O $$(@D) \
>>> $$(@) $$($(2)_KCONFIG_FRAGMENT_FILES)
>>> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
>>> + $$($(2)_REGEN_DOT_CONFIG)
>>>
>>> # If _KCONFIG_FILE or _KCONFIG_FRAGMENT_FILES exists, this dependency is
>>> # already implied, but if we only have a _KCONFIG_DEFCONFIG we have to add
>>> @@ -82,7 +99,7 @@ $$($(2)_DIR)/.config: | $(1)-patch
>>> # The exact rules are specified by the package .mk file.
>>> define $(2)_FIXUP_DOT_CONFIG
>>> $$($(2)_KCONFIG_FIXUP_CMDS)
>>> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
>>> + $$($(2)_REGEN_DOT_CONFIG)
>>> $$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done
>>> endef
>>>
>>> --
>>> 2.5.0
>>>
>>
>> --
>> .-----------------.--------------------.------------------.--------------------.
>> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
>> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
>> | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
>> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
>> '------------------------------^-------^------------------^--------------------'
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>
--
Arnout Vandecappelle arnout at mind be
Senior Embedded Software Architect +32-16-286500
Essensium/Mind http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 15:07 [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available Romain Izard
2016-04-19 17:30 ` Thomas Petazzoni
2016-04-19 17:45 ` Yann E. MORIN
@ 2016-04-19 20:35 ` Thomas Petazzoni
2016-04-19 20:57 ` Yann E. MORIN
3 siblings, 0 replies; 7+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 20:35 UTC (permalink / raw)
To: buildroot
Romain,
On Tue, 19 Apr 2016 17:07:14 +0200, Romain Izard wrote:
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Where did Arnout gave you his Reviewed-by?
If he did give you his Reviewed-by on a previous version, and you
changed the patch significantly (which you did), then you cannot keep
his Reviewed-by tag because you have no idea if his review still holds.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available
2016-04-19 15:07 [Buildroot] [PATCH v5] core/pkg-kconfig: Use olddefconfig when available Romain Izard
` (2 preceding siblings ...)
2016-04-19 20:35 ` Thomas Petazzoni
@ 2016-04-19 20:57 ` Yann E. MORIN
3 siblings, 0 replies; 7+ messages in thread
From: Yann E. MORIN @ 2016-04-19 20:57 UTC (permalink / raw)
To: buildroot
Romain, All,
On 2016-04-19 17:07 +0200, Romain Izard spake thusly:
> Using 'yes "" | make oldconfig' to regenerate a .config file from an
> existing defconfig does not work reliably in all cases. Specifically, it
> does not work well with tristate choice entries.
>
> The correct way to do it is to use 'make olddefconfig', but this target
> is not supported in projects that use an old version of kconfig. As
> most projects do not use tristate entries, there is no need for them to
> upgrade.
>
> 'oldnoconfig' is an alias for 'olddefconfig', but it also is the only
> name for this target between Linux versions 2.6.36 and 3.6.
>
> Check which targets are supported by the current project, and use the
> appropriate method to regenerate the .config file.
>
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
As Thomas said: this was given on a previous iteration of the patch, and
this one is fundamentaly different; the review by arnout no longer
stand on this new version.
However, after some discussion with Thomas on IRC, here is a more
thourough review from me...
> ---
> Changelog:
> v1: New patch
> v2: Use oldnoconfig instead of olddefconfig
> Fix silent mode
> Change the pattern in both places where it is used
> Use a macro to avoid redundancy
> v3: Do not use $(call ...)
> Use the $(2)_KCONFIG_MAKE variable
> v4: No change
> v5: Detect available methods and use the most recent one
>
> package/pkg-kconfig.mk | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> index 4033915..e14b33f 100644
> --- a/package/pkg-kconfig.mk
> +++ b/package/pkg-kconfig.mk
> @@ -60,18 +60,35 @@ $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES): | $(1)-patch
> $(2)_KCONFIG_MAKE = \
> $$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) $$($(2)_KCONFIG_OPTS)
>
> +$(2)_KCONFIG_RULES = \
> + $$(shell $$($(2)_KCONFIG_MAKE) -pn config 2>/dev/null | \
> + sed 's/^\([_0-9a-zA-Z]*config:\).*/\1/ p; d')
Move the colon ':' outside the ()-expression, and then...
> +# The correct way to regenerate a .config file is to use 'make olddefconfig'.
> +# For historical reasons, the target name is 'oldnoconfig' between Linux kernel
> +# versions 2.6.36 and 3.6, and remains as an alias in later versions.
> +# In older versions, and in some other projects that use kconfig, the target is
> +# not supported at all, and we use 'yes "" | make oldconfig' as a fallback
> +# only, as this can fail in complex cases.
> +define $(2)_REGEN_DOT_CONFIG
> + $$(if $$(filter olddefconfig:,$$($(2)_KCONFIG_RULES)),
... drop the colon here...
> + $$(Q)$$($(2)_KCONFIG_MAKE) olddefconfig,
> + $$(if $$(filter oldnoconfig:,$$($(2)_KCONFIG_RULES)),
... and here.
When you fix that, then you can add my:
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Thanks! :-)
Regards,
Yann E. MORIN.
> + $$(Q)$$($(2)_KCONFIG_MAKE) oldnoconfig,
> + $$(Q)(yes "" | $$($(2)_KCONFIG_MAKE) oldconfig)))
> +endef
> +
> # The specified source configuration file and any additional configuration file
> # fragments are merged together to .config, after the package has been patched.
> # Since the file could be a defconfig file it needs to be expanded to a
> -# full .config first. We use 'make oldconfig' because this can be safely
> -# done even when the package does not support defconfigs.
> +# full .config first.
> $$($(2)_DIR)/.config: $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
> $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
> $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
> cp $$($(2)_KCONFIG_FILE) $$(@))
> $$(Q)support/kconfig/merge_config.sh -m -O $$(@D) \
> $$(@) $$($(2)_KCONFIG_FRAGMENT_FILES)
> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> + $$($(2)_REGEN_DOT_CONFIG)
>
> # If _KCONFIG_FILE or _KCONFIG_FRAGMENT_FILES exists, this dependency is
> # already implied, but if we only have a _KCONFIG_DEFCONFIG we have to add
> @@ -82,7 +99,7 @@ $$($(2)_DIR)/.config: | $(1)-patch
> # The exact rules are specified by the package .mk file.
> define $(2)_FIXUP_DOT_CONFIG
> $$($(2)_KCONFIG_FIXUP_CMDS)
> - $$(Q)yes "" | $$($(2)_KCONFIG_MAKE) oldconfig
> + $$($(2)_REGEN_DOT_CONFIG)
> $$(Q)touch $$($(2)_DIR)/.stamp_kconfig_fixup_done
> endef
>
> --
> 2.5.0
>
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 7+ messages in thread