pkg-generic: detect incorrectly used package

PATCH Superseded Thomas Petazzoni 2 participants 3 messages Aug 31, 2015

Share & tools

Thomas Petazzoni

author

2015-08-31 21:55 UTC #0 lore raw

PATCH v3 1/1 pkg-generic: detect incorrectly used package

In Buildroot, the selection of a package from a Config.in level and from a Makefile level are completely disconnected. This can lead to issues where the build of a package is triggered at the Makefile level due to the package being listed in another package <pkg>_DEPENDENCIES variable, even if that package is not enabled in the configuration.

This has for example been the case recently with python-can having 'python' in its <pkg>_DEPENDENCIES, while python-can could be enabled when Python 3.x is used, in which case the 'python' package should not be built.

To detect such issues more easily, this patch adds a check in the package infrastructure. When the build process of a package is being triggered, we verify that the package is enabled in the configuration. We do this check in the "configure" step, since this step is the first common step between the normal download case and the "local site method" / "package override" case.

This requires passing two new target variables to the configure target:

- TYPE, which is either host or target. This is needed since the test should only be done on target packages (most host packages don't have a Config.in option)

- KCONFIG_VAR, which is the name of the Config.in variable corresponding to the package being built. For most packages, it's BR2_PACKAGE_<pkg>, but not for toolchain packages, bootloaders or linux.

Signed-off-by Thomas Petazzoni <[email protected]>

---

Changes since v2:
- Only do the check if MAKECMDGOALS is empty, i.e if a "default" build is being done. This allows advanced users to continue doing "make <pkg>" to forcefully build a package even if not enabled in the configuration. Suggested by Peter Korsgaard.
- Add @ in front of the test command so that it doesn't get displayed. Suggested by Peter Korsgaard.
- Improve error message, as suggested by Peter Korsgaard.

Changes since v1:
- Use KCONFIG_VAR in order to make the thing work for toolchain packages, bootloaders and Linux. Issue reported by Vicente.

---
 package/pkg-generic.mk | 13 +++++++++++++
 1 file changed, 13 insertions(+)

package/pkg-generic.mk +13 -0

index 6a7d97e..e8cf488 100644
@@ -143,6 +143,17 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
 
 # Configure
 $(BUILD_DIR)/%/.stamp_configured:
+# Only trigger the check for default builds. If the user forces
+# building a package, even if not enabled in the configuration, we
+# want to accept it.
+ifeq ($(MAKECMDGOALS),)
+	@if test "$(TYPE)" = "target" -a -z "$($(KCONFIG_VAR))" ; then \
+		echo "ERROR: A package must have added $(PKG) to its _DEPENDENCIES line but" ; \
+		echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \
+		echo "Please review your packages and try again." ; \
+		exit 1 ; \
+	fi
+endif
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))

@@ -664,6 +675,8 @@ $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
 $$($(2)_TARGET_INSTALL_HOST):           PKG=$(2)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		TYPE=$(4)
+$$($(2)_TARGET_CONFIGURE):		KCONFIG_VAR=$$($(2)_KCONFIG_VAR)
 $$($(2)_TARGET_RSYNC):                  SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):                  PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)

Signature · 1 line

2.5.1

Yann E. MORIN

2015-08-31 22:23 UTC #1 lore raw

PATCH v3 1/1 pkg-generic: detect incorrectly used package

Thomas, All,

On 2015-08-31 23:55 +0200, Thomas Petazzoni spake thusly:

Quoted 5 lines

In Buildroot, the selection of a package from a Config.in level and from a Makefile level are completely disconnected. This can lead to issues where the build of a package is triggered at the Makefile level due to the package being listed in another package <pkg>_DEPENDENCIES variable, even if that package is not enabled in the configuration.

[--SNIP--]

Quoted 7 lines

package/pkg-generic.mk +0 -0

index 6a7d97e..e8cf488 100644
@@ -143,6 +143,17 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
 
 # Configure

package/pkg-generic.mk $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\ · line 154

Quoted 10 lines

package/pkg-generic.mk +9 -0

 $(BUILD_DIR)/%/.stamp_configured:
+# Only trigger the check for default builds. If the user forces
+# building a package, even if not enabled in the configuration, we
+# want to accept it.
+ifeq ($(MAKECMDGOALS),)
+	@if test "$(TYPE)" = "target" -a -z "$($(KCONFIG_VAR))" ; then \
+		echo "ERROR: A package must have added $(PKG) to its _DEPENDENCIES line but" ; \
+		echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \
+		echo "Please review your packages and try again." ; \
+		exit 1 ; \

It might even be interesting to tell the user what those packages might be, like so (totally untested):

    diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
    index 6a7d97e..03ca9cc 100644
    --- a/package/pkg-generic.mk
    +++ b/package/pkg-generic.mk
    @@ -758,6 +758,9 @@ endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
     # configuration
     ifeq ($$($$($(2)_KCONFIG_VAR)),y)
     
    +# Store reverse build-dependency information
    +$$(eval $$(foreach d,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(call UPPER,$$(d))_DEPENDENT_OF += $(1)$$(sep)))
    +
     # Ensure the calling package is the declared provider for all the virtual
     # packages it claims to be an implementation of.
     ifneq ($$($(2)_PROVIDES),)

and then, in this new error message (wording to be adapted):

    echo "Potential culprits are:"; \
    for p in $($(2)_DEPENDENT_OF); do \
        echo "  - ${p}"; \
    done

Thoughts?

I'll further review that patch tomorrow...

Regards,
Yann E. MORIN.

Quoted 21 lines

package/pkg-generic.mk +4 -0

+	fi
+endif
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))

@@ -664,6 +675,8 @@ $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
 $$($(2)_TARGET_INSTALL_HOST):           PKG=$(2)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		TYPE=$(4)
+$$($(2)_TARGET_CONFIGURE):		KCONFIG_VAR=$$($(2)_KCONFIG_VAR)
 $$($(2)_TARGET_RSYNC):                  SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):                  PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)

Signature · 6 lines

2.5.1

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

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

Yann E. MORIN

2015-08-31 22:32 UTC #2 lore raw

PATCH v3 1/1 pkg-generic: detect incorrectly used package

Thomas, All,

On 2015-09-01 00:23 +0200, Yann E. MORIN spake thusly:

Quoted 43 lines

On 2015-08-31 23:55 +0200, Thomas Petazzoni spake thusly:
Quoted 5 lines
In Buildroot, the selection of a package from a Config.in level and from a Makefile level are completely disconnected. This can lead to issues where the build of a package is triggered at the Makefile level due to the package being listed in another package <pkg>_DEPENDENCIES variable, even if that package is not enabled in the configuration.
[--SNIP--]
Quoted 7 lines
package/pkg-generic.mk +0 -0
index 6a7d97e..e8cf488 100644
@@ -143,6 +143,17 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
 
 # Configure
package/pkg-generic.mk $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\ · line 154
Quoted 10 lines
package/pkg-generic.mk +9 -0
 $(BUILD_DIR)/%/.stamp_configured:
+# Only trigger the check for default builds. If the user forces
+# building a package, even if not enabled in the configuration, we
+# want to accept it.
+ifeq ($(MAKECMDGOALS),)
+	@if test "$(TYPE)" = "target" -a -z "$($(KCONFIG_VAR))" ; then \
+		echo "ERROR: A package must have added $(PKG) to its _DEPENDENCIES line but" ; \
+		echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \
+		echo "Please review your packages and try again." ; \
+		exit 1 ; \
It might even be interesting to tell the user what those packages might be, like so (totally untested):
    diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
    index 6a7d97e..03ca9cc 100644
    --- a/package/pkg-generic.mk
    +++ b/package/pkg-generic.mk
    @@ -758,6 +758,9 @@ endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
     # configuration
     ifeq ($$($$($(2)_KCONFIG_VAR)),y)
     
    +# Store reverse build-dependency information
    +$$(eval $$(foreach d,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(call UPPER,$$(d))_DEPENDENT_OF += $(1)$$(sep)))
    +
     # Ensure the calling package is the declared provider for all the virtual
     # packages it claims to be an implementation of.
     ifneq ($$($(2)_PROVIDES),)

Quoted 4 lines

and then, in this new error message (wording to be adapted):
    echo "Potential culprits are:"; \
    for p in $($(2)_DEPENDENT_OF); do \

Woops... $($(PKG)_DEPENDENT_OF) of course, sicne we don;t have $(2) at that point in the Makefile...

Regards,
Yann E. MORIN.

Quoted 43 lines

        echo "  - ${p}"; \
    done

Thoughts?

I'll further review that patch tomorrow...

Regards,
Yann E. MORIN.

Quoted 21 lines

package/pkg-generic.mk +4 -0

+	fi
+endif
 	@$(call step_start,configure)
 	@$(call MESSAGE,"Configuring")
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))

@@ -664,6 +675,8 @@ $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
 $$($(2)_TARGET_INSTALL_HOST):           PKG=$(2)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		TYPE=$(4)
+$$($(2)_TARGET_CONFIGURE):		KCONFIG_VAR=$$($(2)_KCONFIG_VAR)
 $$($(2)_TARGET_RSYNC):                  SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):                  PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)

Signature · 6 lines

2.5.1

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Signature · 10 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.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

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