| Message ID | 20260804090834.99906-1-jaipaul.cheernam@est.tech |
|---|---|
| State | New |
| Headers | show |
| Series | parse: warn on trailing whitespace after line continuation backslash | expand |
On Tue, 2026-08-04 at 11:08 +0200, Jaipaul Cheernam via lists.openembedded.org wrote: > rstrip() is called before checking for backslash, so trailing spaces or > tabs after "\" go unnoticed. For example: > > file://foo.patch \<TAB> > file://bar.patch \ > > The first line has a trailing tab after the backslash that is invisible > but ends up in patch context, causing patches to fail to apply on trees > where it was cleaned up. > > Emit a warning when this is detected so developers can fix it early. > > Tested with bitbake-selftest: > $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_trailing_whitespace_continuation -v > $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_clean_continuation_no_warning -v > > 2 tests OK > > Signed-off-by: Jaipaul Cheernam <jaipaul.cheernam@est.tech> > --- > Problem: > Ran into this while submitting libssh2 CVE patches to both master and > wrynose. The libssh2_1.11.1.bb on master has a stray tab after the > backslash on the CVE-2025-15661-3.patch line. Patches adding new file:// > lines below it applied fine on master but git-am refused them on wrynose > because that branch doesn't have the trailing tab — context mismatch. This seems like a lot of code and complexity for what is in reality quite a minor issue which whilst annoying, doesn't actually break anything? Cheers, Richard
On 2026-08-04 12:27, Richard Purdie wrote: > On Tue, 2026-08-04 at 11:08 +0200, Jaipaul Cheernam via lists.openembedded.org wrote: >> rstrip() is called before checking for backslash, so trailing spaces or >> tabs after "\" go unnoticed. For example: >> >> file://foo.patch \<TAB> >> file://bar.patch \ >> >> The first line has a trailing tab after the backslash that is invisible >> but ends up in patch context, causing patches to fail to apply on trees >> where it was cleaned up. >> >> Emit a warning when this is detected so developers can fix it early. >> >> Tested with bitbake-selftest: >> $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_trailing_whitespace_continuation -v >> $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_clean_continuation_no_warning -v >> >> 2 tests OK >> >> Signed-off-by: Jaipaul Cheernam <jaipaul.cheernam@est.tech> >> --- >> Problem: >> Ran into this while submitting libssh2 CVE patches to both master and >> wrynose. The libssh2_1.11.1.bb on master has a stray tab after the >> backslash on the CVE-2025-15661-3.patch line. Patches adding new file:// >> lines below it applied fine on master but git-am refused them on wrynose >> because that branch doesn't have the trailing tab — context mismatch. > This seems like a lot of code and complexity for what is in reality quite a minor issue which > whilst annoying, doesn't actually break anything? > > Cheers, > > Richard Hi Richard, Fair point — it doesn't break parsing itself and the code churn is larger than ideal for a warning. Would you prefer if I just submitted the oe-core fixes to clean up the affected recipes instead and dropped this bitbake change? That solves the immediate problem without adding complexity to the parser. Alternatively, I could reduce this to a bb.note() (debug-level) so it's only visible with -v, keeping it minimal. Happy to go either way. Thanks
On Tue, 2026-08-04 at 12:37 +0200, Jaipaul Cheernam wrote: > Hi Richard, > > Fair point — it doesn't break parsing itself and the code churn is > larger than ideal for a warning. > > Would you prefer if I just submitted the oe-core fixes to clean up > the affected recipes instead and dropped this bitbake change? That > solves the immediate problem without adding complexity to the parser. You should definitely send that clean up for oe-core. I was toying with the idea we just make whitespace after a line break character a fatal parsing error. Most of the fatal errors are inside feeder() rather than where you changed the code. I then looked at the code further and it isn't just trailing whitespace after "\" that is an issue. For example, after "}" closing a function could also be an issue in the same way. I didn't look in detail beyond that, there will be further similar issues though and that does mean your patch is incomplete :/. > Alternatively, I could reduce this to a bb.note() (debug-level) so > it's only visible with -v, keeping it minimal. I think this needs further thought and some wider testing of how widespread trailing whitespace is. We might just want to make any trailing spaces a warning since we currently just swallow and hide it unconditionally? Cheers, Richard
On 2026-08-04 13:37, Richard Purdie wrote: > On Tue, 2026-08-04 at 12:37 +0200, Jaipaul Cheernam wrote: >> Hi Richard, >> >> Fair point — it doesn't break parsing itself and the code churn is >> larger than ideal for a warning. >> >> Would you prefer if I just submitted the oe-core fixes to clean up >> the affected recipes instead and dropped this bitbake change? That >> solves the immediate problem without adding complexity to the parser. > You should definitely send that clean up for oe-core. Hi Richard, Thanks for the detailed feedback. I will send a clean-up patch. > I was toying with the idea we just make whitespace after a line break > character a fatal parsing error. Most of the fatal errors are inside > feeder() rather than where you changed the code. That would be the cleanest long-term. A fatal error for whitespace after \ makes sense since the backslash must be the last character — anything after it is always wrong. > I then looked at the code further and it isn't just trailing whitespace > after "\" that is an issue. For example, after "}" closing a function > could also be an issue in the same way. I didn't look in detail beyond > that, there will be further similar issues though and that does mean > your patch is incomplete :/. Good point, I hadn't considered that. >> Alternatively, I could reduce this to a bb.note() (debug-level) so >> it's only visible with -v, keeping it minimal. > I think this needs further thought and some wider testing of how > widespread trailing whitespace is. > > We might just want to make any trailing spaces a warning since we > currently just swallow and hide it unconditionally? I tried this — warned on any trailing whitespace in all parsed lines (comparing raw line minus newline against rstrip'd version). Running core-image-minimal: - 1665 total warnings (due to repeated re-parse of same files) - 59 unique (distinct file:line) - 31 files affected (.bbclass, .bb, .inc, .conf) Most are in bbclass files like sanity.bbclass, buildstats.bbclass, useradd.bbclass, kernel-yocto.bbclass, libc-package.bbclass etc. The duplication from re-parsing makes it noisy without some form of dedup. A few options for v2: 1. Keep it narrow — only warn after \ and } (the cases that actually cause patch conflicts). Around 10 unique warnings. 2. Warn on everything but with dedup (a module-level set). Cleaner output but adds state to the parser. 3. Warn on everything and send a cleanup series fixing all 59 instances in oe-core. Then the warning stays as a guard for future submissions. What would you prefer? Thanks, Jaipaul > > Cheers, > > Richard >
diff --git a/lib/bb/parse/parse_py/BBHandler.py b/lib/bb/parse/parse_py/BBHandler.py index 008fec230..710ea04d4 100644 --- a/lib/bb/parse/parse_py/BBHandler.py +++ b/lib/bb/parse/parse_py/BBHandler.py @@ -104,7 +104,11 @@ def get_statements(filename, absolute_filename, base_name): lineno = lineno + 1 s = f.readline() if not s: break - s = s.rstrip() + # Warn if trailing whitespace exists after a continuation backslash + raw = s.rstrip('\n').rstrip('\r') + s = raw.rstrip() + if s and s[-1] == '\\' and raw != s: + bb.warn("Trailing whitespace after line continuation backslash in %s, line %s" % (filename, lineno)) feeder(lineno, s, filename, base_name, statements) if __inpython__: diff --git a/lib/bb/parse/parse_py/ConfHandler.py b/lib/bb/parse/parse_py/ConfHandler.py index 9ddbae123..07b0c371f 100644 --- a/lib/bb/parse/parse_py/ConfHandler.py +++ b/lib/bb/parse/parse_py/ConfHandler.py @@ -134,12 +134,18 @@ def handle(fn, data, include, baseconfig=False): # skip empty lines if not w: continue - s = s.rstrip() + raw = s.rstrip('\n').rstrip('\r') + s = raw.rstrip() + if s and s[-1] == '\\' and raw != s: + bb.warn("Trailing whitespace after line continuation backslash in %s, line %s" % (fn, lineno)) while s[-1] == '\\': line = f.readline() origline += line - s2 = line.rstrip() + raw2 = line.rstrip('\n').rstrip('\r') + s2 = raw2.rstrip() lineno = lineno + 1 + if s2 and s2[-1] == '\\' and raw2 != s2: + bb.warn("Trailing whitespace after line continuation backslash in %s, line %s" % (fn, lineno)) if (not s2 or s2 and s2[0] != "#") and s[0] == "#" : bb.fatal("There is a confusing multiline, partially commented expression starting on line %s of file %s:\n%s\nPlease clarify whether this is all a comment or should be parsed." % (origlineno, fn, origline)) diff --git a/lib/bb/tests/parse.py b/lib/bb/tests/parse.py index 6ac2137e0..b01932350 100644 --- a/lib/bb/tests/parse.py +++ b/lib/bb/tests/parse.py @@ -638,3 +638,22 @@ EXTRA_OECONF:append = " foobar" output = run_bitbake(["bitbake", "-e", "recipe-file1"], builddir, extraenv).splitlines() self.assertIn('BBCLASS_FILE="recipe-file.inc"', output) self.assertIn(f'BBCLASS_RECIPE_FILE="recipe-file1.bb"', output) + + trailing_whitespace_continuation = "A = \"1 \\\t \n2\"\n" + + def test_parse_trailing_whitespace_continuation(self): + """Test that trailing whitespace after backslash continuation emits a warning""" + with self.parsehelper(self.trailing_whitespace_continuation) as f: + with self.assertLogs('BitBake', level='WARNING') as cm: + d = bb.parse.handle(f.name, self.d)[''] + self.assertTrue(any("Trailing whitespace after line continuation backslash" in msg for msg in cm.output)) + # Verify it still parses correctly despite the warning + self.assertEqual(d.getVar("A"), "1 2") + + clean_continuation = "A = \"1 \\\n2\"\n" + + def test_parse_clean_continuation_no_warning(self): + """Test that clean backslash continuation does not warn""" + with self.parsehelper(self.clean_continuation) as f: + d = bb.parse.handle(f.name, self.d)[''] + self.assertEqual(d.getVar("A"), "1 2")
rstrip() is called before checking for backslash, so trailing spaces or tabs after "\" go unnoticed. For example: file://foo.patch \<TAB> file://bar.patch \ The first line has a trailing tab after the backslash that is invisible but ends up in patch context, causing patches to fail to apply on trees where it was cleaned up. Emit a warning when this is detected so developers can fix it early. Tested with bitbake-selftest: $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_trailing_whitespace_continuation -v $ python3 -m unittest lib.bb.tests.parse.ParseTest.test_parse_clean_continuation_no_warning -v 2 tests OK Signed-off-by: Jaipaul Cheernam <jaipaul.cheernam@est.tech> --- Problem: Ran into this while submitting libssh2 CVE patches to both master and wrynose. The libssh2_1.11.1.bb on master has a stray tab after the backslash on the CVE-2025-15661-3.patch line. Patches adding new file:// lines below it applied fine on master but git-am refused them on wrynose because that branch doesn't have the trailing tab — context mismatch. Other recipes in oe-core with the same issue: autoconf_2.73.bb:2 perl_5.44.0.bb:313,314 mc_4.8.33.bb:50,51 libxml-sax-perl_1.02.bb:4 A separate series will follow to fix the trailing whitespace in the affected recipes. lib/bb/parse/parse_py/BBHandler.py | 6 +++++- lib/bb/parse/parse_py/ConfHandler.py | 10 ++++++++-- lib/bb/tests/parse.py | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-)