* [PATCH 0/2] Yocto Compatible 2.0 support code @ 2017-06-07 15:31 Patrick Ohly 2017-06-07 15:31 ` [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly ` (3 more replies) 0 siblings, 4 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-07 15:31 UTC (permalink / raw) To: openembedded-core As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 + signature changes" mail thread, changes in a .bbappend cannot be done unconditionally. Making _append and _remove depend on overrides which get set based on DISTRO_FEATURES is one way of achieving this. The oe.utils.optional_includes() helper function has not been discussed before. It's an attempt to address concerns by developers that having to write code for (potentially complex) condition checking is error prone and hard to read. It depends on the bitbake enhancement that allows including multiple files at once. Patrick Ohly (2): bitbake.conf: DISTRO_FEATURES as overrides utils.py: helper function for optional include files meta/conf/bitbake.conf | 17 ++++++++++++++++- meta/lib/oe/utils.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) base-commit: 49c255494c1d0704a1c8c428281c81541b05dc3e -- git-series 0.9.1 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-07 15:31 [PATCH 0/2] Yocto Compatible 2.0 support code Patrick Ohly @ 2017-06-07 15:31 ` Patrick Ohly 2017-06-07 16:11 ` Peter Kjellerstedt 2017-06-07 15:31 ` [PATCH 2/2] utils.py: helper function for optional include files Patrick Ohly ` (2 subsequent siblings) 3 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-07 15:31 UTC (permalink / raw) To: openembedded-core As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 + signature changes", changes in .bbappend must depend on some explicit configuration change, typically selecting a distro feature. For _append and _remove, adding an override that is set only when the corresponding entry is in DISTRO_FEATURES achieves that: DISTRO_FEATURES = " ... my-distro-feature ... " do_install_append_my-distro-feature () { ... } Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/conf/bitbake.conf | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf index 3ad905c..ca6501e 100644 --- a/meta/conf/bitbake.conf +++ b/meta/conf/bitbake.conf @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded" # # This works for functions as well, they are really just environment variables. # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration. -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable" +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable" OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES" CLASSOVERRIDE ?= "class-target" DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}" @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE" FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}" +# Turns certain DISTRO_FEATURES into overrides of the same name +# or (optionally) some other name. Ensures that these special +# distro features remain set also for native and nativesdk +# recipes, so that these overrides can also be used there. +# +# Beware that this part of OVERRIDES changes during parsing, so usage +# of these overrides should be limited to .bb and .bbappend files, +# because then DISTRO_FEATURES is final. +DISTRO_FEATURES_OVERRIDES ??= "" +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ +Each entry is added to OVERRIDES with the <feature> name if <feature> is in DISTRO_FEATURES." +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" +DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" +DISTROFEATURESOVERRIDES = "${@ ''.join([':' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" + ################################################################## # Include the rest of the config files. ################################################################## -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-07 15:31 ` [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly @ 2017-06-07 16:11 ` Peter Kjellerstedt 2017-06-08 6:04 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Peter Kjellerstedt @ 2017-06-07 16:11 UTC (permalink / raw) To: Patrick Ohly, openembedded-core > -----Original Message----- > From: openembedded-core-bounces@lists.openembedded.org > [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of > Patrick Ohly > Sent: den 7 juni 2017 17:32 > To: openembedded-core@lists.openembedded.org > Subject: [OE-core] [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as > overrides > > As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 + > signature changes", changes in .bbappend must depend on some explicit > configuration change, typically selecting a distro feature. > > For _append and _remove, adding an override that is set only when the > corresponding entry is in DISTRO_FEATURES achieves that: > > DISTRO_FEATURES = " ... my-distro-feature ... " > > do_install_append_my-distro-feature () { > ... > } > > Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> > --- > meta/conf/bitbake.conf | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > index 3ad905c..ca6501e 100644 > --- a/meta/conf/bitbake.conf > +++ b/meta/conf/bitbake.conf > @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded" > # > # This works for functions as well, they are really just environment > variables. > # Default OVERRIDES to make compilation fail fast in case of build > system misconfiguration. > -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build- > ${BUILD_OS}:pn- > ${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevari > able" > +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build- > ${BUILD_OS}:pn- > ${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}: > ${CLASSOVERRIDE}:forcevariable" > OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES" > CLASSOVERRIDE ?= "class-target" > DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}" > @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE" > > FILESOVERRIDES = > "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}" > > +# Turns certain DISTRO_FEATURES into overrides of the same name > +# or (optionally) some other name. Ensures that these special > +# distro features remain set also for native and nativesdk > +# recipes, so that these overrides can also be used there. > +# > +# Beware that this part of OVERRIDES changes during parsing, so usage > +# of these overrides should be limited to .bb and .bbappend files, > +# because then DISTRO_FEATURES is final. > +DISTRO_FEATURES_OVERRIDES ??= "" > +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> > entries. \ > +Each entry is added to OVERRIDES with the <feature> name if <feature> > is in DISTRO_FEATURES." > +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" > +DISTRO_FEATURES_FILTER_NATIVESDK_append = " > ${DISTRO_FEATURES_OVERRIDES}" > +DISTROFEATURESOVERRIDES = "${@ ''.join([':' + x for x in > (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & > set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" > + > ################################################################## > # Include the rest of the config files. > ################################################################## > -- > git-series 0.9.1 Rather than requiring that the wanted DISTRO_FEATURES that should be available as overrides are defined in DISTRO_FEATURES_OVERRIDES (which should not be confused with the similarly named DISTROFEATURESOVERRIDES variable that you also add...), why not add them all but with a prefix. I.e., similar to how package names are available as overrides prefixed with "pn-", how about all distro features are made available as overrides with a "df-" prefix? //Peter ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-07 16:11 ` Peter Kjellerstedt @ 2017-06-08 6:04 ` Patrick Ohly 2017-06-08 10:45 ` Richard Purdie 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-08 6:04 UTC (permalink / raw) To: Peter Kjellerstedt; +Cc: openembedded-core On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote: > Rather than requiring that the wanted DISTRO_FEATURES that should be > available as overrides are defined in DISTRO_FEATURES_OVERRIDES (which > should not be confused with the similarly named DISTROFEATURESOVERRIDES > variable that you also add...), I had thought about those names and in the end went ahead with the similar names because the customizable one made sense to me and the internal one is similar to the other entries in OVERRIDES. > why not add them all but with a prefix. > I.e., similar to how package names are available as overrides prefixed > with "pn-", how about all distro features are made available as > overrides with a "df-" prefix? That would be fine with me. I just have a few concerns: * How performance-sensitive is OVERRIDES? How can the impact of both approaches be benchmarked? The idea behind the configurable subset was to add only a few new overrides. We currently have almost 70 individual entries in DISTRO_FEATURES. * I've seen confusion about the pn- prefix. At least df- would be named appropriately (in contrast to PN, which is historic), but it's yet another convention that might not be immediately obvious. The same is true for selecting a subset with the same name as the feature, though. * Can distro features contain characters that are invalid in an override? _ and : would have to be avoided, for example by mapping them to -. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-08 6:04 ` Patrick Ohly @ 2017-06-08 10:45 ` Richard Purdie 2017-06-08 13:16 ` Peter Kjellerstedt 0 siblings, 1 reply; 42+ messages in thread From: Richard Purdie @ 2017-06-08 10:45 UTC (permalink / raw) To: Patrick Ohly, Peter Kjellerstedt; +Cc: openembedded-core On Thu, 2017-06-08 at 08:04 +0200, Patrick Ohly wrote: > On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote: > > > > Rather than requiring that the wanted DISTRO_FEATURES that should > > be > > available as overrides are defined in DISTRO_FEATURES_OVERRIDES > > (which > > should not be confused with the similarly named > > DISTROFEATURESOVERRIDES > > variable that you also add...), > I had thought about those names and in the end went ahead with the > similar names because the customizable one made sense to me and the > internal one is similar to the other entries in OVERRIDES. > > > > > why not add them all but with a prefix. > > I.e., similar to how package names are available as overrides > > prefixed > > with "pn-", how about all distro features are made available as > > overrides with a "df-" prefix? > That would be fine with me. > > I just have a few concerns: > * How performance-sensitive is OVERRIDES? How can the impact of > both approaches be benchmarked? The idea behind the > configurable > subset was to add only a few new overrides. We currently have > almost 70 individual entries in DISTRO_FEATURES. > * I've seen confusion about the pn- prefix. At least df- would > be > named appropriately (in contrast to PN, which is historic), > but > it's yet another convention that might not be immediately > obvious. The same is true for selecting a subset with the > same > name as the feature, though. > * Can distro features contain characters that are invalid in an > override? _ and : would have to be avoided, for example by > mapping them to -. My feelings are: * We need to use a prefix. We've been burnt too many times in the past when not using these. "df-" is fine, users will just have to deal with it. * We should filter the list of overrides to those which the distro wants to nominate. I really don't want to encourage wider spread of things than we need to, these need to be conscious decisions. Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-08 10:45 ` Richard Purdie @ 2017-06-08 13:16 ` Peter Kjellerstedt 2017-06-08 14:38 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Peter Kjellerstedt @ 2017-06-08 13:16 UTC (permalink / raw) To: Richard Purdie, Patrick Ohly; +Cc: openembedded-core > -----Original Message----- > From: Richard Purdie [mailto:richard.purdie@linuxfoundation.org] > Sent: den 8 juni 2017 12:45 > To: Patrick Ohly <patrick.ohly@intel.com>; Peter Kjellerstedt > <peter.kjellerstedt@axis.com> > Cc: openembedded-core@lists.openembedded.org > Subject: Re: [OE-core] [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as > overrides > > On Thu, 2017-06-08 at 08:04 +0200, Patrick Ohly wrote: > > On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote: > > > > > > Rather than requiring that the wanted DISTRO_FEATURES that should > > > be > > > available as overrides are defined in DISTRO_FEATURES_OVERRIDES > > > (which > > > should not be confused with the similarly named > > > DISTROFEATURESOVERRIDES > > > variable that you also add...), > > I had thought about those names and in the end went ahead with the > > similar names because the customizable one made sense to me and the > > internal one is similar to the other entries in OVERRIDES. > > > > > > > > why not add them all but with a prefix. > > > I.e., similar to how package names are available as overrides > > > prefixed > > > with "pn-", how about all distro features are made available as > > > overrides with a "df-" prefix? > > That would be fine with me. > > > > I just have a few concerns: > > * How performance-sensitive is OVERRIDES? How can the impact of > > both approaches be benchmarked? The idea behind the > > configurable > > subset was to add only a few new overrides. We currently have > > almost 70 individual entries in DISTRO_FEATURES. > > * I've seen confusion about the pn- prefix. At least df- would > > be > > named appropriately (in contrast to PN, which is historic), > > but > > it's yet another convention that might not be immediately > > obvious. The same is true for selecting a subset with the > > same > > name as the feature, though. > > * Can distro features contain characters that are invalid in an > > override? _ and : would have to be avoided, for example by > > mapping them to -. > > My feelings are: > > * We need to use a prefix. We've been burnt too many times in the past > when not using these. "df-" is fine, users will just have to deal > with it. > > * We should filter the list of overrides to those which the distro > wants to nominate. I really don't want to encourage wider spread of > things than we need to, these need to be conscious decisions. Isn't there a risk that will be confusing? I.e., recipes that use, e.g., bb.utils.contains() to check if a distro feature is set will be affected as soon as the feature is added to DISTRO_FEATURES, but recipes that use the override will only be affected if the feature has also been added to some filter variable. > Cheers, > > Richard //Peter ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-08 13:16 ` Peter Kjellerstedt @ 2017-06-08 14:38 ` Patrick Ohly 0 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-08 14:38 UTC (permalink / raw) To: Peter Kjellerstedt; +Cc: openembedded-core On Thu, 2017-06-08 at 13:16 +0000, Peter Kjellerstedt wrote: > > My feelings are: > > > > * We need to use a prefix. We've been burnt too many times in the past > > when not using these. "df-" is fine, users will just have to deal > > with it. Fine with me. > > * We should filter the list of overrides to those which the distro > > wants to nominate. I really don't want to encourage wider spread of > > things than we need to, these need to be conscious decisions. > > Isn't there a risk that will be confusing? I.e., recipes that use, e.g., > bb.utils.contains() to check if a distro feature is set will be affected > as soon as the feature is added to DISTRO_FEATURES, but recipes that > use the override will only be affected if the feature has also been > added to some filter variable. I'm not sure. I guess it boils down to proper documentation. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] utils.py: helper function for optional include files 2017-06-07 15:31 [PATCH 0/2] Yocto Compatible 2.0 support code Patrick Ohly 2017-06-07 15:31 ` [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly @ 2017-06-07 15:31 ` Patrick Ohly 2017-06-08 9:20 ` Richard Purdie 2017-06-07 15:43 ` [PATCH 0/2] Yocto Compatible 2.0 support code Joshua Watt 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly 3 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-07 15:31 UTC (permalink / raw) To: openembedded-core By using oe.utils.optional_includes(), developers can simplify the code which selects which additional include files need to be included in a .bbappend. In the simple case (one distro feature and one include file) the code is not shorter, but the intent is clearer than corresponding code using bb.utils.contains(): require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } More complex cases are also supported, in particular include files that are required for one of several distro features or multiple different include files. To keep the common use case simple, DISTRO_FEATURES are checked by default. Checking IMAGE_FEATURES might also be useful. The DISTRO_FEATURES default and the intended usage make this more suitable for OE-core than bitbake. Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/lib/oe/utils.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/meta/lib/oe/utils.py b/meta/lib/oe/utils.py index 330a5ff..a6c6199 100644 --- a/meta/lib/oe/utils.py +++ b/meta/lib/oe/utils.py @@ -126,6 +126,35 @@ def features_backfill(var,d): if addfeatures: d.appendVar(var, " " + " ".join(addfeatures)) +def optional_includes(d, mapping, key_var="DISTRO_FEATURES"): + """ + This can be used to generate a list of files to include depending on + the distro features that are selected. key_var contains the features + that are set, mapping_var a space-separated set of <feature(s)>:<file(s)> + entries. Features and files are separated by comma. Each file on the + right-hand side is included in the result once if any of the features one + the left-hand side is set. + + Example: + require ${@ oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")} + + For DISTRO_FEATURES = "foo xyz" that will include four .inc files in the + order in which they are listed. + """ + key = set((d.getVar(key_var) or "").split()) + mapping = mapping.split() + includes = [] + for entry in mapping: + parts = entry.split(":", 1) + if len(parts) != 2: + bb.fatal("%s must contain entries of the form <feature(s)>:<file(s)>, not %s" % (mapping_var, entry)) + features, files = parts + for feature in features.split(","): + if feature in key: + for file in files.split(","): + if file not in includes: + includes.append(file) + return " ".join(includes) def packages_filter_out_system(d): """ -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] utils.py: helper function for optional include files 2017-06-07 15:31 ` [PATCH 2/2] utils.py: helper function for optional include files Patrick Ohly @ 2017-06-08 9:20 ` Richard Purdie 2017-06-08 14:36 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Richard Purdie @ 2017-06-08 9:20 UTC (permalink / raw) To: Patrick Ohly, openembedded-core On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > By using oe.utils.optional_includes(), developers can simplify the > code which selects which additional include files need to be included > in a .bbappend. > > In the simple case (one distro feature and one include file) the code > is not shorter, but the intent is clearer than corresponding code > using bb.utils.contains(): > > require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } > > More complex cases are also supported, in particular include files > that are required for one of several distro features or multiple > different include files. > > To keep the common use case simple, DISTRO_FEATURES are checked by > default. Checking IMAGE_FEATURES might also be useful. > > The DISTRO_FEATURES default and the intended usage make this more > suitable for OE-core than bitbake. I'm honestly not sure this actually aids readability. Taking your example: require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")} I think I actually prefer: require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc")} require ${@oe.utils.optional_includes(d, "xyz:x.inc,y.inc,z.inc")} since its more explict. Spelling this out with the existing syntax isn't so bad either, I probably still think that this is clearer too: require ${@bb.utils.contains(d, "DISTRO_FEATURES", "foo bar", "foo-or-bar.inc", "")} require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc y.inc z.inc", "")} How about simply: require ${@oe.utils.distro_features(d, "foo bar", "foo-or-bar.inc")} require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")} ? Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] utils.py: helper function for optional include files 2017-06-08 9:20 ` Richard Purdie @ 2017-06-08 14:36 ` Patrick Ohly 2017-06-09 10:02 ` Richard Purdie 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-08 14:36 UTC (permalink / raw) To: Richard Purdie; +Cc: openembedded-core On Thu, 2017-06-08 at 10:20 +0100, Richard Purdie wrote: > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > > By using oe.utils.optional_includes(), developers can simplify the > > code which selects which additional include files need to be included > > in a .bbappend. > > > > In the simple case (one distro feature and one include file) the code > > is not shorter, but the intent is clearer than corresponding code > > using bb.utils.contains(): > > > > require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } > > > > More complex cases are also supported, in particular include files > > that are required for one of several distro features or multiple > > different include files. > > > > To keep the common use case simple, DISTRO_FEATURES are checked by > > default. Checking IMAGE_FEATURES might also be useful. > > > > The DISTRO_FEATURES default and the intended usage make this more > > suitable for OE-core than bitbake. > > I'm honestly not sure this actually aids readability. Fair enough. > Taking your example: > > require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")} > > I think I actually prefer: > > require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc")} > require ${@oe.utils.optional_includes(d, "xyz:x.inc,y.inc,z.inc")} > > since its more explict. Spelling this out with the existing syntax > isn't so bad either, I probably still think that this is clearer too: > > require ${@bb.utils.contains(d, "DISTRO_FEATURES", "foo bar", "foo-or-bar.inc", "")} That's the "foo and bar" case, not "foo or bar". It's an actual mistake that people have made and that didn't get caught during code review. > require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc y.inc z.inc", "")} > > How about simply: > > require ${@oe.utils.distro_features(d, "foo bar", "foo-or-bar.inc")} > require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")} That works for me, I just wonder about the exact semantic of listing multiple features: "and" as in contains(), or "any"? I'm leaning towards two functions where that is explicit: oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc") oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc") -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] utils.py: helper function for optional include files 2017-06-08 14:36 ` Patrick Ohly @ 2017-06-09 10:02 ` Richard Purdie 0 siblings, 0 replies; 42+ messages in thread From: Richard Purdie @ 2017-06-09 10:02 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Thu, 2017-06-08 at 16:36 +0200, Patrick Ohly wrote: > On Thu, 2017-06-08 at 10:20 +0100, Richard Purdie wrote: > > > > require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc > > y.inc z.inc", "")} > > > > How about simply: > > > > require ${@oe.utils.distro_features(d, "foo bar", "foo-or- > > bar.inc")} > > require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")} > That works for me, I just wonder about the exact semantic of listing > multiple features: "and" as in contains(), or "any"? > > I'm leaning towards two functions where that is explicit: > oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc") > oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc") I think you're right, this distinction is easily missed and I think its an idea to highlight it as you propose... Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-07 15:31 [PATCH 0/2] Yocto Compatible 2.0 support code Patrick Ohly 2017-06-07 15:31 ` [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly 2017-06-07 15:31 ` [PATCH 2/2] utils.py: helper function for optional include files Patrick Ohly @ 2017-06-07 15:43 ` Joshua Watt 2017-06-08 8:56 ` Richard Purdie 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly 3 siblings, 1 reply; 42+ messages in thread From: Joshua Watt @ 2017-06-07 15:43 UTC (permalink / raw) To: Patrick Ohly, openembedded-core On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 > + signature changes" mail thread, changes in a .bbappend cannot be > done unconditionally. Making _append and _remove depend on overrides > which get set based on DISTRO_FEATURES is one way of achieving this. > > The oe.utils.optional_includes() helper function has not been > discussed before. It's an attempt to address concerns by developers > that having to write code for (potentially complex) condition > checking > is error prone and hard to read. I promise I'm not trying to start a flame war here, and perhaps there is history behind this that I'm not aware of but... Why doesn't bitbake support some sort of "if" statement? It seems like most of what we are trying to do could be accomplished with much less fuss if one could simply do this in the bb file: if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d): include foo.inc One could even eliminate the separate inc file and simply put its contents under the conditional (as much fun as it seems to have to open a new file just to see what a recipe is doing with a distro feature...) It would also appear that this could make a lot of other things simpler as well (and may even negate the need to backfill DISTRO_FEATURES into overrides?) > > It depends on the bitbake enhancement that allows including multiple > files at once. > > Patrick Ohly (2): > bitbake.conf: DISTRO_FEATURES as overrides > utils.py: helper function for optional include files > > meta/conf/bitbake.conf | 17 ++++++++++++++++- > meta/lib/oe/utils.py | 29 +++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+), 1 deletion(-) > > base-commit: 49c255494c1d0704a1c8c428281c81541b05dc3e > -- > git-series 0.9.1 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-07 15:43 ` [PATCH 0/2] Yocto Compatible 2.0 support code Joshua Watt @ 2017-06-08 8:56 ` Richard Purdie 2017-06-08 13:55 ` Joshua Watt 0 siblings, 1 reply; 42+ messages in thread From: Richard Purdie @ 2017-06-08 8:56 UTC (permalink / raw) To: Joshua Watt, Patrick Ohly, openembedded-core On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote: > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > > > > As discussed in the "[Openembedded-architecture] Yocto Compatible > > 2.0 > > + signature changes" mail thread, changes in a .bbappend cannot be > > done unconditionally. Making _append and _remove depend on > > overrides > > which get set based on DISTRO_FEATURES is one way of achieving > > this. > > > > The oe.utils.optional_includes() helper function has not been > > discussed before. It's an attempt to address concerns by developers > > that having to write code for (potentially complex) condition > > checking > > is error prone and hard to read. > I promise I'm not trying to start a flame war here, and perhaps there > is history behind this that I'm not aware of but... > > Why doesn't bitbake support some sort of "if" statement? It seems > like most of what we are trying to do could be accomplished with much > less fuss if one could simply do this in the bb file: > > if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d): > include foo.inc This wouldn't actually solve as much of the problem as you think it might at first glance and probably causes others, at least as I understand it (as someone who's worked on bitbake's override code). For example, at what point does this get evaluated? Most bitbake variables are expanded at usage time, not parse time but here, the way the parser works today, it would have to do an immediate expansion of DISTRO_FEATURES to decide whether to include this file (or code block). So ok, lets assume we change bitbake massively and defer the expansion somehow. What if foo.inc influences the contents of DISTRO_FEATURES? Should it then "unparse" foo.inc if my-feature was removed? or error? or silently ignore that? bitbake's main conditional today is through overrides and these do allow a controlled delayed expansion of metadata in most cases. In some cases such as include and inherit statements there is still the immediate expansion issue above but at least there aren't huge changes to the parser required to make it work so its the best of both worlds. > One could even eliminate the separate inc file and simply put its > contents under the conditional (as much fun as it seems to have to > open > a new file just to see what a recipe is doing with a distro > feature...) > > It would also appear that this could make a lot of other things > simpler as well (and may even negate the need to backfill > DISTRO_FEATURES into overrides?) See if the above gives food for thought on that... The big problems are the corner cases. If we do add new syntax it needs to avoid these as we already have some pretty nasty ones, thankfully most people don't hit them though. Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 8:56 ` Richard Purdie @ 2017-06-08 13:55 ` Joshua Watt 2017-06-08 14:33 ` Richard Purdie 0 siblings, 1 reply; 42+ messages in thread From: Joshua Watt @ 2017-06-08 13:55 UTC (permalink / raw) To: Richard Purdie, Patrick Ohly, openembedded-core On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote: > On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote: > > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > > > > > > As discussed in the "[Openembedded-architecture] Yocto Compatible > > > 2.0 > > > + signature changes" mail thread, changes in a .bbappend cannot > > > be > > > done unconditionally. Making _append and _remove depend on > > > overrides > > > which get set based on DISTRO_FEATURES is one way of achieving > > > this. > > > > > > The oe.utils.optional_includes() helper function has not been > > > discussed before. It's an attempt to address concerns by > > > developers > > > that having to write code for (potentially complex) condition > > > checking > > > is error prone and hard to read. > > > > I promise I'm not trying to start a flame war here, and perhaps > > there > > is history behind this that I'm not aware of but... > > > > Why doesn't bitbake support some sort of "if" statement? It seems > > like most of what we are trying to do could be accomplished with > > much > > less fuss if one could simply do this in the bb file: > > > > if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d): > > include foo.inc > > This wouldn't actually solve as much of the problem as you think it > might at first glance and probably causes others, at least as I > understand it (as someone who's worked on bitbake's override code). > > For example, at what point does this get evaluated? Most bitbake > variables are expanded at usage time, not parse time but here, the > way > the parser works today, it would have to do an immediate expansion of > DISTRO_FEATURES to decide whether to include this file (or code > block). Doesn't this same argument apply to doing a conditional include of a file? When bitbake goes to resolve the file name while evaluating the AST, it has to evaluate DISTRO_FEATURES which might not be complete. If the conditional in an "if" statement were also evaluated when evaluating the AST, I believe the following snipet: require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } Would be (functionally) identical to something (sort of) like: if oe.utils.optional_includes(d, 'foo-feature:True'): <All of the content of bar.inc> Without requiring splitting the recipe content up into multiple files. > So ok, lets assume we change bitbake massively and defer the > expansion > somehow. What if foo.inc influences the contents of DISTRO_FEATURES? > Should it then "unparse" foo.inc if my-feature was removed? or error? > or silently ignore that? > > bitbake's main conditional today is through overrides and these do > allow a controlled delayed expansion of metadata in most cases. In > some > cases such as include and inherit statements there is still the > immediate expansion issue above but at least there aren't huge > changes > to the parser required to make it work so its the best of both > worlds. I was curious as to what it would it would actually take to make "if" statements like the one I described above work (and I wanted to learn more about the bitbake internals), so I did a proof of concept on GitHub: https://github.com/JPEWdev/poky/commit/998a00f122154bb509d22b412fba0773 97f6e433 It's actually not particularly terrible IMHO, but I'm sure it could be better. I can repost it to the bitbake mailing list as an RFC if you think that would be helpful. > > > One could even eliminate the separate inc file and simply put its > > contents under the conditional (as much fun as it seems to have to > > open > > a new file just to see what a recipe is doing with a distro > > feature...) > > > > It would also appear that this could make a lot of other things > > simpler as well (and may even negate the need to backfill > > DISTRO_FEATURES into overrides?) > > See if the above gives food for thought on that... > > The big problems are the corner cases. If we do add new syntax it > needs > to avoid these as we already have some pretty nasty ones, thankfully > most people don't hit them though. That's fine. I"m not particularly trying to say that an "if" statement is the magic bullet for corner cases, but I think it is equivalent functionality to conditional includes and more readable and maintainable for people writing recipes. Maybe that means DISTRO_FEATURES still need to become OVERRIDES, IDK. > > Cheers, > > Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 13:55 ` Joshua Watt @ 2017-06-08 14:33 ` Richard Purdie 2017-06-08 14:48 ` Patrick Ohly 2017-06-08 15:28 ` Joshua Watt 0 siblings, 2 replies; 42+ messages in thread From: Richard Purdie @ 2017-06-08 14:33 UTC (permalink / raw) To: Joshua Watt, Patrick Ohly, openembedded-core On Thu, 2017-06-08 at 08:55 -0500, Joshua Watt wrote: > On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote: > > > > On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote: > > > > > > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > > > > > > > > > > > > As discussed in the "[Openembedded-architecture] Yocto > > > > Compatible > > > > 2.0 > > > > + signature changes" mail thread, changes in a .bbappend cannot > > > > be > > > > done unconditionally. Making _append and _remove depend on > > > > overrides > > > > which get set based on DISTRO_FEATURES is one way of achieving > > > > this. > > > > > > > > The oe.utils.optional_includes() helper function has not been > > > > discussed before. It's an attempt to address concerns by > > > > developers > > > > that having to write code for (potentially complex) condition > > > > checking > > > > is error prone and hard to read. > > > I promise I'm not trying to start a flame war here, and perhaps > > > there > > > is history behind this that I'm not aware of but... > > > > > > Why doesn't bitbake support some sort of "if" statement? It seems > > > like most of what we are trying to do could be accomplished with > > > much > > > less fuss if one could simply do this in the bb file: > > > > > > if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d): > > > include foo.inc > > This wouldn't actually solve as much of the problem as you think it > > might at first glance and probably causes others, at least as I > > understand it (as someone who's worked on bitbake's override code). > > > > For example, at what point does this get evaluated? Most bitbake > > variables are expanded at usage time, not parse time but here, the > > way > > the parser works today, it would have to do an immediate expansion > > of > > DISTRO_FEATURES to decide whether to include this file (or code > > block). > Doesn't this same argument apply to doing a conditional include of a > file? When bitbake goes to resolve the file name while evaluating the > AST, it has to evaluate DISTRO_FEATURES which might not be complete. > If > the conditional in an "if" statement were also evaluated when > evaluating the AST, I believe the following snipet: > > require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } > > Would be (functionally) identical to something (sort of) like: > > if oe.utils.optional_includes(d, 'foo-feature:True'): > <All of the content of bar.inc> > > Without requiring splitting the recipe content up into multiple > files. I did say the problem applied to the require syntax, yes. Put another way, my big worry is that the if syntax will make people start to want if syntax for things other than include style operations and try and do other things other than "inclusion" type work with it. We'll also need to then start dealing with nesting and most likely other complications as well as pushing us to having to deal with immediate expansion problems. One of the strengths of the current syntax we have to day is that it makes most things possible but does try and encourage you to do things "the right way". In adding an if syntax like this I suspect we're on a path which won't lead to a good place. I appreciate this isn't an exact science answer :/. To recap on how we get here, there is a problem of selective content inclusion in distros/layers. Right now you tend to have to buy into everything in a layer or nothing. This is bad for usability and adoption of components in layers. Sometimes its not practical to separate everything into isolated layers. We've therefore tried to come up with a way of handling this adding minimal changes but allowing the configuration we need. We do need to try and limit the scope of the usage of this as there is a fundamental issue, namely immediate expansion. I know most users will not realise there is even a problem with this. *If* we limit the scope to DISTRO_FEATURES, we stand a reasonable chance of being able to limit the occasions a user runs into this. On the other hand, if we add a generic if syntax, encourage usage of any variable and so on I think we're setting ourselves up for failure. Patrick for example mentioned IMAGE_FEATURES. This one is fraught with problems since: a) Its a recipe level setting so using it in a base configuration context would end badly b) Users change this in a variety of places some of which would be bitten by the immediate expansion problem even just in recipe context So no, I really don't like the idea of the if syntax, attractive as it may look at first. Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 14:33 ` Richard Purdie @ 2017-06-08 14:48 ` Patrick Ohly 2017-06-08 15:28 ` Joshua Watt 1 sibling, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-08 14:48 UTC (permalink / raw) To: Richard Purdie; +Cc: openembedded-core On Thu, 2017-06-08 at 15:33 +0100, Richard Purdie wrote: > Patrick for example mentioned IMAGE_FEATURES. This one is fraught with > problems since: > > a) Its a recipe level setting so using it in a base configuration > context would end badly We cannot even rely on DISTRO_FEATURES in the middle of the base configuration construction; basically conditional includes in the base configuration are not reliable at all. > b) Users change this in a variety of places some of which would be > bitten by the immediate expansion problem even just in recipe context I agree. The same had already occurred to me, with a slightly different failure mode: c) base image recipe does the conditional include based on IMAGE_FEATURES, bbappend is used to change IMAGE_FEATURES => include done based on something other than the final IMAGE_FEATURES -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 14:33 ` Richard Purdie 2017-06-08 14:48 ` Patrick Ohly @ 2017-06-08 15:28 ` Joshua Watt 2017-06-08 19:31 ` Patrick Ohly 1 sibling, 1 reply; 42+ messages in thread From: Joshua Watt @ 2017-06-08 15:28 UTC (permalink / raw) To: Richard Purdie, Patrick Ohly, openembedded-core On Thu, 2017-06-08 at 15:33 +0100, Richard Purdie wrote: > On Thu, 2017-06-08 at 08:55 -0500, Joshua Watt wrote: > > On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote: > > > > > > On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote: > > > > > > > > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote: > > > > > > > > > > > > > > > As discussed in the "[Openembedded-architecture] Yocto > > > > > Compatible > > > > > 2.0 > > > > > + signature changes" mail thread, changes in a .bbappend > > > > > cannot > > > > > be > > > > > done unconditionally. Making _append and _remove depend on > > > > > overrides > > > > > which get set based on DISTRO_FEATURES is one way of > > > > > achieving > > > > > this. > > > > > > > > > > The oe.utils.optional_includes() helper function has not been > > > > > discussed before. It's an attempt to address concerns by > > > > > developers > > > > > that having to write code for (potentially complex) condition > > > > > checking > > > > > is error prone and hard to read. > > > > > > > > I promise I'm not trying to start a flame war here, and perhaps > > > > there > > > > is history behind this that I'm not aware of but... > > > > > > > > Why doesn't bitbake support some sort of "if" statement? It > > > > seems > > > > like most of what we are trying to do could be accomplished > > > > with > > > > much > > > > less fuss if one could simply do this in the bb file: > > > > > > > > if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d): > > > > include foo.inc > > > > > > This wouldn't actually solve as much of the problem as you think > > > it > > > might at first glance and probably causes others, at least as I > > > understand it (as someone who's worked on bitbake's override > > > code). > > > > > > For example, at what point does this get evaluated? Most bitbake > > > variables are expanded at usage time, not parse time but here, > > > the > > > way > > > the parser works today, it would have to do an immediate > > > expansion > > > of > > > DISTRO_FEATURES to decide whether to include this file (or code > > > block). > > > > Doesn't this same argument apply to doing a conditional include of > > a > > file? When bitbake goes to resolve the file name while evaluating > > the > > AST, it has to evaluate DISTRO_FEATURES which might not be > > complete. > > If > > the conditional in an "if" statement were also evaluated when > > evaluating the AST, I believe the following snipet: > > > > require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') } > > > > Would be (functionally) identical to something (sort of) like: > > > > if oe.utils.optional_includes(d, 'foo-feature:True'): > > <All of the content of bar.inc> > > > > Without requiring splitting the recipe content up into multiple > > files. > > I did say the problem applied to the require syntax, yes. > > Put another way, my big worry is that the if syntax will make people > start to want if syntax for things other than include style > operations > and try and do other things other than "inclusion" type work with it. > We'll also need to then start dealing with nesting and most likely > other complications as well as pushing us to having to deal with > immediate expansion problems. > > One of the strengths of the current syntax we have to day is that it > makes most things possible but does try and encourage you to do > things > "the right way". In adding an if syntax like this I suspect we're on > a > path which won't lead to a good place. I appreciate this isn't an > exact > science answer :/. > > > To recap on how we get here, there is a problem of selective content > inclusion in distros/layers. Right now you tend to have to buy into > everything in a layer or nothing. This is bad for usability and > adoption of components in layers. Sometimes its not practical to > separate everything into isolated layers. > > We've therefore tried to come up with a way of handling this adding > minimal changes but allowing the configuration we need. > > We do need to try and limit the scope of the usage of this as there > is > a fundamental issue, namely immediate expansion. I know most users > will > not realise there is even a problem with this. > > *If* we limit the scope to DISTRO_FEATURES, we stand a reasonable > chance of being able to limit the occasions a user runs into this. > > On the other hand, if we add a generic if syntax, encourage usage of > any variable and so on I think we're setting ourselves up for > failure. Sure. I wouldn't suggest using an if statement for "just anything", you can surely do terrible things that way. It would (by convention) be restricted to the same sorts of things that the conditional includes allow now. On a similar token, you can do the same sorts of terrible things with conditional includes as currently proposed because it has the same enforcement policy (i.e. "by convention"). On the other hand, perhaps the range of terrible things that can be done extends to more than just how you conditionally include something. *What* is conditionally included might also require some scrutiny. As you have alluded to, overrides are probably the best option for variables, so putting them in a conditional include file is probably not ideal. Forcing people to move the things that have to be conditional to a separate file might actually be detrimental in a number of ways: 1) It might encourage recipe writers to do more in the include file than they maybe should so that they don't have to make a plethora of files. 2) It might make it harder to verify that what the recipe writers did is correct since the context of what they are doing is removed from the parent recipe. IIRC the conditional syntax (if or conditional include) is really mostly needed for the parts of bitbake that don't allow overrides (addtask and such). If that is the desired restriction, it would not be difficult to have bitbake enforce that by only allowing the subset of things that don't support overrides to be in the body of a if statement. This would be more difficult with conditional includes unless some other bitbake syntax was added. > > Patrick for example mentioned IMAGE_FEATURES. This one is fraught > with > problems since: > > a) Its a recipe level setting so using it in a base configuration > context would end badly > > b) Users change this in a variety of places some of which would be > bitten by the immediate expansion problem even just in recipe context Yes, that is an annoying problem. But it is neither made better or worse by the decision to use if vs conditional include (since they have the same semantics). > > So no, I really don't like the idea of the if syntax, attractive as > it > may look at first. If that's the consensus, than I'm fine with that. From my perspective, conditional includes are just another (more difficult to use) form of an "if" statement, and making it difficult to do things conditionally doesn't necessarily make it better for anyone. > > Cheers, > > Richard > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 15:28 ` Joshua Watt @ 2017-06-08 19:31 ` Patrick Ohly 2017-06-09 8:12 ` Patrick Ohly 2017-06-09 13:50 ` Joshua Watt 0 siblings, 2 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-08 19:31 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote: > Sure. I wouldn't suggest using an if statement for "just anything", you > can surely do terrible things that way. It would (by convention) be > restricted to the same sorts of things that the conditional includes > allow now. On a similar token, you can do the same sorts of terrible > things with conditional includes as currently proposed because it has > the same enforcement policy (i.e. "by convention"). I'm starting to wonder whether this "convention" can be strengthened with additional warnings. The code which currently evaluates the include parameter could record in the datastore the original expression and what it evaluated to, then later when the recipe gets finalized, an event handler can check whether evaluating the expression still gives the same result. This would also be useful for "inherit". I remember struggling to understand why certain image type classes kept getting inherited despite changing IMAGE_FSTYPES - it turned out, that change had to be made earlier. That's neither an argument for nor against the "if" check - the same could be done for that. Just something that occurred to me. > On the other hand, perhaps the range of terrible things that can be > done extends to more than just how you conditionally include something. > *What* is conditionally included might also require some scrutiny. As > you have alluded to, overrides are probably the best option for > variables, so putting them in a conditional include file is probably > not ideal. Forcing people to move the things that have to be > conditional to a separate file might actually be detrimental in a > number of ways: > 1) It might encourage recipe writers to do more in the include file > than they maybe should so that they don't have to make a plethora of > files. > 2) It might make it harder to verify that what the recipe writers did > is correct since the context of what they are doing is removed from the > parent recipe. > > IIRC the conditional syntax (if or conditional include) is really > mostly needed for the parts of bitbake that don't allow overrides > (addtask and such). If that is the desired restriction, it would not be > difficult to have bitbake enforce that by only allowing the subset of > things that don't support overrides to be in the body of a if > statement. This would be more difficult with conditional includes > unless some other bitbake syntax was added. There's some truth to that IMHO, but I'm uncertain whether it warrants introducing entirely new syntax. In refkit, I only ran into one particular case were an include file was necessary. > If that's the consensus, than I'm fine with that. From my perspective, > conditional includes are just another (more difficult to use) form of > an "if" statement, and making it difficult to do things conditionally > doesn't necessarily make it better for anyone. Making it hard sends the message that it shouldn't be used lightly. Documentation will have to make clear that conditional includes are the last resort when everything else isn't usable. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 19:31 ` Patrick Ohly @ 2017-06-09 8:12 ` Patrick Ohly 2017-06-09 13:47 ` Joshua Watt 2017-08-24 9:27 ` Patrick Ohly 2017-06-09 13:50 ` Joshua Watt 1 sibling, 2 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 8:12 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote: > On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote: > > Sure. I wouldn't suggest using an if statement for "just anything", you > > can surely do terrible things that way. It would (by convention) be > > restricted to the same sorts of things that the conditional includes > > allow now. On a similar token, you can do the same sorts of terrible > > things with conditional includes as currently proposed because it has > > the same enforcement policy (i.e. "by convention"). > > I'm starting to wonder whether this "convention" can be strengthened > with additional warnings. The code which currently evaluates the include > parameter could record in the datastore the original expression and what > it evaluated to, then later when the recipe gets finalized, an event > handler can check whether evaluating the expression still gives the same > result. Below is a quick-and-dirty proof of concept. Example bmap-tools_ %.bbappend: FOO = "bmap-tools-deploy.inc" require ${FOO} FOO = "" Example warning: WARNING: .../bmap-tools/bmap-tools_3.2.bb: .../bmap-tools_%.bbappend:2: include/require/inherit "${FOO}" resulted in including "bmap-tools-deploy.inc" while parsing. Variables effecting the parameter changed later such that nothing would have been included at the end of the recipe. I found one false positive (LAYERDIR is set during parsing and unset afterwards) which the code below filters out. I also get for all recipes (i.e. the error is in the base configuration): meta/conf/bitbake.conf:752: include/require/inherit "conf/target/${TARGET_SYS}.conf" resulted in including "conf/target/x86_64-oe-linux.conf" while parsing. Variables effecting the parameter changed later such that "conf/target/x86_64-refkit-linux.conf" would have been included at the end of the recipe. None of these two files exist, so it doesn't make a difference. But is it really intended that a conf/target/${TARGET_SYS}.conf gets included that isn't the one for the final TARGET_SYS? That looks like a genuine bug to me. TARGET_SYS changes because TARGET_VENDOR gets set by the ${DISTRO}.conf, which gets included later. If we agree that such a message is useful, should it get added after merging the proposed bitbake enhancements or together with them? Doing it properly implies adding tests, and I do not have the time for that now. I'd prefer to add the proposed patches first so that they can be used, then add the warning before 2.4 gets released. Should this warning be entirely in bitbake or should it become part of OE's sanity.bbclass? The latter would have the advantage that it could be configured as fatal error. The downside is that bitbake needs to export data, which implies adding a semi-stable API. Do we want some suppression mechanism? Something like: require ${PARSE_TIME_EVALUATION}${FOO} where PARSE_TIME_EVALUATION is unset, but can be checked for in the code that normally would print the warning? diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py index dba4540f5eb..91db56197f0 100644 --- a/lib/bb/parse/ast.py +++ b/lib/bb/parse/ast.py @@ -50,14 +50,13 @@ class IncludeNode(AstNode): """ Include the file and evaluate the statements """ - s = data.expand(self.what_file) - logger.debug(2, "CONF %s:%s: including %s", self.filename, self.lineno, s) + logger.debug(2, "CONF %s:%s: including %s = %s", self.filename, self.lineno, self.what_file, data.expand(self.what_file)) # TODO: Cache those includes... maybe not here though if self.force: - bb.parse.ConfHandler.include(self.filename, s, self.lineno, data, "include required") + bb.parse.ConfHandler.include(self.filename, self.what_file, self.lineno, data, "include required") else: - bb.parse.ConfHandler.include(self.filename, s, self.lineno, data, False) + bb.parse.ConfHandler.include(self.filename, self.what_file, self.lineno, data, False) class ExportNode(AstNode): def __init__(self, filename, lineno, var): @@ -362,6 +361,18 @@ def finalize(fn, d, variant = None): d.setVar('BBINCLUDED', bb.parse.get_file_depends(d)) + parse_expansions = d.getVar('_PARSE_TIME_EXPANSIONS') + if parse_expansions: + def fns_name(fns): + if fns: + return '"%s"' % fns + else: + return 'nothing' + for parentfn, lineno, fns_unexpanded, fns in parse_expansions: + current_fns = d.expand(fns_unexpanded) + if fns != current_fns and '${LAYERDIR}' not in current_fns: + logger.warning('%s:%d: include/require/inherit "%s" resulted in including %s while parsing. Variables effecting the parameter changed later such that %s would have been included at the end of the recipe.' % (parentfn, lineno, fns_unexpanded, fns_name(fns), fns_name(current_fns))) + bb.event.fire(bb.event.RecipeParsed(fn), d) bb.event.set_handlers(saved_handlers) diff --git a/lib/bb/parse/parse_py/BBHandler.py b/lib/bb/parse/parse_py/BBHandler.py index fe918a41f34..923d210a3e8 100644 --- a/lib/bb/parse/parse_py/BBHandler.py +++ b/lib/bb/parse/parse_py/BBHandler.py @@ -61,6 +61,7 @@ def supports(fn, d): def inherit(files, fn, lineno, d): __inherit_cache = d.getVar('__inherit_cache', False) or [] files = d.expand(files).split() + # TODO: remember expansion result for file in files: if not os.path.isabs(file) and not file.endswith(".bbclass"): file = os.path.join('classes', '%s.bbclass' % file) diff --git a/lib/bb/parse/parse_py/ConfHandler.py b/lib/bb/parse/parse_py/ConfHandler.py index 97aa1304314..b4f0c61d256 100644 --- a/lib/bb/parse/parse_py/ConfHandler.py +++ b/lib/bb/parse/parse_py/ConfHandler.py @@ -75,8 +75,16 @@ def include(parentfn, fns, lineno, data, error_out): used in a ParseError that will be raised if the file to be included could not be included. Specify False to avoid raising an error in this case. """ + fns_unexpanded = fns fns = data.expand(fns) parentfn = data.expand(parentfn) + parse_expansions = data.getVar('_PARSE_TIME_EXPANSIONS') + if parse_expansions is None: + parse_expansions = [] + parse_expansions.append((parentfn, lineno, fns_unexpanded, fns)) + data.setVar('_PARSE_TIME_EXPANSIONS', parse_expansions) + if 'bmap-tools' in parentfn: + bb.note('bmap-tools parse_expansions: %s/%s = %s' % (id(data), id(parse_expansions), parse_expansions)) # "include" or "require" accept zero to n space-separated file names to include. for fn in fns.split(): -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-09 8:12 ` Patrick Ohly @ 2017-06-09 13:47 ` Joshua Watt 2017-06-09 14:11 ` Patrick Ohly 2017-08-24 9:27 ` Patrick Ohly 1 sibling, 1 reply; 42+ messages in thread From: Joshua Watt @ 2017-06-09 13:47 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Fri, 2017-06-09 at 10:12 +0200, Patrick Ohly wrote: > On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote: > > On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote: > > > Sure. I wouldn't suggest using an if statement for "just > > > anything", you > > > can surely do terrible things that way. It would (by convention) > > > be > > > restricted to the same sorts of things that the conditional > > > includes > > > allow now. On a similar token, you can do the same sorts of > > > terrible > > > things with conditional includes as currently proposed because it > > > has > > > the same enforcement policy (i.e. "by convention"). > > > > I'm starting to wonder whether this "convention" can be > > strengthened > > with additional warnings. The code which currently evaluates the > > include > > parameter could record in the datastore the original expression and > > what > > it evaluated to, then later when the recipe gets finalized, an > > event > > handler can check whether evaluating the expression still gives the > > same > > result. > > Below is a quick-and-dirty proof of concept. Example bmap-tools_ > %.bbappend: > > FOO = "bmap-tools-deploy.inc" > require ${FOO} > FOO = "" > > Example warning: > > WARNING: .../bmap-tools/bmap-tools_3.2.bb: .../bmap- > tools_%.bbappend:2: include/require/inherit "${FOO}" resulted in > including "bmap-tools-deploy.inc" while parsing. Variables effecting > the parameter changed later such that nothing would have been > included at the end of the recipe. I like it. The error message could maybe use a little bit of work... Maybe it needs some sort of short and unique key words in it, like "conditional invariance violation" to help direct people who copy the error into google when looking for an answer (or slightly better, grep the Mega Manual). > > I found one false positive (LAYERDIR is set during parsing and unset > afterwards) which the code below filters out. > > I also get for all recipes (i.e. the error is in the base > configuration): > > meta/conf/bitbake.conf:752: include/require/inherit > "conf/target/${TARGET_SYS}.conf" resulted in including > "conf/target/x86_64-oe-linux.conf" while parsing. Variables effecting > the parameter changed later such that "conf/target/x86_64-refkit- > linux.conf" would have been included at the end of the recipe. > > None of these two files exist, so it doesn't make a difference. But > is > it really intended that a conf/target/${TARGET_SYS}.conf gets > included > that isn't the one for the final TARGET_SYS? That looks like a > genuine > bug to me. > > TARGET_SYS changes because TARGET_VENDOR gets set by the > ${DISTRO}.conf, > which gets included later. > > If we agree that such a message is useful, should it get added after > merging the proposed bitbake enhancements or together with them? > Doing > it properly implies adding tests, and I do not have the time for that > now. I'd prefer to add the proposed patches first so that they can be > used, then add the warning before 2.4 gets released. > > Should this warning be entirely in bitbake or should it become part > of > OE's sanity.bbclass? The latter would have the advantage that it > could > be configured as fatal error. The downside is that bitbake needs to > export data, which implies adding a semi-stable API. A fatal error would be nice.... anything that was 2.0 compatible should be fatal, but I don't know if you would want it to make it fatal for non-2.0 compatible (i.e. existing) layers, as it might break some existing uses (or maybe we don't care about that?). > > Do we want some suppression mechanism? Something like: > > require ${PARSE_TIME_EVALUATION}${FOO} > > where PARSE_TIME_EVALUATION is unset, but can be checked for in the > code > that normally would print the warning? Is there a currently known valid use for such a thing? I'd just as soon add it once we know it is needed (otherwise, the top search result for "conditional invariance violation" will end up being some blog that says "Just prefix it with ${PARSE_TIME_EVALUATION} and everything will be fine" :) > > diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py > index dba4540f5eb..91db56197f0 100644 > --- a/lib/bb/parse/ast.py > +++ b/lib/bb/parse/ast.py > @@ -50,14 +50,13 @@ class IncludeNode(AstNode): > """ > Include the file and evaluate the statements > """ > - s = data.expand(self.what_file) > - logger.debug(2, "CONF %s:%s: including %s", self.filename, > self.lineno, s) > + logger.debug(2, "CONF %s:%s: including %s = %s", > self.filename, self.lineno, self.what_file, > data.expand(self.what_file)) > > # TODO: Cache those includes... maybe not here though > if self.force: > - bb.parse.ConfHandler.include(self.filename, s, > self.lineno, data, "include required") > + bb.parse.ConfHandler.include(self.filename, > self.what_file, self.lineno, data, "include required") > else: > - bb.parse.ConfHandler.include(self.filename, s, > self.lineno, data, False) > + bb.parse.ConfHandler.include(self.filename, > self.what_file, self.lineno, data, False) > > class ExportNode(AstNode): > def __init__(self, filename, lineno, var): > @@ -362,6 +361,18 @@ def finalize(fn, d, variant = None): > > d.setVar('BBINCLUDED', bb.parse.get_file_depends(d)) > > + parse_expansions = d.getVar('_PARSE_TIME_EXPANSIONS') > + if parse_expansions: > + def fns_name(fns): > + if fns: > + return '"%s"' % fns > + else: > + return 'nothing' > + for parentfn, lineno, fns_unexpanded, fns in > parse_expansions: > + current_fns = d.expand(fns_unexpanded) > + if fns != current_fns and '${LAYERDIR}' not in > current_fns: > + logger.warning('%s:%d: include/require/inherit "%s" > resulted in including %s while parsing. Variables effecting the > parameter changed later such that %s would have been included at the > end of the recipe.' % (parentfn, lineno, fns_unexpanded, > fns_name(fns), fns_name(current_fns))) > + > bb.event.fire(bb.event.RecipeParsed(fn), d) > bb.event.set_handlers(saved_handlers) > > diff --git a/lib/bb/parse/parse_py/BBHandler.py > b/lib/bb/parse/parse_py/BBHandler.py > index fe918a41f34..923d210a3e8 100644 > --- a/lib/bb/parse/parse_py/BBHandler.py > +++ b/lib/bb/parse/parse_py/BBHandler.py > @@ -61,6 +61,7 @@ def supports(fn, d): > def inherit(files, fn, lineno, d): > __inherit_cache = d.getVar('__inherit_cache', False) or [] > files = d.expand(files).split() > + # TODO: remember expansion result > for file in files: > if not os.path.isabs(file) and not > file.endswith(".bbclass"): > file = os.path.join('classes', '%s.bbclass' % file) > diff --git a/lib/bb/parse/parse_py/ConfHandler.py > b/lib/bb/parse/parse_py/ConfHandler.py > index 97aa1304314..b4f0c61d256 100644 > --- a/lib/bb/parse/parse_py/ConfHandler.py > +++ b/lib/bb/parse/parse_py/ConfHandler.py > @@ -75,8 +75,16 @@ def include(parentfn, fns, lineno, data, > error_out): > used in a ParseError that will be raised if the file to be > included could > not be included. Specify False to avoid raising an error in this > case. > """ > + fns_unexpanded = fns > fns = data.expand(fns) > parentfn = data.expand(parentfn) > + parse_expansions = data.getVar('_PARSE_TIME_EXPANSIONS') > + if parse_expansions is None: > + parse_expansions = [] > + parse_expansions.append((parentfn, lineno, fns_unexpanded, fns)) > + data.setVar('_PARSE_TIME_EXPANSIONS', parse_expansions) > + if 'bmap-tools' in parentfn: > + bb.note('bmap-tools parse_expansions: %s/%s = %s' % > (id(data), id(parse_expansions), parse_expansions)) > > # "include" or "require" accept zero to n space-separated file > names to include. > for fn in fns.split(): > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-09 13:47 ` Joshua Watt @ 2017-06-09 14:11 ` Patrick Ohly 2017-06-09 14:24 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 14:11 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Fri, 2017-06-09 at 08:47 -0500, Joshua Watt wrote: > On Fri, 2017-06-09 at 10:12 +0200, Patrick Ohly wrote: > > On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote: > > > On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote: > > > > Sure. I wouldn't suggest using an if statement for "just > > > > anything", you > > > > can surely do terrible things that way. It would (by convention) > > > > be > > > > restricted to the same sorts of things that the conditional > > > > includes > > > > allow now. On a similar token, you can do the same sorts of > > > > terrible > > > > things with conditional includes as currently proposed because it > > > > has > > > > the same enforcement policy (i.e. "by convention"). > > > > > > I'm starting to wonder whether this "convention" can be > > > strengthened > > > with additional warnings. The code which currently evaluates the > > > include > > > parameter could record in the datastore the original expression and > > > what > > > it evaluated to, then later when the recipe gets finalized, an > > > event > > > handler can check whether evaluating the expression still gives the > > > same > > > result. > > > > Below is a quick-and-dirty proof of concept. Example bmap-tools_ > > %.bbappend: > > > > FOO = "bmap-tools-deploy.inc" > > require ${FOO} > > FOO = "" > > > > Example warning: > > > > WARNING: .../bmap-tools/bmap-tools_3.2.bb: .../bmap- > > tools_%.bbappend:2: include/require/inherit "${FOO}" resulted in > > including "bmap-tools-deploy.inc" while parsing. Variables effecting > > the parameter changed later such that nothing would have been > > included at the end of the recipe. > > I like it. The error message could maybe use a little bit of work... > Maybe it needs some sort of short and unique key words in it, like > "conditional invariance violation" to help direct people who copy the > error into google when looking for an answer (or slightly better, grep > the Mega Manual). When doing it as sanity.bbclass check it would have a short ID string. > > Should this warning be entirely in bitbake or should it become part > > of > > OE's sanity.bbclass? The latter would have the advantage that it > > could > > be configured as fatal error. The downside is that bitbake needs to > > export data, which implies adding a semi-stable API. > > A fatal error would be nice.... anything that was 2.0 compatible should > be fatal, but I don't know if you would want it to make it fatal for > non-2.0 compatible (i.e. existing) layers, as it might break some > existing uses (or maybe we don't care about that?). It's independent of Yocto Compatible 2.0. The image type inheritance issue I mentioned is a basic usability problem, and this TARGET_SYS dependency looks like a genuine bug to me. > > Do we want some suppression mechanism? Something like: > > > > require ${PARSE_TIME_EVALUATION}${FOO} > > > > where PARSE_TIME_EVALUATION is unset, but can be checked for in the > > code > > that normally would print the warning? > > Is there a currently known valid use for such a thing? Using LAYERDIR is one, although that particular example is probably better caught by looking for that particular variable instead of annotating all places where it gets used. I'm unsure whether there are valid uses. I'm just worried that by the time someone shows up with one, it will be too late to add a suppression mechanism (as in "I've just updated from 2.3 to 2.4, how do I disable this warning?"). > I'd just as soon > add it once we know it is needed (otherwise, the top search result for > "conditional invariance violation" will end up being some blog that > says "Just prefix it with ${PARSE_TIME_EVALUATION} and everything will > be fine" :) I would add such an explanation to the error message itself and warn about using it, then hopefully no-one writes such stupid blog posts ;-} -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-09 14:11 ` Patrick Ohly @ 2017-06-09 14:24 ` Patrick Ohly 0 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 14:24 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Fri, 2017-06-09 at 16:11 +0200, Patrick Ohly wrote: > On Fri, 2017-06-09 at 08:47 -0500, Joshua Watt wrote: > > Is there a currently known valid use for such a thing? > > Using LAYERDIR is one, although that particular example is probably > better caught by looking for that particular variable instead of > annotating all places where it gets used. > > I'm unsure whether there are valid uses. Perhaps this one is a valid one: WARNING: .../libgcc-initial_6.3.bb:1: include/require/inherit "recipes-devtools/gcc/gcc-${PV}.inc" resulted in including "recipes-devtools/gcc/gcc-6.3.inc" while parsing. Variables effecting the parameter changed later such that "recipes-devtools/gcc/gcc-6.3.0.inc" would have been included at the end of the recipe. PV changes, but that must have been anticipated as it clearly works as intended. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-09 8:12 ` Patrick Ohly 2017-06-09 13:47 ` Joshua Watt @ 2017-08-24 9:27 ` Patrick Ohly 1 sibling, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-08-24 9:27 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Fri, 2017-06-09 at 10:12 +0200, Patrick Ohly wrote: > I also get for all recipes (i.e. the error is in the base > configuration): > > meta/conf/bitbake.conf:752: include/require/inherit "conf/target/${TARGET_SYS}.conf" resulted in including "conf/target/x86_64-oe-linux.conf" while parsing. Variables effecting the parameter changed later such that "conf/target/x86_64-refkit-linux.conf" would have been included at the end of the recipe. > > None of these two files exist, so it doesn't make a difference. But is > it really intended that a conf/target/${TARGET_SYS}.conf gets included > that isn't the one for the final TARGET_SYS? That looks like a genuine > bug to me. And Andre McCurdy also came to the same conclusion - see his "[OE-core] [PATCH] bitbake.conf: fix ineffective include conf/target/${TARGET_SYS}.conf" patch. However, that patch IMHO is incomplete, because it doesn't account for ${DISTRO}.conf changing the TARGET_VENDOR part of TARGET_SYS. I'll comment there, too. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-08 19:31 ` Patrick Ohly 2017-06-09 8:12 ` Patrick Ohly @ 2017-06-09 13:50 ` Joshua Watt 2017-06-09 14:04 ` Patrick Ohly 1 sibling, 1 reply; 42+ messages in thread From: Joshua Watt @ 2017-06-09 13:50 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote: > On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote: > > Sure. I wouldn't suggest using an if statement for "just anything", > > you > > can surely do terrible things that way. It would (by convention) be > > restricted to the same sorts of things that the conditional > > includes > > allow now. On a similar token, you can do the same sorts of > > terrible > > things with conditional includes as currently proposed because it > > has > > the same enforcement policy (i.e. "by convention"). > > I'm starting to wonder whether this "convention" can be strengthened > with additional warnings. The code which currently evaluates the > include > parameter could record in the datastore the original expression and > what > it evaluated to, then later when the recipe gets finalized, an event > handler can check whether evaluating the expression still gives the > same > result. > > This would also be useful for "inherit". I remember struggling to > understand why certain image type classes kept getting inherited > despite > changing IMAGE_FSTYPES - it turned out, that change had to be made > earlier. > > That's neither an argument for nor against the "if" check - the same > could be done for that. Just something that occurred to me. > > > On the other hand, perhaps the range of terrible things that can be > > done extends to more than just how you conditionally include > > something. > > *What* is conditionally included might also require some scrutiny. > > As > > you have alluded to, overrides are probably the best option for > > variables, so putting them in a conditional include file is > > probably > > not ideal. Forcing people to move the things that have to be > > conditional to a separate file might actually be detrimental in a > > number of ways: > > 1) It might encourage recipe writers to do more in the include > > file > > than they maybe should so that they don't have to make a plethora > > of > > files. > > 2) It might make it harder to verify that what the recipe writers > > did > > is correct since the context of what they are doing is removed from > > the > > parent recipe. > > > > IIRC the conditional syntax (if or conditional include) is really > > mostly needed for the parts of bitbake that don't allow overrides > > (addtask and such). If that is the desired restriction, it would > > not be > > difficult to have bitbake enforce that by only allowing the subset > > of > > things that don't support overrides to be in the body of a if > > statement. This would be more difficult with conditional includes > > unless some other bitbake syntax was added. > > There's some truth to that IMHO, but I'm uncertain whether it > warrants > introducing entirely new syntax. In refkit, I only ran into one > particular case were an include file was necessary. I'd be curious to see that. How big was the .inc file? > > > If that's the consensus, than I'm fine with that. From my > > perspective, > > conditional includes are just another (more difficult to use) form > > of > > an "if" statement, and making it difficult to do things > > conditionally > > doesn't necessarily make it better for anyone. > > Making it hard sends the message that it shouldn't be used lightly. > Documentation will have to make clear that conditional includes are > the > last resort when everything else isn't usable. > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Yocto Compatible 2.0 support code 2017-06-09 13:50 ` Joshua Watt @ 2017-06-09 14:04 ` Patrick Ohly 0 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 14:04 UTC (permalink / raw) To: Joshua Watt; +Cc: openembedded-core On Fri, 2017-06-09 at 08:50 -0500, Joshua Watt wrote: > On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote: > > There's some truth to that IMHO, but I'm uncertain whether it > > warrants > > introducing entirely new syntax. In refkit, I only ran into one > > particular case were an include file was necessary. > > I'd be curious to see that. How big was the .inc file? It's here: https://github.com/intel/intel-iot-refkit/blob/master/meta-refkit-core/recipes-devtools/bmap-tools/classes/bmap-tools-deploy.bbclass That's the code still using the "inherit" workaround. Not particularly large, but does things like "inherit deploy" and "addtask" which themselves then either require conditional inherit or anonymous python workarounds, so overall having them in a conditional file is cleaner. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 0/2] Yocto Compatible 2.0 support code 2017-06-07 15:31 [PATCH 0/2] Yocto Compatible 2.0 support code Patrick Ohly ` (2 preceding siblings ...) 2017-06-07 15:43 ` [PATCH 0/2] Yocto Compatible 2.0 support code Joshua Watt @ 2017-06-09 13:04 ` Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly ` (4 more replies) 3 siblings, 5 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 13:04 UTC (permalink / raw) To: openembedded-core As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 + signature changes" mail thread, changes in a .bbappend cannot be done unconditionally. Making _append and _remove depend on overrides which get set based on DISTRO_FEATURES is one way of achieving this. Conditional includes can have unexpected results when used with checks that still change during parsing. Helper functions might make this a bit safer and easier to review by limiting the check to DISTRO_FEATURES. Changes in V2: - added df- prefix to distro feature overrides - replaced too complex optional_includes() with simpler all/any_distro_features() Patrick Ohly (2): bitbake.conf: DISTRO_FEATURES as overrides utils.py: helper function for optional include files meta/conf/bitbake.conf | 17 ++++++++++++++++- meta/lib/oe/utils.py | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) base-commit: 186882ca62bf683b93cd7a250963921b89ba071f -- git-series 0.9.1 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly @ 2017-06-09 13:04 ` Patrick Ohly 2017-06-12 19:46 ` Denys Dmytriyenko 2017-06-09 13:04 ` [PATCH v2 2/2] utils.py: helper function for optional include files Patrick Ohly ` (3 subsequent siblings) 4 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 13:04 UTC (permalink / raw) To: openembedded-core As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 + signature changes", changes in .bbappend must depend on some explicit configuration change, typically selecting a distro feature. For _append and _remove, adding an override that is set only when the corresponding entry is in DISTRO_FEATURES achieves that: In local.conf: DISTRO_FEATURES_append = " my-distro-feature" In layer.conf: DISTRO_FEATURES_OVERRIDES += "my-distro-feature" In a .bbappend: do_install_append_df-my-distro-feature () { ... } The subset of DISTRO_FEATURES that are made available as overrides must be configured explicitly because using them this way should be a conscious decision. Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/conf/bitbake.conf | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf index 3ad905c..fce1882 100644 --- a/meta/conf/bitbake.conf +++ b/meta/conf/bitbake.conf @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded" # # This works for functions as well, they are really just environment variables. # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration. -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable" +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable" OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES" CLASSOVERRIDE ?= "class-target" DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}" @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE" FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}" +# Turns certain DISTRO_FEATURES into overrides of the same name +# or (optionally) some other name. Ensures that these special +# distro features remain set also for native and nativesdk +# recipes, so that these overrides can also be used there. +# +# Beware that this part of OVERRIDES changes during parsing, so usage +# of these overrides should be limited to .bb and .bbappend files, +# because then DISTRO_FEATURES is final. +DISTRO_FEATURES_OVERRIDES ??= "" +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ +Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES." +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" +DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" +DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" + ################################################################## # Include the rest of the config files. ################################################################## -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-09 13:04 ` [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly @ 2017-06-12 19:46 ` Denys Dmytriyenko 2017-06-12 21:05 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Denys Dmytriyenko @ 2017-06-12 19:46 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core This now breaks parsing my distro config on these lines: ENABLE_SYSVINIT ?= "0" DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}" Here's the log: ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py Traceback (most recent call last): File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'): except Exception as exc: > raise ExpansionError(varname, s, exc) from exc bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined -- Denys On Fri, Jun 09, 2017 at 03:04:03PM +0200, Patrick Ohly wrote: > As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 + > signature changes", changes in .bbappend must depend on some explicit > configuration change, typically selecting a distro feature. > > For _append and _remove, adding an override that is set only when the > corresponding entry is in DISTRO_FEATURES achieves that: > > In local.conf: > DISTRO_FEATURES_append = " my-distro-feature" > > In layer.conf: > DISTRO_FEATURES_OVERRIDES += "my-distro-feature" > > In a .bbappend: > do_install_append_df-my-distro-feature () { > ... > } > > The subset of DISTRO_FEATURES that are made available as overrides > must be configured explicitly because using them this way should > be a conscious decision. > > Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> > --- > meta/conf/bitbake.conf | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf > index 3ad905c..fce1882 100644 > --- a/meta/conf/bitbake.conf > +++ b/meta/conf/bitbake.conf > @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded" > # > # This works for functions as well, they are really just environment variables. > # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration. > -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable" > +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable" > OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES" > CLASSOVERRIDE ?= "class-target" > DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}" > @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE" > > FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}" > > +# Turns certain DISTRO_FEATURES into overrides of the same name > +# or (optionally) some other name. Ensures that these special > +# distro features remain set also for native and nativesdk > +# recipes, so that these overrides can also be used there. > +# > +# Beware that this part of OVERRIDES changes during parsing, so usage > +# of these overrides should be limited to .bb and .bbappend files, > +# because then DISTRO_FEATURES is final. > +DISTRO_FEATURES_OVERRIDES ??= "" > +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ > +Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES." > +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" > +DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" > +DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" > + > ################################################################## > # Include the rest of the config files. > ################################################################## > -- > git-series 0.9.1 > -- > _______________________________________________ > Openembedded-core mailing list > Openembedded-core@lists.openembedded.org > http://lists.openembedded.org/mailman/listinfo/openembedded-core ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-12 19:46 ` Denys Dmytriyenko @ 2017-06-12 21:05 ` Patrick Ohly 2017-06-12 23:23 ` Denys Dmytriyenko 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-12 21:05 UTC (permalink / raw) To: Denys Dmytriyenko; +Cc: openembedded-core On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote: > This now breaks parsing my distro config on these lines: > > ENABLE_SYSVINIT ?= "0" > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}" > > > Here's the log: > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py > Traceback (most recent call last): > File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'): > except Exception as exc: > > raise ExpansionError(varname, s, exc) from exc > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined base_conditional() seems to come from utils.bbclass, which gets inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this DISTRO_FEATURES_append end up getting expanded before these classes are fully parsed. I'll need to discuss this with Richard tomorrow. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-12 21:05 ` Patrick Ohly @ 2017-06-12 23:23 ` Denys Dmytriyenko 2017-06-13 7:14 ` Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Denys Dmytriyenko @ 2017-06-12 23:23 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote: > On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote: > > This now breaks parsing my distro config on these lines: > > > > ENABLE_SYSVINIT ?= "0" > > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}" > > > > > > Here's the log: > > > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py > > Traceback (most recent call last): > > File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'): > > except Exception as exc: > > > raise ExpansionError(varname, s, exc) from exc > > > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined > > base_conditional() seems to come from utils.bbclass, which gets > inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this > DISTRO_FEATURES_append end up getting expanded before these classes are > fully parsed. FWIW, replacing it with oe.utils.conditional() doesn't help. > I'll need to discuss this with Richard tomorrow. Richard mentioned this on IRC, but no solution yet. Please keep me posted. -- Denys ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-12 23:23 ` Denys Dmytriyenko @ 2017-06-13 7:14 ` Patrick Ohly 2017-06-13 8:06 ` Richard Purdie ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-13 7:14 UTC (permalink / raw) To: Denys Dmytriyenko; +Cc: openembedded-core On Mon, 2017-06-12 at 19:23 -0400, Denys Dmytriyenko wrote: > On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote: > > On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote: > > > This now breaks parsing my distro config on these lines: > > > > > > ENABLE_SYSVINIT ?= "0" > > > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}" > > > > > > > > > Here's the log: > > > > > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py > > > Traceback (most recent call last): > > > File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'): > > > except Exception as exc: > > > > raise ExpansionError(varname, s, exc) from exc > > > > > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined > > > > base_conditional() seems to come from utils.bbclass, which gets > > inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this > > DISTRO_FEATURES_append end up getting expanded before these classes are > > fully parsed. > > FWIW, replacing it with oe.utils.conditional() doesn't help. How about the following patch? It solves the problem for me. It also addresses some unease that I had when Richard proposed the distro overrides approach (cyclic dependency of "DISTRO_FEATURES depends on OVERRIDES which depends on DISTRO_FEATURES") by setting the overrides once for the DISTRO_FEATURES as they are set at the end of the base configuration. That's more deterministic; previously OVERRIDES could basically change randomly during base configuration parsing. I couldn't give a good example for this cyclic dependency at the time. Let me add that now. This code has an obvious cyclic dependency, and it indeed fails as I had expected: DISTRO_FEATURES_append = " ${@ bb.utils.contains('DISTRO_FEATURES', 'foo', 'bar', '', d) }" DISTRO_FEATURES_append = " foo" => bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES, expression was ${@ bb.utils.contains('DISTRO_FEATURES', 'foo', 'bar', '', d) } which triggered exception RuntimeError: maximum recursion depth exceeded In this code, the cyclic dependency is a bit more hidden, and depending on when things get evaluated, one gets foo and bar in DISTRO_FEATURES (for example, in "bitbake -e" and thus recipes): DISTRO_FEATURES_OVERRIDES += "bar" DISTRO_FEATURES_append = " bar" DISTRO_FEATURES_append_df-bar = " foo" That last example still works with the patch below, but I would argue that because of the conceptual issue (DISTRO_FEATURES depending on content of DISTRO_FEATURES) it cannot be expected to work. Once things get more complicated, it just fails in other unexpected ways: DISTRO_FEATURES_OVERRIDES += "bar foo" DISTRO_FEATURES_append = " bar" DISTRO_FEATURES_append_df-bar = " foo" This adds bar and foo to DISTRO_FEATURES, but only bar ends up in OVERRIDES when using the patch below. It was working with code that sets distro overrides dynamically. This might be seen as a drawback of the approach below, but as it only affects code that (IMHO) isn't valid, I'm not too worried about that. Let me also point out that the approach below completely sidesteps potential vardeps problems, because the dependency of OVERRIDES on DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My expectation is that OVERRIDES are part of the base hash and that thus tracking of how it gets computed isn't necessary. It seems to work in practice for me: DISTRO_FEATURES_append = " systemd" DISTRO_FEATURES_OVERRIDES += "bar" DISTRO_FEATURES_append = " bar" PACKAGECONFIG_append_pn-systemd_df-bar = " gcrypt" Changing either DISTRO_FEATURES_OVERRIDES or the DISTRO_FEATURES_append changes the signature of systemd:do_configure as expected. We are currently having a problem with the yocto-compat-layer.py signature check in refkit where it complains about: List of dependencies for variable DISTROOVERRIDES changed from '{'DISTRO'}' to '{'DISTRO_FEATURES', 'DISTRO', 'DISTROFEATURES2OVERRIDES'}' That's for a slightly older OE-core and the distrooverrides.bbclass approach. I don't know yet why that is now failing (it was working for OE-core 2.3), but my expectation is that the approach below would avoid it. diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass index 78926656d77..356aeba72ad 100644 --- a/meta/classes/base.bbclass +++ b/meta/classes/base.bbclass @@ -234,6 +234,11 @@ python base_eventhandler() { # Works with the line in layer.conf which changes PATH to point here setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS', d) setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS_NONFATAL', d, fatal=False) + # Base configuration complete, DISTRO_FEATURES finalized => convert selected features + # into overrides. + e.data.setVar('DISTROFEATURESOVERRIDES', + ''.join([':df-' + x for x in sorted(set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set(d.getVar('DISTRO_FEATURES').split()))])) + if isinstance(e, bb.event.BuildStarted): localdata = bb.data.createCopy(e.data) diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf index bc438cca826..6069130f488 100644 --- a/meta/conf/bitbake.conf +++ b/meta/conf/bitbake.conf @@ -727,15 +727,23 @@ FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDE # distro features remain set also for native and nativesdk # recipes, so that these overrides can also be used there. # -# Beware that this part of OVERRIDES changes during parsing, so usage -# of these overrides should be limited to .bb and .bbappend files, -# because then DISTRO_FEATURES is final. +# Beware that DISTRO_FEATURES change during parsing of the +# base configuration. These overrides can be used when setting +# variables in the base configuration, but variable expansion +# will only have the desired effect in recipes. +# +# Even worse, DISTRO_FEATURES might not be even usable during base +# configuration parsing. For example, embedded Python code with calls +# to base_conditional() can fail because the function is not defined. +# yet. Therefore these conditionals are set by base.bbclass only after +# parsing the base configuration is complete. DISTRO_FEATURES_OVERRIDES ??= "" DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES." DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" -DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" +# Initially empty, set once when DISTRO_FEATURES is stable. +DISTROFEATURESOVERRIDES = "" ################################################################## # Include the rest of the config files. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-13 7:14 ` Patrick Ohly @ 2017-06-13 8:06 ` Richard Purdie 2017-06-13 8:31 ` Patrick Ohly 2017-06-14 10:32 ` Patrick Ohly 2 siblings, 0 replies; 42+ messages in thread From: Richard Purdie @ 2017-06-13 8:06 UTC (permalink / raw) To: Patrick Ohly, Denys Dmytriyenko; +Cc: openembedded-core On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote: > Let me also point out that the approach below completely sidesteps > potential vardeps problems, because the dependency of OVERRIDES on > DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My > expectation is that OVERRIDES are part of the base hash and that thus > tracking of how it gets computed isn't necessary. I'm not sure what the best approach for the ordering issue is. The system is setup to dynamically adjust for OVERRIDES as they change. Perhaps setting it at the end of parsing is better, I'm not sure. Regardless, you're not quite right above when you say that OVERRIDES are part of the basehash, they're not in most cases. We actually need OVERRIDES to change without changing the basehash, for example when you switch MACHINE, you don't want allarch packages to rebuild (or tune shared packages with the same tune but a different MACHINE). For this reason, OVERRIDES are very much not part of the hash, we rely on computed values. Your patch has broken that in several places and I'm trying to untangle an unfortunate series of changes locally to resolve this behaviour correctly. Cheers, Richard ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-13 7:14 ` Patrick Ohly 2017-06-13 8:06 ` Richard Purdie @ 2017-06-13 8:31 ` Patrick Ohly 2017-06-14 10:32 ` Patrick Ohly 2 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-13 8:31 UTC (permalink / raw) To: Denys Dmytriyenko; +Cc: openembedded-core On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote: > Let me also point out that the approach below completely sidesteps > potential vardeps problems, because the dependency of OVERRIDES on > DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My expectation > is that OVERRIDES are part of the base hash and that thus tracking of > how it gets computed isn't necessary. It seems to work in practice for > me: > > DISTRO_FEATURES_append = " systemd" > DISTRO_FEATURES_OVERRIDES += "bar" > DISTRO_FEATURES_append = " bar" > PACKAGECONFIG_append_pn-systemd_df-bar = " gcrypt" > > Changing either DISTRO_FEATURES_OVERRIDES or the DISTRO_FEATURES_append > changes the signature of systemd:do_configure as expected. > > We are currently having a problem with the yocto-compat-layer.py > signature check in refkit where it complains about: > List of dependencies for variable DISTROOVERRIDES changed from > '{'DISTRO'}' to '{'DISTRO_FEATURES', 'DISTRO', > 'DISTROFEATURES2OVERRIDES'}' > > That's for a slightly older OE-core and the distrooverrides.bbclass > approach. I don't know yet why that is now failing (it was working for > OE-core 2.3), but my expectation is that the approach below would avoid > it. I've not figured out why it started failing. Because it's using the custom distrooverrides.bbclass approach I've not investigated further. What I was able to test now is the refkit_poky.TestRefkitPoky.test_refkit_conf_signature selftest in combination with OE-core/bitbake master (it wasn't working until recently because of the OEQA selftest changes that we hadn't adapted to yet). That test checks that activating the refkit distro configuration without using any of the refkit distro features doesn't change signatures. That's a slightly stronger guarantee than required by Yocto Compatible 2.0; it's useful because it makes it possible to add the refkit distro configuration without causing side effects when the developer doesn't use the features. When using the current bitbake.conf DISTRO_FEATURES_OVERRIDES support from OE-core master, that test fails because adding the refkit distro configuration needs to extend that variable, which shows up as (picking one example): zlib:do_package_qa: a124d530af2a0531e4c18780f9c75c15 -> 56da5c6a55996ec5a4817d1d4003e5ed bitbake-diffsigs --task zlib do_package_qa --signature a124d530af2a0531e4c18780f9c75c15 56da5c6a55996ec5a4817d1d4003e5ed basehash changed from 870d9161cf8d10dfae610cf55b54612b to 528a46f1120a679282ff8e55bf2791cb Variable DISTRO_FEATURES_OVERRIDES value changed from '' to ' refkit-config refkit-firewall refkit-computervision refkit-gateway ' This does not occur with the patch below. On IRC Richard corrected me and explained that it isn't OVERRIDES which gets tracked. My systemd example was working because variable values get recorded after applying overrides, so indirectly OVERRIDES and thus DISTRO_FEATURES_OVERRIDES+DISTRO_FEATURES change signatures. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides 2017-06-13 7:14 ` Patrick Ohly 2017-06-13 8:06 ` Richard Purdie 2017-06-13 8:31 ` Patrick Ohly @ 2017-06-14 10:32 ` Patrick Ohly 2017-06-14 10:33 ` [PATCH 1/2] Revert "bitbake.conf: DISTRO_FEATURES as overrides" Patrick Ohly 2 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-14 10:32 UTC (permalink / raw) To: Denys Dmytriyenko; +Cc: openembedded-core On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote: > On Mon, 2017-06-12 at 19:23 -0400, Denys Dmytriyenko wrote: > > On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote: > > > On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote: > > > > This now breaks parsing my distro config on these lines: > > > > > > > > ENABLE_SYSVINIT ?= "0" > > > > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}" > > > > > > > > > > > > Here's the log: > > > > > > > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py > > > > Traceback (most recent call last): > > > > File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'): > > > > except Exception as exc: > > > > > raise ExpansionError(varname, s, exc) from exc > > > > > > > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined > > > > > > base_conditional() seems to come from utils.bbclass, which gets > > > inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this > > > DISTRO_FEATURES_append end up getting expanded before these classes are > > > fully parsed. > > > > FWIW, replacing it with oe.utils.conditional() doesn't help. > > How about the following patch? It solves the problem for me. Richard had concerns about rewriting OVERRIDES in an event handler, therefore we agreed to use the .bbclass approach that I originally started with. I'm still testing that in refkit (blocked by trying to move towards bleeding edge OE-core master-next, on which my patch was based), but it should be ready at least for OE-core master-next, so I'll post it now. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] Revert "bitbake.conf: DISTRO_FEATURES as overrides" 2017-06-14 10:32 ` Patrick Ohly @ 2017-06-14 10:33 ` Patrick Ohly 2017-06-14 10:33 ` [PATCH 2/2] distrooverrides.bbclass: DISTRO_FEATURES as overrides Patrick Ohly 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-14 10:33 UTC (permalink / raw) To: openembedded-core This reverts commit 3b3ae91a22d6f685e804df4f32cdeebe1bd6bd88. It turned out that the code which expands DISTRO_FEATURES early during base config parsing can fail because some entries in DISTRO_FEATURES might call Python functions like base_conditional() from base.bbclass which aren't defined yet. A different solution will be needed. Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/conf/bitbake.conf | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf index 5d5ddec..80baec8 100644 --- a/meta/conf/bitbake.conf +++ b/meta/conf/bitbake.conf @@ -713,28 +713,13 @@ DISTRO_NAME ??= "OpenEmbedded" # # This works for functions as well, they are really just environment variables. # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration. -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable" +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable" CLASSOVERRIDE ?= "class-target" DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}" MACHINEOVERRIDES ?= "${MACHINE}" FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}" -# Turns certain DISTRO_FEATURES into overrides of the same name -# or (optionally) some other name. Ensures that these special -# distro features remain set also for native and nativesdk -# recipes, so that these overrides can also be used there. -# -# Beware that this part of OVERRIDES changes during parsing, so usage -# of these overrides should be limited to .bb and .bbappend files, -# because then DISTRO_FEATURES is final. -DISTRO_FEATURES_OVERRIDES ??= "" -DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ -Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES." -DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" -DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" -DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" - ################################################################## # Include the rest of the config files. ################################################################## base-commit: 6073813151383121f9fbd3dab9f33033ddf21ee2 -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] distrooverrides.bbclass: DISTRO_FEATURES as overrides 2017-06-14 10:33 ` [PATCH 1/2] Revert "bitbake.conf: DISTRO_FEATURES as overrides" Patrick Ohly @ 2017-06-14 10:33 ` Patrick Ohly 0 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-14 10:33 UTC (permalink / raw) To: openembedded-core This achieves the same goal as the same change to bitbake.conf itself, but because the class gets added later as part expanding INHERIT, this new approach is less likely to run into problems when DISTRO_FEATURES contains complex code. Another difference is that the class currently does not get inherited by default and thus is completely absent from a build unless some layer or include file adds it to INHERIT. Compared to the earlier code in bitbake.conf and a similar class in intel-iot-refkit, additional overrides now get sorted. This makes the final OVERRIDES more deterministic. The lessons learned about unintentionally depending on OVERRIDES are documented in the class because such problems are more likely to show up as unexpected signature differences when using this class. Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/classes/distrooverrides.bbclass | 32 +++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+) create mode 100644 meta/classes/distrooverrides.bbclass diff --git a/meta/classes/distrooverrides.bbclass b/meta/classes/distrooverrides.bbclass new file mode 100644 index 0000000..9a42712 --- /dev/null +++ b/meta/classes/distrooverrides.bbclass @@ -0,0 +1,32 @@ +# Turns certain DISTRO_FEATURES into overrides with the same +# name plus a df- prefix. Ensures that these special +# distro features remain set also for native and nativesdk +# recipes, so that these overrides can also be used there. +# +# This makes it simpler to write .bbappends that only change the +# task signatures of the recipe if the change is really enabled, +# for example with: +# do_install_append_df-my-feature () { ... } +# where "my-feature" is a DISTRO_FEATURE. +# +# The class is meant to be used in a layer.conf or distro +# .inc file with: +# INHERIT += "distrooverrides" +# DISTRO_FEATURES_OVERRIDES += "my-feature" +# +# Beware that this part of OVERRIDES changes during parsing, so usage +# of these overrides should be limited to .bb and .bbappend files, +# because then DISTRO_FEATURES is final. + +DISTRO_FEATURE_OVERRIDES ?= "" +DISTRO_FEATURE_OVERRIDES[doc] = "A space-separated list of <feature> entries. \ +Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES." + +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}" +DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}" + +# If DISTRO_FEATURES_OVERRIDES or DISTRO_FEATURES show up in a task +# signature because of this line, then the task dependency on +# OVERRIDES itself should be fixed. Excluding these two variables +# with DISTROOVERRIDES[vardepsexclude] would just work around the problem. +DISTROOVERRIDES .= "${@ ''.join([':df-' + x for x in sorted(set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }" -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 2/2] utils.py: helper function for optional include files 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly @ 2017-06-09 13:04 ` Patrick Ohly 2017-06-11 18:47 ` [PATCH v2 0/2] Yocto Compatible 2.0 support code Denys Dmytriyenko ` (2 subsequent siblings) 4 siblings, 0 replies; 42+ messages in thread From: Patrick Ohly @ 2017-06-09 13:04 UTC (permalink / raw) To: openembedded-core The main intention is to provide easy-to-use and read helper functions for including files only when certain distro features are set. Functionally they are the same as bb.utils.contains and bb.utils.contains_any. Distro features are part of the base configuration and thus safe to use for conditional inclusion in recipes and bbappends, in contrast to recipe variables which might still change during parsing. Therefore the check is limited to DISTRO_FEATURES. This is the reason for having this in OE-core instead of bitbake. Default values are set so that no redundant parameters have to be passed for conditional includes. As a secondary usage, the functions can also be used in boolean checks. Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> --- meta/lib/oe/utils.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/meta/lib/oe/utils.py b/meta/lib/oe/utils.py index 330a5ff..822d0cd 100644 --- a/meta/lib/oe/utils.py +++ b/meta/lib/oe/utils.py @@ -126,6 +126,46 @@ def features_backfill(var,d): if addfeatures: d.appendVar(var, " " + " ".join(addfeatures)) +def all_distro_features(d, features, truevalue="1", falsevalue=""): + """ + Returns truevalue if *all* given features are set in DISTRO_FEATURES, + else falsevalue. The features can be given as single string or anything + that can be turned into a set. + + This is a shorter, more flexible version of + bb.utils.contains("DISTRO_FEATURES", features, truevalue, falsevalue, d). + + Without explicit true/false values it can be used directly where + Python expects a boolean: + if oe.utils.all_distro_features(d, "foo bar"): + bb.fatal("foo and bar are mutually exclusive DISTRO_FEATURES") + + With just a truevalue, it can be used to include files that are meant to be + used only when requested via DISTRO_FEATURES: + require ${@ oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc") + """ + return bb.utils.contains("DISTRO_FEATURES", features, truevalue, falsevalue, d) + +def any_distro_features(d, features, truevalue="1", falsevalue=""): + """ + Returns truevalue if at least *one* of the given features is set in DISTRO_FEATURES, + else falsevalue. The features can be given as single string or anything + that can be turned into a set. + + This is a shorter, more flexible version of + bb.utils.contains_any("DISTRO_FEATURES", features, truevalue, falsevalue, d). + + Without explicit true/false values it can be used directly where + Python expects a boolean: + if not oe.utils.any_distro_features(d, "foo bar"): + bb.fatal("foo, bar or both must be set in DISTRO_FEATURES") + + With just a truevalue, it can be used to include files that are meant to be + used only when requested via DISTRO_FEATURES: + require ${@ oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc") + + """ + return bb.utils.contains_any("DISTRO_FEATURES", features, truevalue, falsevalue, d) def packages_filter_out_system(d): """ -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/2] Yocto Compatible 2.0 support code 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 2/2] utils.py: helper function for optional include files Patrick Ohly @ 2017-06-11 18:47 ` Denys Dmytriyenko 2017-06-12 6:22 ` Patrick Ohly 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2) Patchwork 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3) Patchwork 4 siblings, 1 reply; 42+ messages in thread From: Denys Dmytriyenko @ 2017-06-11 18:47 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote: > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 > + signature changes" mail thread, changes in a .bbappend cannot be > done unconditionally. Making _append and _remove depend on overrides > which get set based on DISTRO_FEATURES is one way of achieving this. Just wanted to confirm that this feature is going to be optional, right? > Conditional includes can have unexpected results when used with checks > that still change during parsing. Helper functions might make this a bit > safer and easier to review by limiting the check to DISTRO_FEATURES. > > Changes in V2: > - added df- prefix to distro feature overrides > - replaced too complex optional_includes() with simpler > all/any_distro_features() > > Patrick Ohly (2): > bitbake.conf: DISTRO_FEATURES as overrides > utils.py: helper function for optional include files > > meta/conf/bitbake.conf | 17 ++++++++++++++++- > meta/lib/oe/utils.py | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 1 deletion(-) > > base-commit: 186882ca62bf683b93cd7a250963921b89ba071f > -- > git-series 0.9.1 > -- > _______________________________________________ > Openembedded-core mailing list > Openembedded-core@lists.openembedded.org > http://lists.openembedded.org/mailman/listinfo/openembedded-core ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/2] Yocto Compatible 2.0 support code 2017-06-11 18:47 ` [PATCH v2 0/2] Yocto Compatible 2.0 support code Denys Dmytriyenko @ 2017-06-12 6:22 ` Patrick Ohly 2017-06-12 15:32 ` Denys Dmytriyenko 0 siblings, 1 reply; 42+ messages in thread From: Patrick Ohly @ 2017-06-12 6:22 UTC (permalink / raw) To: Denys Dmytriyenko; +Cc: openembedded-core On Sun, 2017-06-11 at 14:47 -0400, Denys Dmytriyenko wrote: > On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote: > > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 > > + signature changes" mail thread, changes in a .bbappend cannot be > > done unconditionally. Making _append and _remove depend on overrides > > which get set based on DISTRO_FEATURES is one way of achieving this. > > Just wanted to confirm that this feature is going to be optional, right? To achieve "Yocto Compatible 2.0" status, something like this will be needed. But not everyone needs (or wants) that, so those who don't are free to not use it, and it is off by default. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/2] Yocto Compatible 2.0 support code 2017-06-12 6:22 ` Patrick Ohly @ 2017-06-12 15:32 ` Denys Dmytriyenko 0 siblings, 0 replies; 42+ messages in thread From: Denys Dmytriyenko @ 2017-06-12 15:32 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core On Mon, Jun 12, 2017 at 08:22:45AM +0200, Patrick Ohly wrote: > On Sun, 2017-06-11 at 14:47 -0400, Denys Dmytriyenko wrote: > > On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote: > > > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0 > > > + signature changes" mail thread, changes in a .bbappend cannot be > > > done unconditionally. Making _append and _remove depend on overrides > > > which get set based on DISTRO_FEATURES is one way of achieving this. > > > > Just wanted to confirm that this feature is going to be optional, right? > > To achieve "Yocto Compatible 2.0" status, something like this will be > needed. But not everyone needs (or wants) that, so those who don't are > free to not use it, and it is off by default. Thanks for clarification. -- Denys ^ permalink raw reply [flat|nested] 42+ messages in thread
* ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2) 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly ` (2 preceding siblings ...) 2017-06-11 18:47 ` [PATCH v2 0/2] Yocto Compatible 2.0 support code Denys Dmytriyenko @ 2017-06-14 11:01 ` Patchwork 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3) Patchwork 4 siblings, 0 replies; 42+ messages in thread From: Patchwork @ 2017-06-14 11:01 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core == Series Details == Series: "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2) Revision: 2 URL : https://patchwork.openembedded.org/series/7163/ State : failure == Summary == Thank you for submitting this patch series to OpenEmbedded Core. This is an automated response. Several tests have been executed on the proposed series by patchtest resulting in the following failures: * Issue Series does not apply on top of target branch [test_series_merge_on_head] Suggested fix Rebase your series on top of targeted branch Targeted branch master (currently at 78041e20e4) If you believe any of these test results are incorrect, please reply to the mailing list (openembedded-core@lists.openembedded.org) raising your concerns. Otherwise we would appreciate you correcting the issues and submitting a new version of the patchset if applicable. Please ensure you add/increment the version number when sending the new version (i.e. [PATCH] -> [PATCH v2] -> [PATCH v3] -> ...). --- Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest Test suite: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe ^ permalink raw reply [flat|nested] 42+ messages in thread
* ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3) 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly ` (3 preceding siblings ...) 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2) Patchwork @ 2017-06-14 11:01 ` Patchwork 4 siblings, 0 replies; 42+ messages in thread From: Patchwork @ 2017-06-14 11:01 UTC (permalink / raw) To: Patrick Ohly; +Cc: openembedded-core == Series Details == Series: "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3) Revision: 3 URL : https://patchwork.openembedded.org/series/7163/ State : failure == Summary == Thank you for submitting this patch series to OpenEmbedded Core. This is an automated response. Several tests have been executed on the proposed series by patchtest resulting in the following failures: * Issue Series does not apply on top of target branch [test_series_merge_on_head] Suggested fix Rebase your series on top of targeted branch Targeted branch master (currently at 78041e20e4) If you believe any of these test results are incorrect, please reply to the mailing list (openembedded-core@lists.openembedded.org) raising your concerns. Otherwise we would appreciate you correcting the issues and submitting a new version of the patchset if applicable. Please ensure you add/increment the version number when sending the new version (i.e. [PATCH] -> [PATCH v2] -> [PATCH v3] -> ...). --- Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest Test suite: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-08-24 9:27 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-07 15:31 [PATCH 0/2] Yocto Compatible 2.0 support code Patrick Ohly 2017-06-07 15:31 ` [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly 2017-06-07 16:11 ` Peter Kjellerstedt 2017-06-08 6:04 ` Patrick Ohly 2017-06-08 10:45 ` Richard Purdie 2017-06-08 13:16 ` Peter Kjellerstedt 2017-06-08 14:38 ` Patrick Ohly 2017-06-07 15:31 ` [PATCH 2/2] utils.py: helper function for optional include files Patrick Ohly 2017-06-08 9:20 ` Richard Purdie 2017-06-08 14:36 ` Patrick Ohly 2017-06-09 10:02 ` Richard Purdie 2017-06-07 15:43 ` [PATCH 0/2] Yocto Compatible 2.0 support code Joshua Watt 2017-06-08 8:56 ` Richard Purdie 2017-06-08 13:55 ` Joshua Watt 2017-06-08 14:33 ` Richard Purdie 2017-06-08 14:48 ` Patrick Ohly 2017-06-08 15:28 ` Joshua Watt 2017-06-08 19:31 ` Patrick Ohly 2017-06-09 8:12 ` Patrick Ohly 2017-06-09 13:47 ` Joshua Watt 2017-06-09 14:11 ` Patrick Ohly 2017-06-09 14:24 ` Patrick Ohly 2017-08-24 9:27 ` Patrick Ohly 2017-06-09 13:50 ` Joshua Watt 2017-06-09 14:04 ` Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 " Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides Patrick Ohly 2017-06-12 19:46 ` Denys Dmytriyenko 2017-06-12 21:05 ` Patrick Ohly 2017-06-12 23:23 ` Denys Dmytriyenko 2017-06-13 7:14 ` Patrick Ohly 2017-06-13 8:06 ` Richard Purdie 2017-06-13 8:31 ` Patrick Ohly 2017-06-14 10:32 ` Patrick Ohly 2017-06-14 10:33 ` [PATCH 1/2] Revert "bitbake.conf: DISTRO_FEATURES as overrides" Patrick Ohly 2017-06-14 10:33 ` [PATCH 2/2] distrooverrides.bbclass: DISTRO_FEATURES as overrides Patrick Ohly 2017-06-09 13:04 ` [PATCH v2 2/2] utils.py: helper function for optional include files Patrick Ohly 2017-06-11 18:47 ` [PATCH v2 0/2] Yocto Compatible 2.0 support code Denys Dmytriyenko 2017-06-12 6:22 ` Patrick Ohly 2017-06-12 15:32 ` Denys Dmytriyenko 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2) Patchwork 2017-06-14 11:01 ` ✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3) Patchwork
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.