From: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
To: Saul Wold <sgw@bigsur.com>,
Richard Purdie <richard.purdie@linuxfoundation.org>,
"bitbake-devel@lists.openembedded.org"
<bitbake-devel@lists.openembedded.org>
Cc: "randy.macleod@windriver.com" <randy.macleod@windriver.com>
Subject: RE: [bitbake-devel] Underscore vs Dash in bbclass filenames
Date: Wed, 14 Feb 2024 05:52:53 +0000 [thread overview]
Message-ID: <DB5PR02MB102133EBD67873AE8C003947FEF4E2@DB5PR02MB10213.eurprd02.prod.outlook.com> (raw)
In-Reply-To: <58ab14ca-5a58-48dd-ac0f-bdc03c783f84@bigsur.com>
> -----Original Message-----
> From: bitbake-devel@lists.openembedded.org <bitbake-
> devel@lists.openembedded.org> On Behalf Of Saul Wold
> Sent: den 13 februari 2024 19:35
> To: Richard Purdie <richard.purdie@linuxfoundation.org>; bitbake-
> devel@lists.openembedded.org
> Cc: randy.macleod@windriver.com
> Subject: Re: [bitbake-devel] Underscore vs Dash in bbclass filenames
>
> Following up on this after the Tech Meeting today. The discussion
> centered around if any function names (python or shell) contain invalid
> characters such a dash (-) and issue a warning or error.
>
> The current __func_start_regexp__ in BBHandler.py is defined as follows:
>
> __func_start_regexp__ = re.compile(r"(((?P<py>python(?=(\s|\()))|(?P<fr>fakeroot(?=\s)))\s*)*(?P<func>[\w\.\-\+\{\}\$:]+)?\s*\(\s*\)\s*{$")
>
> Note the sub-pattern for the function name: (?P<func>[\w\.\-\+\{\}\$:]+)
> It includes '.', '-', '+' and '$' which are all invalid for shell and
> python function names. The \w expression which matches [a-zA-Z0-9_] is
> correct for both shell and python functions.
>
> If an invalid character is introduced in a function name with the a
> modified regular expression that removes the invalid characters the
> following error will occur:
>
> ERROR: Unable to parse
> /home/swold/src/yocto/poky/bitbake/lib/bb/parse/parse_py/ConfHandler.py
> Traceback (most recent call last):
> File
> "/home/swold/src/yocto/poky/bitbake/lib/bb/parse/parse_py/ConfHandler.py",
> line 200, in feeder(lineno=7, s='python dash-test-python-function() {',
> fn='/home/swold/src/yocto/poky/meta/classes/underscore_bbclass.bbclass',
> statements=[<bb.parse.ast.MethodNode object at 0x7fe6a2dbc700>,
> <bb.parse.ast.AddTaskNode object at 0x7fe6a2dbc5b0>], baseconfig=False,
> conffile=False):
>
> > raise ParseError("unparsed line: '%s'" % s, fn, lineno);
>
> bb.parse.ParseError: ParseError at
> /home/swold/src/yocto/poky/meta/classes/underscore_bbclass.bbclass:7:
> unparsed line: 'python dash-test-python-function() {'
>
> With that limited investigation, I realized that there are some 'class'
> specific functions like a do_write_config:append:class-target() or
> do_compile:class-native() which would fail the RE match() in BBHandler.py.
>
> I think more investigation is needed on how those are parsed and handled
> in bitbake (I admit to not being an expert on bitbake internals).
>
> Sau!
Going back in time, the following three commits added support for this:
0ded6c7ba6ba27c1fc0d1c7c2adbeda673ebd15c allow + and - in function names
8bf780c087df9dca19c9d16739731eca9f6e6bc9 tolerate ${...} in function names
3d7348c9eb7deebecce594f005f0019f9139e51c bitbake/lib/bb/parse/parse_py/BBHandler.py:
-Patch by pHilipp Zabel to allow dots
in the function names. This fixes bug #139. It seems right
to have dots in the packagename
(the commit messages were not very descriptive back then...)
Not sure what use `+` and `.` are, but `-` is definitely needed. Likewise
for ${...}, which, e.g., is used to define package specific functions such
as pkg_postinst:${PN}().
//Peter
> On 2/9/24 16:56, Saul Wold wrote:
> >
> >
> > On 2/9/24 10:05, Richard Purdie wrote:
> >> Hi Saul,
> >>
> >> Thanks for looking at this.
> >>
> > No Problem, I grabbed it at Randy's suggestion!
> >
> >> On Wed, 2024-02-07 at 14:21 -0800, Saul Wold wrote:
> >>> I was looking into Bug 14235 [0] and had initial patch based on
> Randy's
> >>> original recommendation of using dash (-) which was a basic
> >>> python/tinfoil script to check existing bbclasses in the BBPATH.
> >>>
> >>> It was pointed out that the bbclass name is prepended to exported
> >>> function and that would cause problems for SHELL functions as dash is
> >>> not allowed in the shell function name.
> >>>
> >>> On further investigation this of course becomes a more nuanced
> problem,
> >>> so I considered an alternative approach for setting up a warning in
> >>> bitbake itself. There is currently an error check in bitbake's ast.py
> >>> ExportFuncsNode() code. The code below would warnonce() for other
> >>> bbclasses that have a dash and are not in a NO_WARN_BBCLASSES list.
> >>>
> >>> At some point in the future this warning can be changed to an error.
> >>> I also noticed that the Regular Expressions defined in ast.py could be
> >>> tightened up slightly, there are a couple that use \s (general string)
> >>> vs \w which limits some special symbols, in order to limit dashes from
> >>> appearing in function names in bbclasses.
> >>
> >> Was this for python or shell functions or both? We should probably
> >> tighten the shell case at the very least.
> >>
> > I would consider it both.
> >
> > Oops, BBHandler.py has the re.compiles(), and I totally confused '\s' is
> > whitespace, not string! Not sure what I was thinking, I had a python
> > regex page open even!
> >
> > __export_func_regexp__ = re.compile(r"EXPORT_FUNCTIONS\s+(.+)" )
> > probably should use \w+ to grab the EXPORT_FUNCTIONS function names.
> >
> > __addtask_regexp__ =
> >
> re.compile(r"addtask\s+(?P<func>\w+)\s*((before\s*(?P<before>((.*(?=after)
> )|(.*))))|(after\s*(?P<after>((.*(?=before))|(.*)))))*")
> > Here the \w+ is used to capture the "addtask" function list.
> >
> > deltask_regexg uses .+ instead of the tighter \w+ also.
> >
> > If we do rename all dash bbclasses, then the inherit_regexp can change
> > from .+ to \w+ also I think, but that's would be future!
> >
> >>>
> >>> This is not for the current Scarthgap release.
> >>>
> >>> diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py
> >>> b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> >>> index cd1c998f8f8..faf6e9ede63 100644
> >>> --- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
> >>> +++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> >>> @@ -116,6 +116,9 @@ def handle(fn, d, include, baseconfig=False):
> >>> init(d)
> >>>
> >>> if ext == ".bbclass":
> >>> + nowarn_bbclass_list = d.getVar('NOWARN_DASH_BBCLASSES')
> >>> + if '-' in root and root not in nowarn_bbclass_list:
> >>> + bb.warnonce("%s contains dash ('-') in name which is not
> >>> safe" % base_name)
> >>> __classname__ = root
> >>> __inherit_cache = d.getVar('__inherit_cache', False) or []
> >>> if not fn in __inherit_cache:
> >>>
> >>>
> >>> Thoughts from the bitbake team on this approach?
> >>>
> >>>
> >>> [0] https://bugzilla.yoctoproject.org/show_bug.cgi?id=14235
> >>
> >> If we are going to warn about something, it would have to be the dash.
> >> This kind of patch would seem to be roughly the right way to handle it.
> >>
> >> My main question is whether we do want to force users through this
> >> transition or just stop new classes? Does the inconsistency really
> >> matter? Do we need to do anything, is there a problem we need to solve
> >> here?
> >>
> > If we try to stop new classes, we need to keep a list of "grandfathered"
> > classes which is the NOWARN_DASH_BBCLASSES variable in my proposal
> above.
> >
> > I can post this with an associated patch to oe-core and
> > meta-openembbeded layers, other layers would need to add their own
> > NOWARN_DASH_BBCLASSES += the extend the list.
> >
> > I am not sure if this would require a version bump which is why I think
> > it needs to wait until after Scarthgap.
> >
> >> If we do, do we support a set of "remapped" classes, where for example
> >> "inherit create-spdx" would translate into "inherit create_spdx" ? That
> >> does still have potential function name issues but they probably don't
> >> work today with dashes in and any class using those did already
> >> probably switch to an underscore.
> >>
> > Honestly, I would rather not try to deal with remapping classes or
> > function names. As you point out any bbclass that has a shell function
> > is already using either an underscore or single name (probably due to
> > the error check you added).
> >
> > This would just help make future naming more consistent with underscore
> > or single name as Randy points.
> >
> > If there is an opening on Tuesday and I make it, I will bring it.
> >
> > Sau!
> >
> >> Cheers,
> >>
> >> Richard
> >>
> >>
prev parent reply other threads:[~2024-02-14 5:53 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-07 22:21 Underscore vs Dash in bbclass filenames Saul Wold
2024-02-08 15:39 ` [bitbake-devel] " Enrico Scholz
2024-02-08 21:09 ` Peter Kjellerstedt
[not found] ` <17B1FF6690DC06E7.12768@lists.openembedded.org>
2024-02-08 21:16 ` Peter Kjellerstedt
2024-02-08 21:21 ` Sam Liddicott
2024-02-09 16:54 ` Richard Purdie
2024-02-09 18:05 ` Richard Purdie
2024-02-09 21:13 ` Randy MacLeod
2024-02-10 0:56 ` Saul Wold
2024-02-13 18:35 ` Saul Wold
2024-02-14 5:52 ` Peter Kjellerstedt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB5PR02MB102133EBD67873AE8C003947FEF4E2@DB5PR02MB10213.eurprd02.prod.outlook.com \
--to=peter.kjellerstedt@axis.com \
--cc=bitbake-devel@lists.openembedded.org \
--cc=randy.macleod@windriver.com \
--cc=richard.purdie@linuxfoundation.org \
--cc=sgw@bigsur.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).