| Message ID | 20241118162610.1423384-3-alex.kanavin@gmail.com |
|---|---|
| State | Accepted, archived |
| Commit | f687746703e7b096c5480668fd4f49bd4951182b |
| Headers | show |
| Series | [v3,1/4] bitbake-layers: ensure tinfoil.shutdown() gets executed when tinfoil.prepare() fails | expand |
Hi Alex, Thanks for working on this. It is close but there is one small issue needing to be resolved below. On Mon, 2024-11-18 at 17:26 +0100, Alexander Kanavin via lists.openembedded.org wrote: > From: Alexander Kanavin <alex@linutronix.de> > > It takes two parameters: > - location prefix for fragments > - name of variable that holds the list of enabled fragments, each of them prefixed > by layer id > > Implementation of this directive essentially expands the fragment list > obtained from the variable into absolute fragment paths and hands them to the > implementation of 'require'. > > Signed-off-by: Alexander Kanavin <alex@linutronix.de> > --- > lib/bb/parse/ast.py | 33 ++++++++++++++++++++++++++++ > lib/bb/parse/parse_py/ConfHandler.py | 6 +++++ > 2 files changed, 39 insertions(+) > > diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py > index 001ba8d289a..d3b7fef9895 100644 > --- a/lib/bb/parse/ast.py > +++ b/lib/bb/parse/ast.py > @@ -326,6 +326,34 @@ class InheritDeferredNode(AstNode): > inherits.append(self.inherit) > data.setVar('__BBDEFINHERITS', inherits) > > +class AddFragmentsNode(AstNode): > + def __init__(self, filename, lineno, fragments_path_prefix, fragments_variable): > + AstNode.__init__(self, filename, lineno) > + self.path = fragments_path_prefix > + self.variable = fragments_variable "path" in bitbake context becomes easily confusing. I think I'd prefer something like frag_pathprefix or something which makes it clear both that it isn't a full path and what kind of path it is. > + > + def eval(self, data): > + def find_fragment(layers, layerid, full_fragment_name): > + for layerpath in layers.split(): > + candidate_fragment_path = os.path.join(layerpath, full_fragment_name) > + if os.path.exists(candidate_fragment_path) and bb.utils.get_file_layer(candidate_fragment_path, data) == layerid: > + return candidate_fragment_path > + return None > + > + fragments = data.getVar(self.variable) > + layers = data.getVar('BBLAYERS') > + > + if not fragments: > + return > + for f in fragments.split(): > + layerid, fragment_name = f.split('/', 1) > + full_fragment_name = data.expand("{}/{}.conf".format(self.path, fragment_name)) > + fragment_path = find_fragment(layers, layerid, full_fragment_name) > + if fragment_path: > + bb.parse.ConfHandler.include(self.filename, fragment_path, self.lineno, data, "include fragment") > + else: > + bb.error("Could not find fragment {} in enabled layers: {}".format(f, layers)) > + Whilst checking if the file exists and only including if present sounds like the right thing to do, it needs to be a bit more complex. The issue is that bitbake needs to track both the files it accesses but also the files it looked for but didn't find. If those files are then created, we need to reparse. There are two ways to handle this, one is to trust that the bb.parse.ConfHandler code does already know how to do this and just call unconditionally until it matches a file. The apis might not let you easily know if it found a file. The other way is to call bb.parse.mark_dependency() and just mark the accessed files that way. Otherwise I think this looks good to me, thanks! Cheers, Richard
On Thu, 28 Nov 2024 at 13:54, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > Whilst checking if the file exists and only including if present sounds > like the right thing to do, it needs to be a bit more complex. The > issue is that bitbake needs to track both the files it accesses but > also the files it looked for but didn't find. If those files are then > created, we need to reparse. > > There are two ways to handle this, one is to trust that the > bb.parse.ConfHandler code does already know how to do this and just > call unconditionally until it matches a file. The apis might not let > you easily know if it found a file. > > The other way is to call bb.parse.mark_dependency() and just mark the > accessed files that way. After reading through usage of mark_dependency() in existing code, I need a bit more clarification there, because I'm not 100% sure you are correct, or alternatively I don't have complete understanding of how things work :) The original 'include some/file' directive iterates over BBPATH, and appends some/file to each of those, regardless of whether they exist, and marks them as dependency in case they appear later. This is fine. The fragments code takes a fragment name (which has a layer id as prefix), unambiguously converts that to a full file path that is checked for existence and hands it over to ConfHandler.include() (which calls mark_dependency() internally). If such conversion to full path fails, it calls bb.error, which immediately fails the whole thing. Would we want to still call mark_dependency() on a full path that does not exist in a case of such failure, and why? If the error is addressed and file is created, then bitbake will parse it anyway, no? Alex
On Thu, 2024-12-05 at 16:12 +0100, Alexander Kanavin wrote: > On Thu, 28 Nov 2024 at 13:54, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > Whilst checking if the file exists and only including if present sounds > > like the right thing to do, it needs to be a bit more complex. The > > issue is that bitbake needs to track both the files it accesses but > > also the files it looked for but didn't find. If those files are then > > created, we need to reparse. > > > > There are two ways to handle this, one is to trust that the > > bb.parse.ConfHandler code does already know how to do this and just > > call unconditionally until it matches a file. The apis might not let > > you easily know if it found a file. > > > > The other way is to call bb.parse.mark_dependency() and just mark the > > accessed files that way. > > After reading through usage of mark_dependency() in existing code, I > need a bit more clarification there, because I'm not 100% sure you are > correct, or alternatively I don't have complete understanding of how > things work :) > > The original 'include some/file' directive iterates over BBPATH, and > appends some/file to each of those, regardless of whether they exist, > and marks them as dependency in case they appear later. This is fine. > > The fragments code takes a fragment name (which has a layer id as > prefix), unambiguously converts that to a full file path that is > checked for existence and hands it over to ConfHandler.include() > (which calls mark_dependency() internally). If such conversion to full > path fails, it calls bb.error, which immediately fails the whole > thing. Would we want to still call mark_dependency() on a full path > that does not exist in a case of such failure, and why? If the error > is addressed and file is created, then bitbake will parse it anyway, > no? You're right, the loop in the fragment code is a little subtle. The code in question: + def find_fragment(layers, layerid, full_fragment_name): + for layerpath in layers.split(): + candidate_fragment_path = os.path.join(layerpath, full_fragment_name) + if os.path.exists(candidate_fragment_path) and bb.utils.get_file_layer(candidate_fragment_path, data) == layerid: + return candidate_fragment_path + return None looks like it loops checking for file existence but it is guarded by the layername matching check too. It shouldn't be possible for two layers to have the same layer name. I'm not sure how rigorous our checks are for that. For some reason I was thinking that fragments with and without a layer prefix were possible but that isn't the case. The patch is therefore probably ok but I am a bit worried about this potentially breaking in the future if someone else (or I!) miss the subtly in the code. Cheers, Richard
On Thu, 5 Dec 2024 at 16:28, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > The patch is therefore probably ok but I am a bit worried about this > potentially breaking in the future if someone else (or I!) miss the > subtly in the code. I still need to improve variable naming. I'll do that and send v2, but I'm not sure at the moment what can be done about this subtlety. Alex
On Thu, 2024-12-05 at 16:47 +0100, Alexander Kanavin wrote: > On Thu, 5 Dec 2024 at 16:28, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > > The patch is therefore probably ok but I am a bit worried about > > this > > potentially breaking in the future if someone else (or I!) miss the > > subtly in the code. > > I still need to improve variable naming. I'll do that and send v2, > but I'm not sure at the moment what can be done about this subtlety. Probably a comment: # No need to use mark_dependency since we would only match a fragment # from a specific layer and there can only be a single layer with a # given namespace. Cheers, Richard
On Thu, 5 Dec 2024 at 18:06, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > > I still need to improve variable naming. I'll do that and send v2, > > but I'm not sure at the moment what can be done about this subtlety. > > Probably a comment: > > # No need to use mark_dependency since we would only match a fragment > # from a specific layer and there can only be a single layer with a > # given namespace. Variables renamed in v4. This comment added in v5 :) Alex
diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py index 001ba8d289a..d3b7fef9895 100644 --- a/lib/bb/parse/ast.py +++ b/lib/bb/parse/ast.py @@ -326,6 +326,34 @@ class InheritDeferredNode(AstNode): inherits.append(self.inherit) data.setVar('__BBDEFINHERITS', inherits) +class AddFragmentsNode(AstNode): + def __init__(self, filename, lineno, fragments_path_prefix, fragments_variable): + AstNode.__init__(self, filename, lineno) + self.path = fragments_path_prefix + self.variable = fragments_variable + + def eval(self, data): + def find_fragment(layers, layerid, full_fragment_name): + for layerpath in layers.split(): + candidate_fragment_path = os.path.join(layerpath, full_fragment_name) + if os.path.exists(candidate_fragment_path) and bb.utils.get_file_layer(candidate_fragment_path, data) == layerid: + return candidate_fragment_path + return None + + fragments = data.getVar(self.variable) + layers = data.getVar('BBLAYERS') + + if not fragments: + return + for f in fragments.split(): + layerid, fragment_name = f.split('/', 1) + full_fragment_name = data.expand("{}/{}.conf".format(self.path, fragment_name)) + fragment_path = find_fragment(layers, layerid, full_fragment_name) + if fragment_path: + bb.parse.ConfHandler.include(self.filename, fragment_path, self.lineno, data, "include fragment") + else: + bb.error("Could not find fragment {} in enabled layers: {}".format(f, layers)) + def handleInclude(statements, filename, lineno, m, force): statements.append(IncludeNode(filename, lineno, m.group(1), force)) @@ -370,6 +398,11 @@ def handleInheritDeferred(statements, filename, lineno, m): classes = m.group(1) statements.append(InheritDeferredNode(filename, lineno, classes)) +def handleAddFragments(statements, filename, lineno, m): + fragments_path_prefix = m.group(1) + fragments_variable = m.group(2) + statements.append(AddFragmentsNode(filename, lineno, fragments_path_prefix, fragments_variable)) + def runAnonFuncs(d): code = [] for funcname in d.getVar("__BBANONFUNCS", False) or []: diff --git a/lib/bb/parse/parse_py/ConfHandler.py b/lib/bb/parse/parse_py/ConfHandler.py index 27665443dd4..35321dacfe1 100644 --- a/lib/bb/parse/parse_py/ConfHandler.py +++ b/lib/bb/parse/parse_py/ConfHandler.py @@ -47,6 +47,7 @@ __export_regexp__ = re.compile( r"export\s+([a-zA-Z0-9\-_+.${}/~]+)$" ) __unset_regexp__ = re.compile( r"unset\s+([a-zA-Z0-9\-_+.${}/~]+)$" ) __unset_flag_regexp__ = re.compile( r"unset\s+([a-zA-Z0-9\-_+.${}/~]+)\[([a-zA-Z0-9\-_+.][a-zA-Z0-9\-_+.@]+)\]$" ) __addpylib_regexp__ = re.compile(r"addpylib\s+(.+)\s+(.+)" ) +__addfragments_regexp__ = re.compile(r"addfragments\s+(.+)\s+(.+)" ) def init(data): return @@ -197,6 +198,11 @@ def feeder(lineno, s, fn, statements, baseconfig=False, conffile=True): ast.handlePyLib(statements, fn, lineno, m) return + m = __addfragments_regexp__.match(s) + if m: + ast.handleAddFragments(statements, fn, lineno, m) + return + raise ParseError("unparsed line: '%s'" % s, fn, lineno); # Add us to the handlers list