| Message ID | 20240509160843.3283186-1-alex.kanavin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitbake-config-build: add a plugin for config fragments | expand |
On Thu, 2024-05-09 at 18:08 +0200, Alexander Kanavin via lists.openembedded.org wrote: > From: Alexander Kanavin <alex@linutronix.de> > > This allows fine-tuning local configurations with pre-frabricated > configuration snippets in a structured, controlled way. It's also > an important building block for bitbake-setup. > > There are three operations (list/add/remove), and here's the list output: > > alex@Zen2:/srv/storage/alex/yocto/build-64$ bitbake-config-build list-fragments > NOTE: Starting bitbake server... > Available fragments in selftest layer located in /srv/work/alex/poky/meta-selftest: > > selftest/test-another-fragment This is a second configuration fragment intended for testing in oe-selftest context > selftest/test-fragment This is a configuration fragment intended for testing in oe-selftest context > > The tool requires that each fragment contains a one-line summary > at the top, followed by multiple lines of description, as hash-prefixed > comments. > > Signed-off-by: Alexander Kanavin <alex@linutronix.de> Firstly, thanks for putting this together, I'm very much in favour of adding something like this. The main questions I have are around the interface and naming. My feedback/open questions: a) I'm not totally sold on "bitbake-config-build" as the command name. I will give that a bit more thought. b) The code adding/removing require lines from local.conf seems a little fragile. I did wonder if we wanted a higher level bitbake API to this, for example a BB_CONF_FRAGMENTS variable which basically tagges a set of config files onto the end of the base configuration parsing? This wouldn't be hard to implement in bitbake and would then make the tooling easier and less fragile. c) Do we want to have the config fragment name as a "<layername>/<fragementname>" pair and have the fragments live directly in conf/fragments in a layer? d) How do you control ordering between the fragments? Do we need dependencies? How does layer ordering or dependencies affect anything? Can fragments conflict? e) You've used comments at the start of the files as summary/description. Should we use some kind of more variable like syntax which allows for other information in the future? Cheers, Richard
On Mon, 10 Jun 2024 at 12:08, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > b) The code adding/removing require lines from local.conf seems a > little fragile. I did wonder if we wanted a higher level bitbake API to > this, for example a BB_CONF_FRAGMENTS variable which basically tagges a > set of config files onto the end of the base configuration parsing? > This wouldn't be hard to implement in bitbake and would then make the > tooling easier and less fragile. I don't quite follow. If BB_CONF_FRAGMENTS value is a space separated list of fragments, then how would the tooling - add to it? - remove from it? It would still need to look for an assignment to that variable in a 'standard location' (e.g. conf/local.conf), and then rewrite the file with the new assignment. That doesn't seem any easier to me than just using 'require path/to/fragment'. On the other hand, there could be qa checks for presence of a certain fragment(s) in that variable. E.g. a layer could stop a build if it detects that something is not there. Another option is to copy/symlink the fragments into build/conf/fragments/ and have bitbake pick them up with a glob. > c) Do we want to have the config fragment name as a > "<layername>/<fragementname>" pair and have the fragments live directly > in conf/fragments in a layer? The idea is to allow deep slash-separated hierarchies here to be used as categories, and map their names directly to file paths (stripping the conf/fragments/ prefix and .inc suffix in the tooling UI). Think of something like meta-yocto-bsp/conf/fragments/machines/arm/beaglebone.inc that would be presented as 'machines/arm/beaglebone' in the UI. Conversely, what layer it's in is not relevant in the tooling UI. If someone wants to enable systemd, they wouldn't care which layer would give them that. This would come particularly handy in interactive UIs (e.g. something curses-driven and mimicking buildroot). > d) How do you control ordering between the fragments? Do we need > dependencies? How does layer ordering or dependencies affect anything? > Can fragments conflict? I deliberately kept all of this out of scope to avoid going crazy with thought experiments and resulting over-engineering. I feel we need to start using the fragments in practice, particularly eat our own dog food on the autobuilder with them, and then problems and answers to them will arise from that practice. > e) You've used comments at the start of the files as > summary/description. Should we use some kind of more variable like > syntax which allows for other information in the future? I'm open to proposals, but this was by far the easiest and most straightforward and obvious to anyone reading the fragment. Python has no support for 'metadata in comments' that we can reuse. If there's need for other metadata about fragments we can invent special syntax for it, but summary/description pair can stay as #-comments. Alex
On Mon, 2024-06-10 at 12:58 +0200, Alexander Kanavin wrote: > On Mon, 10 Jun 2024 at 12:08, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > > b) The code adding/removing require lines from local.conf seems a > > little fragile. I did wonder if we wanted a higher level bitbake > > API to > > this, for example a BB_CONF_FRAGMENTS variable which basically > > tagges a > > set of config files onto the end of the base configuration parsing? > > This wouldn't be hard to implement in bitbake and would then make > > the > > tooling easier and less fragile. > > I don't quite follow. If BB_CONF_FRAGMENTS value is a space separated > list of fragments, then how would the tooling > > - add to it? > - remove from it? bb.utils.edit_metadata()/bb.utils.edit_metadata_file() ? > It would still need to look for an assignment to that variable in a > 'standard location' (e.g. conf/local.conf), and then rewrite the file > with the new assignment. That doesn't seem any easier to me than just > using 'require path/to/fragment'. I can see this both ways but we do have the helper functions above that can do this (as used in recipetool/devtool). > On the other hand, there could be qa checks for presence of a certain > fragment(s) in that variable. E.g. a layer could stop a build if it > detects that something is not there. I'm wondering about whether having a variable makes it easier to users to understand/edit. I meant to also mention that I'm a bit concerned about using local.conf specifically in the tool, I'd prefer not to require a specific conf file. > Another option is to copy/symlink the fragments into > build/conf/fragments/ and have bitbake pick them up with a glob. I don't like the copy idea, the symlink approach is interesting. The main downside there is capturing the give config and ordering. > > c) Do we want to have the config fragment name as a > > "<layername>/<fragementname>" pair and have the fragments live > > directly > > in conf/fragments in a layer? > > The idea is to allow deep slash-separated hierarchies here to be used > as categories, and map their names directly to file paths (stripping > the conf/fragments/ prefix and .inc suffix in the tooling UI). Think > of something like > meta-yocto-bsp/conf/fragments/machines/arm/beaglebone.inc that would > be presented as 'machines/arm/beaglebone' in the UI. Conversely, what > layer it's in is not relevant in the tooling UI. If someone wants to > enable systemd, they wouldn't care which layer would give them that. That seems reasonable but I wanted to avoid every layer needing to nest by a directory level to make it clear where the fragments come from. I think some kind of naming standard might ultimately make things clearer to the user? Requiring the first entry be a layer name doesn't preclude the rest... > This would come particularly handy in interactive UIs (e.g. something > curses-driven and mimicking buildroot). > > > d) How do you control ordering between the fragments? Do we need > > dependencies? How does layer ordering or dependencies affect > > anything? > > Can fragments conflict? > > I deliberately kept all of this out of scope to avoid going crazy > with thought experiments and resulting over-engineering. I feel we > need to start using the fragments in practice, particularly eat our > own dog food on the autobuilder with them, and then problems and > answers to them will arise from that practice. I ask the questions since we have been trapped by choices in the past and it since these are likely things which will arise, we might want to at least consider them now. > > e) You've used comments at the start of the files as > > summary/description. Should we use some kind of more variable like > > syntax which allows for other information in the future? > > I'm open to proposals, but this was by far the easiest and most > straightforward and obvious to anyone reading the fragment. Python > has no support for 'metadata in comments' that we can reuse. If > there's need for other metadata about fragments we can invent special > syntax for it, but summary/description pair can stay as #-comments. We do have our parser which can easily handle: CONFFRAG[summary] = "some summary" CONFFRAG[description] = "Longer description with \ multiple \ lines \ " or CONFFRAGS += "myfragment_name" myfragment_name[summary] = "some summary" myfragment_name[description] = "Longer description with \ multiple \ lines \ " both of which have some pros and cons. I'm not sold on either of the above but I wanted to give some examples. For examples on handling such files see lib/bb/tests/parse.py, basically call bb.parse.handle(f.name, self.d)['']. Cheers, Richard
On Mon, 10 Jun 2024 at 16:51, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > > I don't quite follow. If BB_CONF_FRAGMENTS value is a space separated > > list of fragments, then how would the tooling > > > > - add to it? > > - remove from it? > > bb.utils.edit_metadata()/bb.utils.edit_metadata_file() ? Right, I had no idea this exists :) > I'm wondering about whether having a variable makes it easier to users > to understand/edit. I think there's benefit in having a variable that makes it separate and distinct from regular 'require' statements, but I can't form a clear preference for either option. Should this be taken to some kind of decision forum, e.g. oe-architecture or TSC? > I meant to also mention that I'm a bit concerned about using local.conf > specifically in the tool, I'd prefer not to require a specific conf > file. I agree that local.conf should be left alone. It's something coming from templates and I would rather never touch it with tools after the fact. But either the 'require' statements or the variable containing the fragment list has to be written into something that bitbake can see. Should they go to build/conf/fragments.conf ? > > Another option is to copy/symlink the fragments into > > build/conf/fragments/ and have bitbake pick them up with a glob. > > I don't like the copy idea, the symlink approach is interesting. The > main downside there is capturing the give config and ordering. 'give config'? > That seems reasonable but I wanted to avoid every layer needing to nest > by a directory level to make it clear where the fragments come from. I > think some kind of naming standard might ultimately make things clearer > to the user? > > Requiring the first entry be a layer name doesn't preclude the rest... Okay, I'll rework to include the layer name upfront. > I ask the questions since we have been trapped by choices in the past > and it since these are likely things which will arise, we might want to > at least consider them now. Hopefully the current code makes no choices for these things. It just adds/removes requested fragments, nothing else. But it can be extended to read fragment metadata, and armed with that (and the list of fragments already enabled) make decisions about what the new list should be, or whether the request should be declined. > We do have our parser which can easily handle: > > CONFFRAG[summary] = "some summary" > CONFFRAG[description] = "Longer description with \ > multiple \ > lines \ > " I must be missing something here. If this is defined in two different fragments and both are included, how would one not step on the other with these assignments? > or > > CONFFRAGS += "myfragment_name" > > myfragment_name[summary] = "some summary" > myfragment_name[description] = "Longer description with \ > multiple \ > lines \ > " Okay, this one in my opinion is a big readability (and write-ability) regression compared to #-comments. For extended metadata (such as dependencies/conflicts) something like this would be needed, but I'd rather not use it for summary/description. Alex
On Tue, 2024-06-11 at 12:45 +0200, Alexander Kanavin wrote: > On Mon, 10 Jun 2024 at 16:51, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > > I don't quite follow. If BB_CONF_FRAGMENTS value is a space separated > > > list of fragments, then how would the tooling > > > > > > - add to it? > > > - remove from it? > > > > bb.utils.edit_metadata()/bb.utils.edit_metadata_file() ? > > Right, I had no idea this exists :) > > > I'm wondering about whether having a variable makes it easier to users > > to understand/edit. > > I think there's benefit in having a variable that makes it separate > and distinct from regular 'require' statements, but I can't form a > clear preference for either option. Should this be taken to some kind > of decision forum, e.g. oe-architecture or TSC? oe-arch is the right place to discuss with the OE TSC being the ultimate decision maker if needed. > > I meant to also mention that I'm a bit concerned about using local.conf > > specifically in the tool, I'd prefer not to require a specific conf > > file. > > I agree that local.conf should be left alone. It's something coming > from templates and I would rather never touch it with tools after the > fact. > > But either the 'require' statements or the variable containing the > fragment list has to be written into something that bitbake can see. > Should they go to build/conf/fragments.conf ? I'd wondered about a dedicated file, yes. Perhaps with locked sigs also becoming a dedicated conf file for similar reasons. > > > Another option is to copy/symlink the fragments into > > > build/conf/fragments/ and have bitbake pick them up with a glob. > > > > I don't like the copy idea, the symlink approach is interesting. The > > main downside there is capturing the give config and ordering. > > 'give config'? Sorry, it was meant to read "given config". > > > That seems reasonable but I wanted to avoid every layer needing to nest > > by a directory level to make it clear where the fragments come from. I > > think some kind of naming standard might ultimately make things clearer > > to the user? > > > > Requiring the first entry be a layer name doesn't preclude the rest... > > Okay, I'll rework to include the layer name upfront. > > > I ask the questions since we have been trapped by choices in the past > > and it since these are likely things which will arise, we might want to > > at least consider them now. > > Hopefully the current code makes no choices for these things. It just > adds/removes requested fragments, nothing else. But it can be extended > to read fragment metadata, and armed with that (and the list of > fragments already enabled) make decisions about what the new list > should be, or whether the request should be declined. I'd like to explore these issues a little bit further now before we lock in on a format as I think their need is probably inevitable. > > > We do have our parser which can easily handle: > > > > CONFFRAG[summary] = "some summary" > > CONFFRAG[description] = "Longer description with \ > > multiple \ > > lines \ > > " > > I must be missing something here. If this is defined in two different > fragments and both are included, how would one not step on the other > with these assignments? They would overwrite each other but you can still parse the data from each file in turn. layer.conf files did give bitbake a similar challenge. It gives more readability at the cost of not being able to store all the data in the same data store. > > or > > > > CONFFRAGS += "myfragment_name" > > > > myfragment_name[summary] = "some summary" > > myfragment_name[description] = "Longer description with \ > > multiple \ > > lines \ > > " > > Okay, this one in my opinion is a big readability (and write-ability) > regression compared to #-comments. For extended metadata (such as > dependencies/conflicts) something like this would be needed, but I'd > rather not use it for summary/description. I'm not happy with the above either but I do think we should fine a format which is parsable as I really dislike 'magic' comments (similarly in machine conf files too). They don't scale and have machine readability issues. I'm open to other other ideas. I'd even consider some kind of new syntax if needed and we can't make the existing syntax work. Cheers, Richard
On Tue, 11 Jun 2024 at 12:56, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > oe-arch is the right place to discuss with the OE TSC being the > ultimate decision maker if needed. I've written a summary reply to oe-arch so that we can continue there. Alex
diff --git a/meta-selftest/conf/fragments/selftest/test-another-fragment.inc b/meta-selftest/conf/fragments/selftest/test-another-fragment.inc new file mode 100644 index 00000000000..4f6c535de2d --- /dev/null +++ b/meta-selftest/conf/fragments/selftest/test-another-fragment.inc @@ -0,0 +1,4 @@ +# This is a second configuration fragment intended for testing in oe-selftest context +# +# It defines another variable that can be checked inside the test. +SELFTEST_FRAGMENT_ANOTHER_VARIABLE = "someothervalue" diff --git a/meta-selftest/conf/fragments/selftest/test-fragment.inc b/meta-selftest/conf/fragments/selftest/test-fragment.inc new file mode 100644 index 00000000000..0ad62db9e13 --- /dev/null +++ b/meta-selftest/conf/fragments/selftest/test-fragment.inc @@ -0,0 +1,4 @@ +# This is a configuration fragment intended for testing in oe-selftest context +# +# It defines a variable that can be checked inside the test. +SELFTEST_FRAGMENT_VARIABLE = "somevalue" diff --git a/meta/lib/bbconfigbuild/configfragments.py b/meta/lib/bbconfigbuild/configfragments.py new file mode 100644 index 00000000000..d7e4f54dba2 --- /dev/null +++ b/meta/lib/bbconfigbuild/configfragments.py @@ -0,0 +1,117 @@ +# +# Copyright OpenEmbedded Contributors +# +# SPDX-License-Identifier: GPL-2.0-only +# + +import logging +import os +import sys + +import bb.utils + +from bblayers.common import LayerPlugin + +logger = logging.getLogger('bitbake-config-layers') + +sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) + +def plugin_init(plugins): + return ConfigFragmentsPlugin() + +class ConfigFragmentsPlugin(LayerPlugin): + def get_fragment_info(self, path): + summary = "" + description = [] + with open(path) as f: + for l in f.readlines(): + if not l.startswith('#'): + break + if not summary: + summary = l[1:].strip() + else: + description.append(l[1:].strip()) + if not summary or not description: + raise Exception('Please add a one-line summary followed by a description as #-prefixed comments at the beginning of {}'.format(path)) + + return summary, description + + + def discover_fragments(self): + allfragments = {} + for layername in self.bbfile_collections: + layerdir = self.bbfile_collections[layername] + fragments = [] + for topdir, dirs, files in os.walk(os.path.join(layerdir, 'conf/fragments')): + fragmentdir = topdir.split('conf/fragments/')[-1] + for fragmentfile in sorted(files): + fragmentname = "/".join((fragmentdir, fragmentfile.split('.')[0])) + fragmentpath = os.path.join(topdir, fragmentfile) + fragmentsummary, fragmentdesc = self.get_fragment_info(fragmentpath) + fragments.append({'path':fragmentpath, 'name':fragmentname, 'summary':fragmentsummary, 'description':fragmentdesc}) + if fragments: + allfragments[layername] = {'layerdir':layerdir,'fragments':fragments} + return allfragments + + + def do_list_fragments(self, args): + """ List available configuration fragments """ + for layername, layerdata in self.discover_fragments().items(): + layerdir = layerdata['layerdir'] + fragments = layerdata['fragments'] + + print('Available fragments in {} layer located in {}:\n'.format(layername, layerdir)) + for f in fragments: + if not args.verbose: + print('{}\t{}'.format(f['name'], f['summary'])) + else: + print('Name: {}\nPath: {}\nSummary: {}\nDescription:\n{}\n'.format(f['name'], f['path'], f['summary'],''.join(f['description']))) + print('') + + def fragment_exists(self, fragmentname): + for layername, layerdata in self.discover_fragments().items(): + for f in layerdata['fragments']: + if f['name'] == fragmentname: + return True + return False + + def do_add_fragment(self, args): + """ Add a fragment to the local build configuration """ + if not self.fragment_exists(args.fragmentname): + raise Exception("Fragment {} does not exist; use 'list-fragments' to see the full list.".format(args.fragmentname)) + + confpath = os.path.join(os.environ["BBPATH"], "conf/local.conf") + appendline = "require conf/fragments/{}.inc\n".format(args.fragmentname) + + with open(confpath) as f: + lines = f.readlines() + for l in lines: + if l == appendline: + print("Fragment {} already included in {}".format(args.fragmentname, confpath)) + return + + lines.append(appendline) + with open(confpath, 'w') as f: + f.write(''.join(lines)) + + def do_remove_fragment(self, args): + """ Remove a fragment from the local build configuration """ + confpath = os.path.join(os.environ["BBPATH"], "conf/local.conf") + appendline = "require conf/fragments/{}.inc\n".format(args.fragmentname) + + with open(confpath) as f: + lines = f.readlines() + lines = [l for l in lines if l != appendline] + + with open(confpath, 'w') as f: + f.write(''.join(lines)) + + def register_commands(self, sp): + parser_list_fragments = self.add_command(sp, 'list-fragments', self.do_list_fragments, parserecipes=False) + parser_list_fragments.add_argument('--verbose', '-v', action='store_true', help='Print extended descriptions of the fragments') + + parser_add_fragment = self.add_command(sp, 'add-fragment', self.do_add_fragment, parserecipes=False) + parser_add_fragment.add_argument('fragmentname', help='The name of the fragment (use list-fragments to see them)') + + parser_remove_fragment = self.add_command(sp, 'remove-fragment', self.do_remove_fragment, parserecipes=False) + parser_remove_fragment.add_argument('fragmentname', help='The name of the fragment') diff --git a/meta/lib/oeqa/selftest/cases/bblayers.py b/meta/lib/oeqa/selftest/cases/bblayers.py index 8b2bc319d50..9e75ef6928e 100644 --- a/meta/lib/oeqa/selftest/cases/bblayers.py +++ b/meta/lib/oeqa/selftest/cases/bblayers.py @@ -253,3 +253,24 @@ class BitbakeLayers(OESelftestTestCase): meta_selftest_found = True self.assertTrue(oe_core_found, "meta/conf/layer.conf not found in {}".format(testcopydir)) self.assertTrue(meta_selftest_found, "meta-selftest/conf/layer.conf not found in {}".format(testcopydir)) + +class BitbakeConfigBuild(OESelftestTestCase): + def test_add_remove_fragments(self): + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_VARIABLE'), None) + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_ANOTHER_VARIABLE'), None) + + runCmd('bitbake-config-build add-fragment selftest/test-fragment') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_VARIABLE'), 'somevalue') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_ANOTHER_VARIABLE'), None) + + runCmd('bitbake-config-build add-fragment selftest/test-another-fragment') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_VARIABLE'), 'somevalue') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_ANOTHER_VARIABLE'), 'someothervalue') + + runCmd('bitbake-config-build remove-fragment selftest/test-fragment') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_VARIABLE'), None) + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_ANOTHER_VARIABLE'), 'someothervalue') + + runCmd('bitbake-config-build remove-fragment selftest/test-another-fragment') + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_VARIABLE'), None) + self.assertEqual(get_bb_var('SELFTEST_FRAGMENT_ANOTHER_VARIABLE'), None)