[Buildroot] [PATCH 0/2] pkg-kconfig: fix checking .config file (branch yem/misc)

[Buildroot] [PATCH 0/2] pkg-kconfig: fix checking .config file (branch yem/misc)

[Yann E. MORIN]

Hello All!

This series tries to fix the pkg-kconfig infra for disabled packages.

Currently, the pkg-kconfig infra always checks that the specified
FOO_KCONFIG_FILE variable is set (and does not care if it's empty, as
long as it is set), even for packages that were not enabled by the user.

This is dubious at best, and forces kconfig-based packages to always set
that variable, even to an empty value.

Furthermore, it does not even check if the variable is non-empty.

Fix that in two ways:
  - only do the check when the package is actually enabled;
  - change the check to test if the variable is non-empty (instead of
    merely set).

Regards,
Yann E. MORIN.


The following changes since commit 2552729232b4d9c5cb63ad247d6d94ab83e48e49:

  package/mplayer: add optional libenca support (2015-04-07 23:27:23 +0200)

are available in the git repository at:

  git://git.busybox.net/~ymorin/git/buildroot yem/misc

for you to fetch changes up to c5c81694cabca70c5651fe42a7de99247275659c:

  infra/pkg-kconfig: require an non-empty KCONFIG_FILE (2015-04-08 19:01:54 +0200)

----------------------------------------------------------------
Yann E. MORIN (2):
      infra/pkg-kconfig: don't require a kconfig file for disabled packages
      infra/pkg-kconfig: require an non-empty KCONFIG_FILE

 package/pkg-kconfig.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 1/2] infra/pkg-kconfig: don't require a kconfig file for disabled packages

[Yann E. MORIN]

When a package is not enabled, we need not check that it provides
a .config/defconfig file.

So far, all our pkg-kconfig packages unconditionally set their
_KCONFIG_FILE, even to an empty value. But some packages (e.g.
at91bootstrap3) wanted to set it only when enabled, and broke.

So, only do the check when the package is enabled.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
---
 package/pkg-kconfig.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index 04ac37d..fd9f19d 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -36,11 +36,6 @@ $(2)_KCONFIG_EDITORS ?= menuconfig
 $(2)_KCONFIG_OPTS ?=
 $(2)_KCONFIG_FIXUP_CMDS ?=
 
-# FOO_KCONFIG_FILE is required
-ifndef $(2)_KCONFIG_FILE
-$$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
-endif
-
 # The config file could be in-tree, so before depending on it the package should
 # be extracted (and patched) first
 $$($(2)_KCONFIG_FILE): | $(1)-patch
@@ -72,6 +67,11 @@ $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done
 # already called above, so we can effectively use this variable.
 ifeq ($$($$($(2)_KCONFIG_VAR)),y)
 
+# FOO_KCONFIG_FILE is required
+ifndef $(2)_KCONFIG_FILE
+$$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
+endif
+
 # Configuration editors (menuconfig, ...)
 $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $$($(2)_DIR)/.stamp_kconfig_fixup_done
 	$$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
-- 
1.9.1

[Buildroot] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Yann E. MORIN]

Currently, we only check that the variable is defined, which is not
enough since we really want it to be non-empty.

We however can't check it points to an existing file, because the
package might well not be extracted yet, and we may use an internal
defconfig. If that file does not eventually exist, there will be a
failure down the road at build time when we try to copy it...

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
---
 package/pkg-kconfig.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
index fd9f19d..fe8d266 100644
--- a/package/pkg-kconfig.mk
+++ b/package/pkg-kconfig.mk
@@ -68,7 +68,7 @@ $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done
 ifeq ($$($$($(2)_KCONFIG_VAR)),y)
 
 # FOO_KCONFIG_FILE is required
-ifndef $(2)_KCONFIG_FILE
+ifeq ($$($(2)_KCONFIG_FILE),)
 $$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
 endif
 
-- 
1.9.1

[Buildroot] [PATCH 1/2] infra/pkg-kconfig: don't require a kconfig file for disabled packages

[Arnout Vandecappelle]

On 08/04/15 19:08, Yann E. MORIN wrote:
> When a package is not enabled, we need not check that it provides
> a .config/defconfig file.
> 
> So far, all our pkg-kconfig packages unconditionally set their
> _KCONFIG_FILE, even to an empty value. But some packages (e.g.
> at91bootstrap3) wanted to set it only when enabled, and broke.
> 
> So, only do the check when the package is enabled.
> 
> Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout

> ---
>  package/pkg-kconfig.mk | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> index 04ac37d..fd9f19d 100644
> --- a/package/pkg-kconfig.mk
> +++ b/package/pkg-kconfig.mk
> @@ -36,11 +36,6 @@ $(2)_KCONFIG_EDITORS ?= menuconfig
>  $(2)_KCONFIG_OPTS ?=
>  $(2)_KCONFIG_FIXUP_CMDS ?=
>  
> -# FOO_KCONFIG_FILE is required
> -ifndef $(2)_KCONFIG_FILE
> -$$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
> -endif
> -
>  # The config file could be in-tree, so before depending on it the package should
>  # be extracted (and patched) first
>  $$($(2)_KCONFIG_FILE): | $(1)-patch
> @@ -72,6 +67,11 @@ $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done
>  # already called above, so we can effectively use this variable.
>  ifeq ($$($$($(2)_KCONFIG_VAR)),y)
>  
> +# FOO_KCONFIG_FILE is required
> +ifndef $(2)_KCONFIG_FILE
> +$$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
> +endif
> +
>  # Configuration editors (menuconfig, ...)
>  $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $$($(2)_DIR)/.stamp_kconfig_fixup_done
>  	$$($(2)_MAKE_ENV) $$(MAKE) -C $$($(2)_DIR) \
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Arnout Vandecappelle]

On 08/04/15 19:08, Yann E. MORIN wrote:
> Currently, we only check that the variable is defined, which is not
> enough since we really want it to be non-empty.

 Have you tested this? According to the make documentation, this is not true...

> 
> We however can't check it points to an existing file, because the
> package might well not be extracted yet, and we may use an internal
> defconfig. If that file does not eventually exist, there will be a
> failure down the road at build time when we try to copy it...
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> ---
>  package/pkg-kconfig.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> index fd9f19d..fe8d266 100644
> --- a/package/pkg-kconfig.mk
> +++ b/package/pkg-kconfig.mk
> @@ -68,7 +68,7 @@ $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done
>  ifeq ($$($$($(2)_KCONFIG_VAR)),y)
>  
>  # FOO_KCONFIG_FILE is required
> -ifndef $(2)_KCONFIG_FILE
> +ifeq ($$($(2)_KCONFIG_FILE),)

 These two are exactly the same, cfr. [1]:

>      The 'ifdef' form takes the _name_ of a variable as its argument,
>      not a reference to a variable.  The value of that variable has a
>      non-empty value, the TEXT-IF-TRUE is effective; otherwise, the
       ^^^^^^^^^^^^^^^
>      TEXT-IF-FALSE, if any, is effective.  Variables that have never
>      been defined have an empty value.  The text VARIABLE-NAME is
>      expanded, so it could be a variable or function that expands to the
       ^^^^^^^^
>      name of a variable.



 Regards,
 Arnout

[1] https://www.gnu.org/software/make/manual/html_node/Conditional-Syntax.html

>  $$(error Internal error: no value specified for $(2)_KCONFIG_FILE)
>  endif
>  
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 1/2] infra/pkg-kconfig: don't require a kconfig file for disabled packages

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Wed,  8 Apr 2015 19:08:34 +0200, Yann E. MORIN wrote:
> When a package is not enabled, we need not check that it provides
> a .config/defconfig file.
> 
> So far, all our pkg-kconfig packages unconditionally set their
> _KCONFIG_FILE, even to an empty value. But some packages (e.g.
> at91bootstrap3) wanted to set it only when enabled, and broke.
> 
> So, only do the check when the package is enabled.
> 
> Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

Applied, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Yann E. MORIN]

Arnout, All,

On 2015-04-08 22:22 +0200, Arnout Vandecappelle spake thusly:
> On 08/04/15 19:08, Yann E. MORIN wrote:
> > Currently, we only check that the variable is defined, which is not
> > enough since we really want it to be non-empty.
> 
>  Have you tested this? According to the make documentation, this is not true...
> 
> > 
> > We however can't check it points to an existing file, because the
> > package might well not be extracted yet, and we may use an internal
> > defconfig. If that file does not eventually exist, there will be a
> > failure down the road at build time when we try to copy it...
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> > ---
> >  package/pkg-kconfig.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/package/pkg-kconfig.mk b/package/pkg-kconfig.mk
> > index fd9f19d..fe8d266 100644
> > --- a/package/pkg-kconfig.mk
> > +++ b/package/pkg-kconfig.mk
> > @@ -68,7 +68,7 @@ $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done
> >  ifeq ($$($$($(2)_KCONFIG_VAR)),y)
> >  
> >  # FOO_KCONFIG_FILE is required
> > -ifndef $(2)_KCONFIG_FILE
> > +ifeq ($$($(2)_KCONFIG_FILE),)
> 
>  These two are exactly the same, cfr. [1]:
> 
> >      The 'ifdef' form takes the _name_ of a variable as its argument,
> >      not a reference to a variable.  The value of that variable has a
> >      non-empty value, the TEXT-IF-TRUE is effective; otherwise, the
>        ^^^^^^^^^^^^^^^
> >      TEXT-IF-FALSE, if any, is effective.  Variables that have never
> >      been defined have an empty value.  The text VARIABLE-NAME is
> >      expanded, so it could be a variable or function that expands to the
>        ^^^^^^^^
> >      name of a variable.

Weird, because we do have, for example when the linux kernel is not
enabled:

  171 ifeq ($(BR2_LINUX_KERNEL_USE_DEFCONFIG),y)
  172 KERNEL_SOURCE_CONFIG = $(KERNEL_ARCH_PATH)/configs/$(call qstrip,$(BR2_LINUX_KERNEL_DEFCONFIG))_defconfig
  173 else ifeq ($(BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG),y)
  174 KERNEL_SOURCE_CONFIG = $(call qstrip,$(BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE))
  175 endif
  176 
  177 LINUX_KCONFIG_FILE = $(KERNEL_SOURCE_CONFIG)

So, because neither "use defconfig" nor "use custom config" is set,
KERNEL_SOURCE_CONFIG is not set, so LINUX_KCONFIG_FILE ends up empty.

However, our ifndef did not trigger so far.

Maybe it is because there is a space between the '=' sign and the
expansion of KERNEL_SOURCE_CONFIG...

Well, I don't care that much. ;-] So I'll mark this patch rejected.

Thanks! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Arnout Vandecappelle]

On 08/04/15 22:49, Yann E. MORIN wrote:
[snip]
> So, because neither "use defconfig" nor "use custom config" is set,
> KERNEL_SOURCE_CONFIG is not set, so LINUX_KCONFIG_FILE ends up empty.


BR2_LINUX_KERNEL_USE_DEFCONFIG and BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG are the
only two possibilities in the "Kernel configuration" choice, so one of them will
always be set.

> However, our ifndef did not trigger so far.

 The kernel isn't built in the autobuilders, so where would it trigger?

 Regards,
 Arnout

-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Yann E. MORIN]

Arnout, All,

On 2015-04-10 22:01 +0200, Arnout Vandecappelle spake thusly:
> On 08/04/15 22:49, Yann E. MORIN wrote:
> [snip]
> > So, because neither "use defconfig" nor "use custom config" is set,
> > KERNEL_SOURCE_CONFIG is not set, so LINUX_KCONFIG_FILE ends up empty.
> 
> 
> BR2_LINUX_KERNEL_USE_DEFCONFIG and BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG are the
> only two possibilities in the "Kernel configuration" choice, so one of them will
> always be set.

No, neither would be set if you don't enable building a kernel. In that
case, LINUX_KCONFIG_FILE is empty, but the $(eval $(kconfig-package)) is
still called, and thus the test should fail. But it does not.

> > However, our ifndef did not trigger so far.
> 
>  The kernel isn't built in the autobuilders, so where would it trigger?

Well, obviously, locally. ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Arnout Vandecappelle]

On 10/04/15 22:40, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2015-04-10 22:01 +0200, Arnout Vandecappelle spake thusly:
>> On 08/04/15 22:49, Yann E. MORIN wrote:
>> [snip]
>>> So, because neither "use defconfig" nor "use custom config" is set,
>>> KERNEL_SOURCE_CONFIG is not set, so LINUX_KCONFIG_FILE ends up empty.
>>
>>
>> BR2_LINUX_KERNEL_USE_DEFCONFIG and BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG are the
>> only two possibilities in the "Kernel configuration" choice, so one of them will
>> always be set.
> 
> No, neither would be set if you don't enable building a kernel. In that
> case, LINUX_KCONFIG_FILE is empty, but the $(eval $(kconfig-package)) is
> still called, and thus the test should fail. But it does not.

 Argh, stupid me... The documentation I pointed to explicitly says a few lines
lower:

Note that ifdef only tests whether a variable has a value. It does not expand
the variable to see if that value is nonempty.


 Therefore, I've un-rejected this patch, and I'm adding my

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>


 Regards,
 Arnout

> 
>>> However, our ifndef did not trigger so far.
>>
>>  The kernel isn't built in the autobuilders, so where would it trigger?
> 
> Well, obviously, locally. ;-)
> 
> Regards,
> Yann E. MORIN.
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 2/2] infra/pkg-kconfig: require an non-empty KCONFIG_FILE

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Wed,  8 Apr 2015 19:08:35 +0200, Yann E. MORIN wrote:
> Currently, we only check that the variable is defined, which is not
> enough since we really want it to be non-empty.
> 
> We however can't check it points to an existing file, because the
> package might well not be extracted yet, and we may use an internal
> defconfig. If that file does not eventually exist, there will be a
> failure down the road at build time when we try to copy it...
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> ---
>  package/pkg-kconfig.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com