Skip to contentSkip to conversation

pkg-kconfig: fix checking .config file (branch yem/misc)

PATCH New Yann E. MORIN 3 participants 11 messages Apr 8, 2015 – Jun 10, 2015

lore
Share & tools
lore

Yann E. MORIN

author
#0 lore raw

cover pkg-kconfig: fix checking .config file (branch yem/misc)

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(-)
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
author
#1 lore raw

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

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 <[email protected]>
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
---
 package/pkg-kconfig.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
package/pkg-kconfig.mk +5 -5
index 04ac37d..fd9f19d 100644
@@ -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) \
Signature · 1 line
1.9.1
#2 lore raw

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

Quoted 7 lines · 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.

Quoted 6 lines

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

Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Regards,
Arnout

Quoted 33 lines
---
 package/pkg-kconfig.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
package/pkg-kconfig.mk +5 -5
index 04ac37d..fd9f19d 100644
@@ -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) \
Signature · 6 lines
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
Applied
#3 lore raw

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

In reply to Yann E. MORIN

Dear Yann E. MORIN,

Quoted 7 lines · 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.

Quoted 6 lines

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

Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>

Applied, thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#4 lore raw

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

In reply to 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" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
---
 package/pkg-kconfig.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
package/pkg-kconfig.mk +1 -1
index fd9f19d..fe8d266 100644
@@ -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
Signature · 1 line
1.9.1
#5 lore raw

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

Quoted 2 lines · 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.

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

Quoted 18 lines

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" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
---
 package/pkg-kconfig.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
package/pkg-kconfig.mk +0 -0
index fd9f19d..fe8d266 100644

package/pkg-kconfig.mk $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done · line 71

Quoted 5 lines
package/pkg-kconfig.mk +1 -1
@@ -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]:

package/pkg-kconfig.mk $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done · line 74

Quoted 3 lines
package/pkg-kconfig.mk +0 -0
     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

       ^^^^^^^^^^^^^^^
Quoted 3 lines

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

       ^^^^^^^^
Quoted 1 line

name of a variable.

Regards,
Arnout

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

Quoted 4 lines

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

Signature · 6 lines
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
author
#6 lore raw

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

Arnout, All,

On 2015-04-08 22:22 +0200, Arnout Vandecappelle spake thusly:

Quoted 32 lines · Yann E. MORIN
Quoted 2 lines

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...

Quoted 18 lines

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" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
---
 package/pkg-kconfig.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
package/pkg-kconfig.mk +0 -0
index fd9f19d..fe8d266 100644

package/pkg-kconfig.mk $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done · line 71

Quoted 5 lines
package/pkg-kconfig.mk +1 -1
@@ -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]:

Quoted 9 lines

package/pkg-kconfig.mk $$($(2)_TARGET_CONFIGURE): $$($(2)_DIR)/.stamp_kconfig_fixup_done · line 74

Quoted 3 lines
package/pkg-kconfig.mk +0 -0
     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

       ^^^^^^^^^^^^^^^
Quoted 3 lines

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

       ^^^^^^^^
Quoted 1 line

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.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#7 lore raw

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

On 08/04/15 22:49, Yann E. MORIN wrote:
[snip]

Quoted 2 lines

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.

Quoted 1 line

However, our ifndef did not trigger so far.

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

Regards,
Arnout

Signature · 6 lines
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
author
#8 lore raw

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

Arnout, All,

On 2015-04-10 22:01 +0200, Arnout Vandecappelle spake thusly:

Quoted 9 lines · Yann E. MORIN

[snip]

Quoted 2 lines

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.

Quoted 3 lines
Quoted 1 line

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.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#9 lore raw

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

Quoted 9 lines · Yann E. MORIN

Arnout, All,

On 2015-04-10 22:01 +0200, Arnout Vandecappelle spake thusly:

Quoted 6 lines · Yann E. MORIN

[snip]

Quoted 2 lines

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

Quoted 7 lines
Quoted 3 lines

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) <[email protected]>

Regards,
Arnout

Quoted 10 lines
Quoted 4 lines
Quoted 2 lines

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.

Signature · 6 lines
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
Applied
#10 lore raw

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

In reply to Yann E. MORIN

Dear Yann E. MORIN,

Quoted 8 lines · 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...

Quoted 6 lines
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc Thomas De Schampheleire <[email protected]>
---
 package/pkg-kconfig.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thomas

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