* [Buildroot] [PATCHv3] pkg-generic: detect incorrectly used package
@ 2015-08-31 21:55 Thomas Petazzoni
2015-08-31 22:23 ` Yann E. MORIN
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Petazzoni @ 2015-08-31 21:55 UTC (permalink / raw)
To: buildroot
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 <thomas.petazzoni@free-electrons.com>
---
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(+)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 6a7d97e..e8cf488 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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)
--
2.5.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Buildroot] [PATCHv3] pkg-generic: detect incorrectly used package
2015-08-31 21:55 [Buildroot] [PATCHv3] pkg-generic: detect incorrectly used package Thomas Petazzoni
@ 2015-08-31 22:23 ` Yann E. MORIN
2015-08-31 22:32 ` Yann E. MORIN
0 siblings, 1 reply; 3+ messages in thread
From: Yann E. MORIN @ 2015-08-31 22:23 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2015-08-31 23:55 +0200, Thomas Petazzoni spake thusly:
> 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--]
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 6a7d97e..e8cf488 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -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 ; \
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.
> + 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)
> --
> 2.5.1
>
> _______________________________________________
> 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] 3+ messages in thread* [Buildroot] [PATCHv3] pkg-generic: detect incorrectly used package
2015-08-31 22:23 ` Yann E. MORIN
@ 2015-08-31 22:32 ` Yann E. MORIN
0 siblings, 0 replies; 3+ messages in thread
From: Yann E. MORIN @ 2015-08-31 22:32 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2015-09-01 00:23 +0200, Yann E. MORIN spake thusly:
> On 2015-08-31 23:55 +0200, Thomas Petazzoni spake thusly:
> > 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--]
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 6a7d97e..e8cf488 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -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 ; \
>
> 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 \
Woops... $($(PKG)_DEPENDENT_OF) of course, sicne we don;t have $(2) at
that point in the Makefile...
Regards,
Yann E. MORIN.
> echo " - ${p}"; \
> done
>
> Thoughts?
>
> I'll further review that patch tomorrow...
>
> Regards,
> Yann E. MORIN.
>
> > + 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)
> > --
> > 2.5.1
> >
> > _______________________________________________
> > 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. |
> '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> 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] 3+ messages in thread
end of thread, other threads:[~2015-08-31 22:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-31 21:55 [Buildroot] [PATCHv3] pkg-generic: detect incorrectly used package Thomas Petazzoni
2015-08-31 22:23 ` Yann E. MORIN
2015-08-31 22:32 ` Yann E. MORIN
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox