[Buildroot] [PATCH] pkg-generic: fix rdepends and phony targets of virtual packages

[Arnout Vandecappelle]

On 16/03/2022 11:12, Anssi Hannula wrote:
> Virtual packages are not added to the _RDEPENDENCIES list of packages
> that they depend on (i.e. their provider).
> 
> This causes <provider>-show-rdepends to not show the virtual package
> and <provider>-show-recursive-rdepends to miss all the packages that
> transitively depend on <provider> via the virtual package.
> 
> The virtual make targets (e.g. <pkg>-show-info) are also not marked as
> phony for virtual packages.
> 
> To fix those issues, remove most of the special handling of virtual
> packages in pkg-generic by making $($($(1)_KCONFIG_VAR))=y for them as
> well.
> 
> This also allows removal of some duplicated code in pkg-generic.mk and a
> now unneeded special condition in CHECK_ONE_DEPENDENCY.
> 
> Still keep the virtual package out of PACKAGES since there is e.g. no
> need to rsync per-package target dir to global target dir. I am not
> aware of any showstoppers preventing addition to PACKAGES as well,
> though, so it is probably just an optimization.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula at bitwise.fi>
> ---
> 
> I encountered this while using <pkg>-show-recursive-rdepends to
> determine which packages to dirclean after modification of a
> (non-virtual) <pkg> in per-package mode.
> 
> The current virtual vs. real distinction didn't seem worth the risk of

  Well, I guess there *was* a reason that we introduced this distinction way 
back when, and that all the build targets are skipped for virtual packages. 
However, I can't remember the reason. Maybe Yann (the instigator of this 
feature) still remembers?

  Looking at the history, in particular e.g. commit c5d546450fc1 that converts 
libgles from generic infrastructure to virtual, it looks like it really *should* 
be OK to keep the full build targets. And in that case, this patch is a really 
nice simplification!

  So, unless Yann objects, I'd say: let's try it and let the autobuilders complain!

  Regards,
  Arnout

> unintentional differences so I dropped it, but I'm not too familiar with
> the code so I could have missed something.
> In case I did and this is not a good idea, one less intrusive option
> would be to split the common-to-virtual-and-real-packages code to be
> under a new separate conditional block.
> 
> 
>   Makefile               |  2 --
>   package/pkg-generic.mk | 24 +++++-------------------
>   2 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 4f693d40a9..9e875dd8d8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -565,14 +565,12 @@ ifeq ($(BR_FORCE_CHECK_DEPENDENCIES),YES)
>   
>   define CHECK_ONE_DEPENDENCY
>   ifeq ($$($(2)_TYPE),target)
> -ifeq ($$($(2)_IS_VIRTUAL),)
>   ifneq ($$($$($(2)_KCONFIG_VAR)),y)
>   $$(error $$($(2)_NAME) is in the dependency chain of $$($(1)_NAME) that \
>   has added it to its _DEPENDENCIES variable without selecting it or \
>   depending on it from Config.in)
>   endif
>   endif
> -endif
>   endef
>   
>   $(foreach pkg,$(call UPPERCASE,$(PACKAGES)),\
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index b3a7e1d60e..a0d6198edc 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -1064,15 +1064,15 @@ $$($(2)_TARGET_DIRCLEAN):		PKG=$(2)
>   $$($(2)_TARGET_DIRCLEAN):		NAME=$(1)
>   
>   # Compute the name of the Kconfig option that correspond to the
> -# package being enabled. We handle three cases: the special Linux
> -# kernel case, the bootloaders case, and the normal packages case.
> -# Virtual packages are handled separately (see below).
> +# package being enabled.
>   ifeq ($(1),linux)
>   $(2)_KCONFIG_VAR = BR2_LINUX_KERNEL
>   else ifneq ($$(filter boot/% $$(foreach dir,$$(BR2_EXTERNAL_DIRS),$$(dir)/boot/%),$(pkgdir)),)
>   $(2)_KCONFIG_VAR = BR2_TARGET_$(2)
>   else ifneq ($$(filter toolchain/% $$(foreach dir,$$(BR2_EXTERNAL_DIRS),$$(dir)/toolchain/%),$(pkgdir)),)
>   $(2)_KCONFIG_VAR = BR2_$(2)
> +else ifeq ($$($(2)_IS_VIRTUAL),YES)
> +$(2)_KCONFIG_VAR = BR2_PACKAGE_HAS_$(2)
>   else
>   $(2)_KCONFIG_VAR = BR2_PACKAGE_$(2)
>   endif
> @@ -1178,7 +1178,9 @@ $(eval $(call check-deprecated-variable,$(2)_BUILD_OPT,$(2)_BUILD_OPTS))
>   $(eval $(call check-deprecated-variable,$(2)_GETTEXTIZE_OPT,$(2)_GETTEXTIZE_OPTS))
>   $(eval $(call check-deprecated-variable,$(2)_KCONFIG_OPT,$(2)_KCONFIG_OPTS))
>   
> +ifneq ($$($(2)_IS_VIRTUAL),YES)
>   PACKAGES += $(1)
> +endif
>   
>   ifneq ($$($(2)_PERMISSIONS),)
>   PACKAGES_PERMISSIONS_TABLE += $$($(2)_PERMISSIONS)$$(sep)
> @@ -1268,22 +1270,6 @@ ifneq ($$($(2)_HELP_CMDS),)
>   HELP_PACKAGES += $(2)
>   endif
>   
> -# Virtual packages are not built but it's useful to allow them to have
> -# permission/device/user tables and target-finalize/rootfs-pre-cmd hooks.
> -else ifeq ($$(BR2_PACKAGE_HAS_$(2)),y) # $(2)_KCONFIG_VAR
> -
> -ifneq ($$($(2)_PERMISSIONS),)
> -PACKAGES_PERMISSIONS_TABLE += $$($(2)_PERMISSIONS)$$(sep)
> -endif
> -ifneq ($$($(2)_DEVICES),)
> -PACKAGES_DEVICES_TABLE += $$($(2)_DEVICES)$$(sep)
> -endif
> -ifneq ($$($(2)_USERS),)
> -PACKAGES_USERS += $$($(2)_USERS)$$(sep)
> -endif
> -TARGET_FINALIZE_HOOKS += $$($(2)_TARGET_FINALIZE_HOOKS)
> -ROOTFS_PRE_CMD_HOOKS += $$($(2)_ROOTFS_PRE_CMD_HOOKS)
> -
>   endif # $(2)_KCONFIG_VAR
>   endef # inner-generic-package
>