* Toolchain library whitelisting: A first pass @ 2012-04-26 1:38 Peter Seebach 2012-04-26 21:08 ` Toolchain library whitelisting: A first pass (preliminary patch/RFC) Peter Seebach 0 siblings, 1 reply; 5+ messages in thread From: Peter Seebach @ 2012-04-26 1:38 UTC (permalink / raw) To: openembedded-core This is a followup from some chat in #yocto and elsewhere. To make a long story short, some of the commercial vendors (like Wind River) tend to ship prebuilt binaries for glibc, and want to require these binaries to be used as-is (unless the user has done something special and is building libc). It would be convenient for people providing replacement toolchain layers to be able to catch attempts to build something that will fail because there are no corresponding libc binaries. With prebuilt libraries, the library is not always built with the exact same flags that everything else is, but it needs to be ABI-compatible with them. With that in mind, I coin the name "TUNEABI" to refer to the descriptor for a prebuilt library. This is basically like a tuning, but it's special in that multiple tunings can refer back to it as a thing they are based on or will work with. So I define a couple of values: 1. TUNEABI_WHITELIST. This is a space-separated list of tuneabi values which are supported. If this list does not exist, nothing happens; the check is a no-op. 2. TUNEABI_OVERRIDE. If this value exists, it suppresses the check even if TUNEABI_WHITELIST is set. This is sort of a placeholder for whatever people want to do about specifying a rebuild instead of using prebuilts. 3. TUNEABI_tune-foo. This specifies the (space-separated list of) tuneabis that can support the tuning foo. If absent, the tuning name is used. Intended use: Imagine that you are distributing a toolchain layer which has only one ARM library, which is armv7 thumb2, you would specify TUNEABI_WHITELIST = armv7at. Then a build for armv7at would work, but a build for armv6t would fail. If you are using the interwork feature, you might also specify TUNEABI_tune-armv7a = armv7at; then armv7a builds would be allowed also. The intent of this is that if you're providing a toolchain, and you want to support a limited set of prebuilt libraries, this check should be able to let you specify what you need without necessarily having to replace all the existing tuning and configurations. All you have to do is provide TUNEABI_tune-foo settings for any tunings you know to work with your libraries, and a TUNEABI_WHITELIST value, and the sanity check should catch everything likely. Thoughts, feedback? -s -- Listen, get this. Nobody with a good compiler needs to be justified. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Toolchain library whitelisting: A first pass (preliminary patch/RFC) 2012-04-26 1:38 Toolchain library whitelisting: A first pass Peter Seebach @ 2012-04-26 21:08 ` Peter Seebach 2012-04-26 22:01 ` Mark Hatle 0 siblings, 1 reply; 5+ messages in thread From: Peter Seebach @ 2012-04-26 21:08 UTC (permalink / raw) To: Patches and discussions about the oe-core layer 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(' ') + validities = data.getVarFlags('TUNEVALID') or "" + 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 != "": + 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.") + else: + tune = data.getVar("DEFAULTTUNE_virtclass-multilib-%s" % pairs[1], True) + if tune == deftune: + tune_error_set.append("Multilib '%s' (%s) is also the default tuning." % (pairs[1], deftune)) + 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' # 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': -- Listen, get this. Nobody with a good compiler needs to be justified. ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Toolchain library whitelisting: A first pass (preliminary patch/RFC) 2012-04-26 21:08 ` Toolchain library whitelisting: A first pass (preliminary patch/RFC) Peter Seebach @ 2012-04-26 22:01 ` Mark Hatle 2012-04-26 22:42 ` Peter Seebach 0 siblings, 1 reply; 5+ messages in thread From: Mark Hatle @ 2012-04-26 22:01 UTC (permalink / raw) To: openembedded-core 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': ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Toolchain library whitelisting: A first pass (preliminary patch/RFC) 2012-04-26 22:01 ` Mark Hatle @ 2012-04-26 22:42 ` Peter Seebach 2012-04-27 5:15 ` Chris Larson 0 siblings, 1 reply; 5+ messages in thread From: Peter Seebach @ 2012-04-26 22:42 UTC (permalink / raw) To: Patches and discussions about the oe-core layer On Thu, 26 Apr 2012 17:01:41 -0500 Mark Hatle <mark.hatle@windriver.com> wrote: > split does a whitespace based split automatically, I'm used to > seeing .split() everywhere. (I won't comment on the other similar > split items) Okay. > > + validities = data.getVarFlags('TUNEVALID') or "" > "validities"? that a new word? ;) Pretty much. I was trying to think of a name for "the set of valid things". :) > Change the above to: > multilibs = data.getVar("MULTILIBS", True) > if multilibs: If someone has done: MULTILIBS = "" this then ends up being confused because pairs[0] of the single returned item is still "", and that's not valid. > > + 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. I am surprised too, but if I use that name, I get a cryptic parse error from libxcb's recipe which I couldn't understand. > Also we can definitely have more then just lib32 or lib64. We > already often use libx32 in testing the x32 layer. Okay. I can improve the message, for sure. > 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. That probably wants to be tested too. > 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? I'm not totally sure. I blamed that for my problem where I kept getting a TUNE_ARCH of "x86_64x86_64" or "i586i586", but it turned out to be a red herring. I could change that to a warning. > Your whitespace usage looks different.. perhaps that is just my > mailer. I almost certainly have tabs. -s -- Listen, get this. Nobody with a good compiler needs to be justified. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Toolchain library whitelisting: A first pass (preliminary patch/RFC) 2012-04-26 22:42 ` Peter Seebach @ 2012-04-27 5:15 ` Chris Larson 0 siblings, 0 replies; 5+ messages in thread From: Chris Larson @ 2012-04-27 5:15 UTC (permalink / raw) To: Patches and discussions about the oe-core layer On Thu, Apr 26, 2012 at 3:42 PM, Peter Seebach <peter.seebach@windriver.com> wrote: >> Change the above to: >> multilibs = data.getVar("MULTILIBS", True) >> if multilibs: > > If someone has done: > > MULTILIBS = "" > > this then ends up being confused because pairs[0] of the single > returned item is still "", and that's not valid. This doesn't make sense. If you do "if multilibs:" and multilibs is the empty string, it'll evaluate as false, and that block won't be entered. -- Christopher Larson ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-27 5:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-26 1:38 Toolchain library whitelisting: A first pass Peter Seebach 2012-04-26 21:08 ` Toolchain library whitelisting: A first pass (preliminary patch/RFC) Peter Seebach 2012-04-26 22:01 ` Mark Hatle 2012-04-26 22:42 ` Peter Seebach 2012-04-27 5:15 ` Chris Larson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.