Re: Toolchain library whitelisting: A first pass (preliminary patch/RFC)

[Mark Hatle <mark.hatle@windriver.com>]

In general I like this.. see a few critiques below:

On 4/26/12 4:08 PM, Peter Seebach wrote:
> On Wed, 25 Apr 2012 20:38:05 -0500
> Peter Seebach<peter.seebach@windriver.com>  wrote:
>> This is a followup from some chat in #yocto and elsewhere.
>
> Okay, some more followup.
>
> While testing this, I kept burning myself on perfectly reasonable
> things to get wrong while defining and using multilibs, so I wrote a
> bunch of sanity checks for that.
>
> The intent of this is to validate that tunings (including multilibs)
> are configured in a reasonable way that we would expect to work.  This
> includes:
>
> 1.  Verifying that no multilib's tuning is the same as DEFAULTTUNE.
> 2.  Verifying that no multilib's library name is 'lib', because that
> causes really cryptic error messages parsing recipes.
> 3.  For each tuning, verify:
> 3a.  The tuning has features.
> 3b.  Every feature has a TUNEVALID[x] entry.
> 3c.  If the feature has a TUNECONFLICTS[x] entry, no feature listed in
> it is included in the feature list.
> 3d.  If the value TUNEABI_WHITELIST exists, the tuning's
> TUNEABI_tune-foo value, or the tuning's name if that doesn't exist, is
> in TUNEABI_WHITELIST, or alternatively, TUNEABI_OVERRIDE is defined.
>
> The whitelist feature is optional, and my intent would be not to define
> any TUNEABI_tune values in oe-core, but just to maintain the hooks so
> that people with custom (and often prebuilt-binary) toolchains can use
> it without all of us writing our own.
>
> I am totally aware that my Python is a little rough, and would be happy
> to improve the legibility or idiom.
>
> Separately, I propose also the following fix:
>
>       # Check TARGET_ARCH is set correctly
> -    if data.getVar('TARGE_ARCH', e.data, False) == '${TUNE_ARCH}':
> +    if data.getVar('TARGET_ARCH', e.data, False) == '${TUNE_ARCH}':
>
> Anyway, the patch:
>
> diff --git a/meta/classes/sanity.bbclass b/meta/classes/sanity.bbclass
> index 687ddeb..b7f93b5 100644
> --- a/meta/classes/sanity.bbclass
> +++ b/meta/classes/sanity.bbclass
> @@ -11,6 +11,70 @@ def raise_sanity_error(msg):
>
>       %s""" % msg)
>
> +# Check a single tune for validity.
> +def check_toolchain_tune(data, tune, multilib):
> +    tune_errors = []
> +    bb.note("Sanity-checking tuning '%s' (%s) features:" % (tune, multilib))
> +    features = data.getVar("TUNE_FEATURES_tune-%s" % tune, True) or ""
> +    if features == '':
> +        return "Tuning '%s' has no defined features, and cannot be used." % tune
> +    features = features.split(' ')

split does a whitespace based split automatically, I'm used to seeing .split() 
everywhere.  (I won't comment on the other similar split items)

> +    validities = data.getVarFlags('TUNEVALID') or ""

"validities"?  that a new word?  ;)

> +    conflicts = data.getVarFlags('TUNECONFLICTS') or ""
> +    split_conflicts = {}
> +    for feature in features:
> +       if feature in conflicts:
> +	   if feature not in split_conflicts:
> +	       split_conflicts[feature] = conflicts[feature].split(' ')
> +	   for other in features:
> +	       if other in split_conflicts[feature]:
> +		   tune_errors.append("Feature '%s' conflicts with '%s'." %
> +		       ( feature, other ))
> +       if feature in validities:
> +	   bb.note("  %s: %s" % (feature, validities[feature]))
> +       else:
> +	   tune_errors.append("Feature '%s' is not defined." % feature)
> +    whitelist = data.getVar("TUNEABI_WHITELIST", True) or ''
> +    if whitelist != '':
> +	override = data.getVar("TUNEABI_OVERRIDE", True) or ''
> +	if not override:
> +	    tuneabi = data.getVar("TUNEABI", True) or ""
> +	    if tuneabi == "" or tuneabi.startswith('$'):
> +		tuneabi = tune
> +	    if True not in [x in whitelist.split(' ') for x in tuneabi.split(' ')]:
> +		tune_errors.append("Tuning '%s' (%s) cannot be used with any supported tuning/ABI." %
> +		    (tune, tuneabi))
> +    if tune_errors:
> +        return "Tuning '%s' has the following errors:\n" + '\n'.join(tune_errors)
> +
> +def check_toolchain(data):
> +    tune_error_set = []
> +    deftune = data.getVar("DEFAULTTUNE", True)
> +    tune_errors = check_toolchain_tune(data, deftune, 'default')
> +    if tune_errors:
> +        tune_error_set.append(tune_errors)
> +
> +    multilibs = data.getVar("MULTILIBS", True) or ""
> +    if multilibs != "":

Change the above to:
     multilibs = data.getVar("MULTILIBS", True)
     if multilibs:

> +        for pairs in [x.split(':') for x in multilibs.split(' ')]:
> +	    if pairs[0] != 'multilib':
> +	        bb.warn("Got an unexpected '%s' in MULTILIBS." % pairs[0])
> +	    else:
> +		if pairs[1] == 'lib':
> +		    tune_error_set.append("The multilib 'lib' was specified, but that doesn't work. You need lib32 or lib64.")

I'm surprised, why doesn't 'lib' work?  I was under the impression the naming 
was completely arbitrary.

Also we can definitely have more then just lib32 or lib64.  We already often use 
libx32 in testing the x32 layer.

> +		else:
> +		    tune = data.getVar("DEFAULTTUNE_virtclass-multilib-%s" % pairs[1], True)

If the tune isn't defined, then the multilib configuration is invalid.  I 
thought we already had a check for that somewhere else.. but if not.. it 
wouldn't be a bad idea to mention that here for the user.

A simple if not tune:  would do it..

> +		    if tune == deftune:
> +		        tune_error_set.append("Multilib '%s' (%s) is also the default tuning." % (pairs[1], deftune))

I wonder if this is an error or a warning.. I suspect it would be unintentional, 
but I'm not sure it's a failure?

> +		    else:
> +		        tune_errors = check_toolchain_tune(data, tune, pairs[1])
> +		    if tune_errors:
> +		        tune_error_set.append(tune_errors)
> +    if tune_error_set:
> +        return "Toolchain tunings invalid:\n" + '\n'.join(tune_error_set)
> +
> +    return ""
> +
>   def check_conf_exists(fn, data):
>       bbpath = []
>       fn = data.expand(fn)
> @@ -327,6 +391,9 @@ def check_sanity(e):
>           messages = messages + pseudo_msg + '\n'
>
>       check_supported_distro(e)
> +    toolchain_msg = check_toolchain(e.data)
> +    if toolchain_msg != "":
> +        messages = messages + toolchain_msg + '\n'

Your whitespace usage looks different.. perhaps that is just my mailer.

--Mark

>
>       # Check if DISPLAY is set if IMAGETEST is set
>       if not data.getVar( 'DISPLAY', e.data, True ) and data.getVar( 'IMAGETEST', e.data, True ) == 'qemu':