Re: [PATCH 1/6] allarch: disable allarch when multilib is used

[richard.purdie@linuxfoundation.org]

Hi,

I think this is close and it passes the tests on the autobuilder
however I did spot a couple of potential issues after thinking about
this for a bit.

On Sun, 2018-08-26 at 06:06 -0700, Kai Kang wrote:
> Some allarch packages rdepends non-allarch packages. When multilib is
> used, it doesn't expand the dependency chain correctly, e.g.
> 
> core-image-sato -> ca-certificates(allarch) -> openssl
> 
> we expect dependency chain for lib32-core-image-sato:
> 
> lib32-core-image-sato -> ca-certificates(allarch) -> lib32-openssl
> 
> it should install lib32-openssl for ca-certificates but openssl is
> still wrongly required.
> 
> Copy allarch.bbclass to allarch-enabled.bbclass and only inherit
> allarch-enabled.bbclass when multilib is not used.
> 
> Signed-off-by: Kai Kang <kai.kang@windriver.com>
> ---
>  meta/classes/allarch-enabled.bbclass | 52 ++++++++++++++++++++++++++++++++++++
>  meta/classes/allarch.bbclass         | 51 ++---------------------------------
>  meta/classes/icecc.bbclass           |  2 +-
>  meta/classes/multilib.bbclass        |  2 +-
>  meta/classes/multilib_global.bbclass |  2 +-
>  meta/classes/package.bbclass         |  6 ++---
>  6 files changed, 60 insertions(+), 55 deletions(-)
>  create mode 100644 meta/classes/allarch-enabled.bbclass
> 
> 
> diff --git a/meta/classes/allarch.bbclass b/meta/classes/allarch.bbclass
> index 1eebe0bf2e7..0eca076df0b 100644
> --- a/meta/classes/allarch.bbclass
> +++ b/meta/classes/allarch.bbclass
> @@ -1,52 +1,5 @@
>  #
> -# This class is used for architecture independent recipes/data files (usually scripts)
> +# This class enables allarch only when multilib is not used.
>  #
>  
> -PACKAGE_ARCH = "all"
> -
> -python () {
> -    # Allow this class to be included but overridden - only set
> -    # the values if we're still "all" package arch.
> -    if d.getVar("PACKAGE_ARCH") == "all":
> -        # No need for virtual/libc or a cross compiler
> -        d.setVar("INHIBIT_DEFAULT_DEPS","1")
> -
> -        # Set these to a common set of values, we shouldn't be using them other that for WORKDIR directory
> -        # naming anyway
> -        d.setVar("baselib", "lib")
> -        d.setVar("TARGET_ARCH", "allarch")
> -        d.setVar("TARGET_OS", "linux")
> -        d.setVar("TARGET_CC_ARCH", "none")
> -        d.setVar("TARGET_LD_ARCH", "none")
> -        d.setVar("TARGET_AS_ARCH", "none")
> -        d.setVar("TARGET_FPU", "")
> -        d.setVar("TARGET_PREFIX", "")
> -        # Expand PACKAGE_EXTRA_ARCHS since the staging code needs this
> -        # (this removes any dependencies from the hash perspective)
> -        d.setVar("PACKAGE_EXTRA_ARCHS", d.getVar("PACKAGE_EXTRA_ARCHS"))
> -        d.setVar("SDK_ARCH", "none")
> -        d.setVar("SDK_CC_ARCH", "none")
> -        d.setVar("TARGET_CPPFLAGS", "none")
> -        d.setVar("TARGET_CFLAGS", "none")
> -        d.setVar("TARGET_CXXFLAGS", "none")
> -        d.setVar("TARGET_LDFLAGS", "none")
> -        d.setVar("POPULATESYSROOTDEPS", "")
> -
> -        # Avoid this being unnecessarily different due to nuances of
> -        # the target machine that aren't important for "all" arch
> -        # packages.
> -        d.setVar("LDFLAGS", "")
> -
> -        # No need to do shared library processing or debug symbol handling
> -        d.setVar("EXCLUDE_FROM_SHLIBS", "1")
> -        d.setVar("INHIBIT_PACKAGE_DEBUG_SPLIT", "1")
> -        d.setVar("INHIBIT_PACKAGE_STRIP", "1")
> -
> -        # These multilib values shouldn't change allarch packages so exclude them
> -        d.appendVarFlag("emit_pkgdata", "vardepsexclude", " MULTILIB_VARIANTS")
> -        d.appendVarFlag("write_specfile", "vardepsexclude", " MULTILIBS")
> -        d.appendVarFlag("do_package", "vardepsexclude", " package_do_shlibs")
> -    elif bb.data.inherits_class('packagegroup', d) and not bb.data.inherits_class('nativesdk', d):
> -        bb.error("Please ensure recipe %s sets PACKAGE_ARCH before inherit packagegroup" % d.getVar("FILE"))
> -}
> -
> +inherit ${@oe.utils.ifelse(d.getVar('MULTILIB_VARIANTS'), '', 'allarch-enabled')}

Firstly, whilst I do understand why you've made this change like this,
I don't really like one line classes which clutter up the classes
directory. Using inherit like this does force the expansion of the
variable early and is very prone to "race" conditions. The above is
safe but see below. 

FWIW the original code tried to switch off the value of the
PACKAGE_ARCH variable. If we change the way we enable/disable the code,
we should consider whether we can consistently use one mechanism for
both.

Secondly, does this change affect the behaviour of nativesdk?

I think this will disable allarch for nativesdk in any multilib build
and we don't want to do that, we only have a problem with target
multilib allarch.

At a guess you probably need to check class-target is in overrides and
multilib_variants is set? The problem could be that class-target is set
comparatively late and then we're into ordering issues.

I do want to get the nativesdk issue fixed before we merge this though.

Cheers,

Richard