| Message ID | 1441141898-26885-1-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas, All, On 2015-09-01 23:11 +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--] > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Small nit below (but already discussed on IRC), otherwise: Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 6a7d97e..596c798 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -143,6 +143,18 @@ $(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)_NAME) to its _DEPENDENCIES line but" ; \ > + echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \ > + echo "Potential culprits: " ; \ Extraneous space after the ':'. Regards, Yann E. MORIN. > + for p in $($(PKG)_DEPENDENT_OF) ; do echo " - $$p" ; done ; \ > + exit 1 ; \ > + fi > +endif > @$(call step_start,configure) > @$(call MESSAGE,"Configuring") > $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) > @@ -664,6 +676,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) > @@ -758,6 +772,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 UPPERCASE,$$(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),) > -- > 2.5.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 01-09-15 23:11, Thomas Petazzoni wrote: > 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. I would actually prefer checks like this to de done by a separate checkpackage script rather than as part of the package infrastructure. This is IMHO more in line with the buildroot is simple principle. However, we don't have a checkpackage script yet - I started on one a while ago but it still has a ways to go. And anyway, it is probably pretty complicated to check this in a checkpackage script. Therefore, let's include this feature. > 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) You can just use $($(PKG)_TYPE). > > - 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. You can just use $($(PKG)_KCONFIG_VAR). > > In addition to displaying an error, we try to help the user by saying > which packages could be the culprit. To achieve this, we register the > reverse dependencies of each package in a variable called > <pkg>_DEPENDENT_OF, and display this variable for the problematic > package when the error is detected. Many thanks to Yann E. Morin for > the idea and implementation! > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > Changes since v3: > - Add the DEPENDENT_OF mechanism to display which packages are > mistakenly depending on a package without selecting it. Suggested > and implement by Yann E. Morin. > > 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 | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 6a7d97e..596c798 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -143,6 +143,18 @@ $(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)_NAME) to its _DEPENDENCIES line but" ; \ > + echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \ > + echo "Potential culprits: " ; \ > + for p in $($(PKG)_DEPENDENT_OF) ; do echo " - $$p" ; done ; \ I don't really like adding that extra _DEPENDENT_OF variable just for this purpose. But it will be hard to avoid I guess. It _could_ be avoided by looping over $(.VARIABLES), then selecting the ones that match the _DEPENDENCIES pattern and that have $(call lowercase,$(PKG)) in them. But that's a bit complicated :-) > + exit 1 ; \ > + fi > +endif > @$(call step_start,configure) > @$(call MESSAGE,"Configuring") > $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) > @@ -664,6 +676,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) > @@ -758,6 +772,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 UPPERCASE,$$(d))_DEPENDENT_OF += $(1)$$(sep))) Can you split this over two lines? Or even three? $$(eval $$(foreach d,$$($(2)_FINAL_ALL_DEPENDENCIES),\ $$(call UPPERCASE,$$(d))_DEPENDENT_OF += $(1)$$(sep)\ )) or perhaps even define a helper. Regards, Arnout > + > # Ensure the calling package is the declared provider for all the virtual > # packages it claims to be an implementation of. > ifneq ($$($(2)_PROVIDES),) >
Arnout, On Sat, 5 Sep 2015 11:07:27 +0200, Arnout Vandecappelle wrote: > > 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. > > I would actually prefer checks like this to de done by a separate checkpackage > script rather than as part of the package infrastructure. This is IMHO more in > line with the buildroot is simple principle. > > However, we don't have a checkpackage script yet - I started on one a while ago > but it still has a ways to go. And anyway, it is probably pretty complicated to > check this in a checkpackage script. Therefore, let's include this feature. I agree on principle, but I also believe doing such a check in a separate script is going to be quite tricky. > > 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) > > You can just use $($(PKG)_TYPE). > > > > > - 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. > > You can just use $($(PKG)_KCONFIG_VAR). I'll try to do that, it seems like a good idea indeed. > > # 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)_NAME) to its _DEPENDENCIES line but" ; \ > > + echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \ > > + echo "Potential culprits: " ; \ > > + for p in $($(PKG)_DEPENDENT_OF) ; do echo " - $$p" ; done ; \ > > I don't really like adding that extra _DEPENDENT_OF variable just for this > purpose. But it will be hard to avoid I guess. It _could_ be avoided by looping > over $(.VARIABLES), then selecting the ones that match the _DEPENDENCIES pattern > and that have $(call lowercase,$(PKG)) in them. But that's a bit complicated :-) I am also not sure adding this additional variable just for this purpose is really needed. My initial proposal did not have this, and was just saying "some package has a problem". Yann proposed this improvement to point to potential offenders, making the investigation easier. So it's up to us to decide whether we want to make that investigation easier (which requires this additional variable), or keep the code simpler and remove one variable, but make the investigation of such errors a bit more complicated. I don't have a very strong feeling on this. It's just one more variable, computed in a reasonably simple way. > > +# Store reverse build-dependency information > > +$$(eval $$(foreach d,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(call UPPERCASE,$$(d))_DEPENDENT_OF += $(1)$$(sep))) > > Can you split this over two lines? Or even three? > $$(eval $$(foreach d,$$($(2)_FINAL_ALL_DEPENDENCIES),\ > $$(call UPPERCASE,$$(d))_DEPENDENT_OF += $(1)$$(sep)\ > )) > > or perhaps even define a helper. Will see what I can do. Thanks! Thomas
Thomas, Arnout, All, On 2015-09-05 11:43 +0200, Thomas Petazzoni spake thusly: > On Sat, 5 Sep 2015 11:07:27 +0200, Arnout Vandecappelle wrote: > > > 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. > > > > I would actually prefer checks like this to de done by a separate checkpackage > > script rather than as part of the package infrastructure. This is IMHO more in > > line with the buildroot is simple principle. > > > > However, we don't have a checkpackage script yet - I started on one a while ago > > but it still has a ways to go. And anyway, it is probably pretty complicated to > > check this in a checkpackage script. Therefore, let's include this feature. > > I agree on principle, but I also believe doing such a check in a > separate script is going to be quite tricky. Well, I believe it *will* be quite tricky. If we do it in a check-package script, then we'll have to handle quite some special cases. For example, how can we differentiate those cases: - Config.in does not select/depend, but .mk has it unconditionally in the _DEPENDENCIES; - Config.in does not select/depend, but .mk has it conditionally in the _DEPEDNENCIES (i.e. if foo enabled, add it to dependencies); - Config.in conditionally selects/depends, and .mk has it unconditionally in the _DEPENDENCIES; - Config.in conditionally selects/depends, and .mk has it conditionally in the _DEPENDENCIES. Short of writing an actual Makefile parser (Kconfiglib already exists for the Kconfig part) and writing a "dependency solver" (i.e. being able to test all condition-paths), I don;t see how we can do a good job with a check-package script... So, checkign at build-time is not the optimum (especially since the error may happen quite late during the build), but that's IMHO the best we can hope for without investing a huge amount of work. > > > # 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)_NAME) to its _DEPENDENCIES line but" ; \ > > > + echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \ > > > + echo "Potential culprits: " ; \ > > > + for p in $($(PKG)_DEPENDENT_OF) ; do echo " - $$p" ; done ; \ > > > > I don't really like adding that extra _DEPENDENT_OF variable just for this > > purpose. But it will be hard to avoid I guess. It _could_ be avoided by looping > > over $(.VARIABLES), then selecting the ones that match the _DEPENDENCIES pattern > > and that have $(call lowercase,$(PKG)) in them. But that's a bit complicated :-) Yes, that's a bit compicated. And I'm afraid looping over .VARIABLES would take quite some time: "make printvars" takes quite some time (especially for large configurations), and is doing almost the same as what you suggest. But that's an error-path, so we may not care completely... Yet, this new variable could be usefull for other stuff. We currently have "make PKG-show-depends" to see what the dependencies are for that package. We could also add "make PKG-show-dependees" to show the reverse dependencies for that package. I can't really think what the final use-case would be for that, but I believe it would be useful on its own. > I am also not sure adding this additional variable just for this > purpose is really needed. My initial proposal did not have this, and > was just saying "some package has a problem". Yann proposed this > improvement to point to potential offenders, making the investigation > easier. So it's up to us to decide whether we want to make that > investigation easier (which requires this additional variable), or keep > the code simpler and remove one variable, but make the investigation of > such errors a bit more complicated. > > I don't have a very strong feeling on this. It's just one more > variable, computed in a reasonably simple way. Well, you do split the patch in two: you original submission (with the comments addressed, of course) followed by an additional patch that introduces the culprits. That way, if that second part (my sugggestion) is too controversial, we can still benefit for the sanity-check you proposed, which is still a very good thing! ;-) Regards, Yann E. MORIN.
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 6a7d97e..596c798 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -143,6 +143,18 @@ $(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)_NAME) to its _DEPENDENCIES line but" ; \ + echo "forgot to add the corresponding select / depends on $(KCONFIG_VAR)." ; \ + echo "Potential culprits: " ; \ + for p in $($(PKG)_DEPENDENT_OF) ; do echo " - $$p" ; done ; \ + exit 1 ; \ + fi +endif @$(call step_start,configure) @$(call MESSAGE,"Configuring") $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) @@ -664,6 +676,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) @@ -758,6 +772,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 UPPERCASE,$$(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),)
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. In addition to displaying an error, we try to help the user by saying which packages could be the culprit. To achieve this, we register the reverse dependencies of each package in a variable called <pkg>_DEPENDENT_OF, and display this variable for the problematic package when the error is detected. Many thanks to Yann E. Morin for the idea and implementation! Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- Changes since v3: - Add the DEPENDENT_OF mechanism to display which packages are mistakenly depending on a package without selecting it. Suggested and implement by Yann E. Morin. 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 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)