Repository navigation
Both test_flush_reparse_deferral_disabled tests fail with older libexpat (2.4.4) #117187
Description
Activity
I am rather confused by the coding style of #115623 … isn’t there a preference for
@unittest.skipIfinstead of randomly includedself.SkipTestcommands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests includingSetReparseDeferralEnabledandGetReparseDeferralEnabledbe just covered by@unittest.skipIf(pyexpat.version_info < (2, 6, 0))and be done with it, if those methods (as mentioned in the documentation) were introduced just inlibexpat2.6.0?Wouldn’t something like this be more appropriate?
--- Lib/test/test_sax.py | 10 +++++----- Lib/test/test_xml_etree.py | 17 ++++++++--------- 2 files changed, 13 insertions(+), 14 deletions(-) --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase): self.assertEqual(result.getvalue(), start + b"<doc>text</doc>") + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_enabled(self): - if pyexpat.version_info < (2, 6, 0): - self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral') - result = BytesIO() xmlgen = XMLGenerator(result) parser = create_parser() @@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase): self.assertEqual(result.getvalue(), start + b"<doc></doc>") + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_disabled(self): result = BytesIO() xmlgen = XMLGenerator(result) @@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase): for chunk in ("<doc", ">"): parser.feed(chunk) - if pyexpat.version_info >= (2, 6, 0): - parser._parser.SetReparseDeferralEnabled(False) + parser._parser.SetReparseDeferralEnabled(False) self.assertEqual(result.getvalue(), start) # i.e. no elements started self.assertFalse(parser._parser.GetReparseDeferralEnabled()) --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas with self.assertRaises(ValueError): ET.XMLPullParser(events=('start', 'end', 'bogus')) + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_enabled(self): - if pyexpat.version_info < (2, 6, 0): - self.skipTest(f'Expat {pyexpat.version_info} does not ' - 'support reparse deferral') - parser = ET.XMLPullParser(events=('start', 'end')) for chunk in ("<doc", ">"): @@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas self.assert_event_tags(parser, [('end', 'doc')]) + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_disabled(self): parser = ET.XMLPullParser(events=('start', 'end')) for chunk in ("<doc", ">"): parser.feed(chunk) - if pyexpat.version_info >= (2, 6, 0): - if not ET is pyET: - self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled ' - 'methods not available in C') - parser._parser._parser.SetReparseDeferralEnabled(False) + if not ET is pyET: + self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled ' + 'methods not available in C') + parser._parser._parser.SetReparseDeferralEnabled(False) self.assert_event_tags(parser, []) # i.e. no elements started if ET is pyET:
breaks with the old
libexpat(of course, patched for the security issues):@mcepl that's expected and distros backporting libexpat security fixed need to adjust the tests (and C pre-processor checks!) of CPython at the same time if doing so. This is a downstream issue, not an upstream one. I'm not alone in this view, e.g. see #115623 (review) .
I am rather confused by the coding style of #115623 … isn’t there a preference for
@unittest.skipIfinstead of randomly includedself.SkipTestcommands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests includingSetReparseDeferralEnabledandGetReparseDeferralEnabledbe just covered by@unittest.skipIf(pyexpat.version_info < (2, 6, 0))and be done with it, if those methods (as mentioned in the documentation) were introduced just inlibexpat2.6.0?Wouldn’t something like this be more appropriate?
@mcepl hunk 1 and 3 are purely cosmetic, and hence of limited interest to me:
Hunk 1:
--- Lib/test/test_sax.py | 10 +++++----- Lib/test/test_xml_etree.py | 17 ++++++++--------- 2 files changed, 13 insertions(+), 14 deletions(-) --- a/Lib/test/test_sax.py +++ b/Lib/test/test_sax.py @@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase): self.assertEqual(result.getvalue(), start + b"<doc>text</doc>") + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_enabled(self): - if pyexpat.version_info < (2, 6, 0): - self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral') - result = BytesIO() xmlgen = XMLGenerator(result) parser = create_parser()
Hunk 3:
--- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas with self.assertRaises(ValueError): ET.XMLPullParser(events=('start', 'end', 'bogus')) + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_enabled(self): - if pyexpat.version_info < (2, 6, 0): - self.skipTest(f'Expat {pyexpat.version_info} does not ' - 'support reparse deferral') - parser = ET.XMLPullParser(events=('start', 'end')) for chunk in ("<doc", ">"):
Hunk 2 and 4 are a very different story:
Hunk 2:
@@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase): self.assertEqual(result.getvalue(), start + b"<doc></doc>") + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_disabled(self): result = BytesIO() xmlgen = XMLGenerator(result) @@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase): for chunk in ("<doc", ">"): parser.feed(chunk) - if pyexpat.version_info >= (2, 6, 0): - parser._parser.SetReparseDeferralEnabled(False) + parser._parser.SetReparseDeferralEnabled(False) self.assertEqual(result.getvalue(), start) # i.e. no elements started self.assertFalse(parser._parser.GetReparseDeferralEnabled())
Hunk 4:
@@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas self.assert_event_tags(parser, [('end', 'doc')]) + @unittest.skipIf(pyexpat.version_info < (2, 6, 0), + "Reparse deferral not defined for libexpat < 2.6.0") def test_flush_reparse_deferral_disabled(self): parser = ET.XMLPullParser(events=('start', 'end')) for chunk in ("<doc", ">"): parser.feed(chunk) - if pyexpat.version_info >= (2, 6, 0): - if not ET is pyET: - self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled ' - 'methods not available in C') - parser._parser._parser.SetReparseDeferralEnabled(False) + if not ET is pyET: + self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled ' + 'methods not available in C') + parser._parser._parser.SetReparseDeferralEnabled(False) self.assert_event_tags(parser, []) # i.e. no elements started if ET is pyET:
@mcepl both hunk 2 and 4 seem to change semantics for the worse, as tests that were previously running with <2.6.0 fine are now skipped without need to. This inconsistency was done on full purpose.
I'm not against simplifying the two of the four skip markers that keep semantics intact, but I don't consider it of much value, personally.
My vote for closing this issue as "works as expected, no upstream issue to fix".
Reacted by Serhiy StorchakaShouldn’t all tests including
SetReparseDeferralEnabledandGetReparseDeferralEnabledbe just covered by@unittest.skipIf(pyexpat.version_info < (2, 6, 0))and be done with it, if those methods (as mentioned in the documentation) were introduced just inlibexpat2.6.0?@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.
Hi @mcepl,
@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.
So, you admit that the documentation is wrong.I don't, and I don't see how it would be wrong. What does the documentation state that is incorrect and which documentation exactly?
Also, why then these tests fail with older libexpat?
Because of the security patches that distros backported into older releases of libexpat that got you into unsupported territory between old and new releases. The failing version is not vanilla 2.4.4 or it would not fail. It's 2.4.4 with backported patches.
Reacted by Serhiy Storchaka- @mcepl that's expected and distros backporting libexpat security fixed need to adjust the tests (and C pre-processor checks!) of CPython at the same time if doing so. This is a downstream issue, not an upstream one. I'm not alone in this view, e.g. see #115623 (review) .I am still confused, but it probably doesn’t matter. Either you expect everybody to use the latest and greatest libraries (which is kind of strange for the universal interpreter), or you would like to support even older libraries. For the first I don’t understand why you have those `if pyexpat.version_info` spread over the code, for the latter I don’t understand why you are not interested when I report you that the support for the older libraries is broken. Anyway, I will just use my patch in *SUSE and let it be.@mcepl this seems to change semantics for the worse, as a test that was previously running with <2.6.0 fine is now skipped without need to.No it wasn’t running, as this ticket is all about tests not running with older libexpat.My vote for closing this issue as "works as expected, no upstream issue to fix".Sure, I will just patch the Python for ourselves as long as you don’t care.
@mcepl I have sent you an e-mail offering a voice call in the mean time, about 30 minutes ago. I will refrain from replying here more for now, before we had a real chance to talk. Thanks for your consideration.
I am in the Central European Time zone, and it is Sunday night here. As I see you are in Berlin, we can talk tomorrow (
mceplon Libera.chat (preferable), or@mcepl:one.ems.hoston Matrix).I completely agree with @hartwork. Python tests work pretty well with both new and old versions of libexpat. They do not work with a distro-specific build of libexpat with distro-specific patches applied. This is not supported case. You perhaps need to apply a distro-specific patch for Python tests to make them consistent with the rest of of the distro. Or what is the standard way to solve such issues in your distro.
Reacted by Sebastian PippingHi, glad you two are chatting.
Looking at the build log you link to from the opening message, the important bit is:
- This was a
./configure --with-system-expatbuild. (not our default build mode) - The system expat is version 2.4.4.
There were a pile of distro patches on top of that expat and Python applied, but none of those matter. (No backport of the related expat 2.6.0 security improvement feature was involved).
This is easy to reproduce on current non-bleeding-edge Debian and Ubuntu today (sporting expat 2.5.0 or earlier), make sure
libexpat1-devis installed, and presumbly a bunch of other distros (like opensuse or SuSE as was being used):./configure --with-system-expat && ./python -m test test_sax test_xml_etreeUnfortunately, we do not test
--with-system-expatin any way as part of the CPython project. So this failing right now isn't too surprising. That flag exists primarily for use by external distributors who unvendor libraries.A good way to help prevent this kind of building against legacy libraries failure from happening in the future is to contribute & maintain a relevant builtbot. https://devguide.python.org/testing/new-buildbot-worker/
Reacted by Sebastian Pipping- This was a
Python tests work pretty well with both new and old versions of libexpat.
This currently appears to unfortunately be false because we don't test this anywhere.
Reacted by Sebastian Pipping10 remaining items
Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.
No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's
expat_2.5.0-2ubuntu0.1package. Ideally a distro needing that would contribute that?- added 2 commits that reference this issue
on Mar 26, 2024 Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.
@gpshead thank you!
No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's
expat_2.5.0-2ubuntu0.1package. Ideally a distro needing that would contribute that?I would argue that support for downstream backports would be out of place upstream. Supporting multiple versions of vanilla Expat is complex enough. So instead of asking for these patches for upstream, let's not patch upstream and keep the whole backporting cake downstream.
- added 2 commits that reference this issue
on Apr 2, 2024
Bug report
Bug description:
Running the test suite for 3.9.19 (and other currently released versions) breaks with the old
libexpat(of course, patched for the security issues):Complete build log with all package versions and steps taken recorded.
CPython versions tested on:
3.8, 3.9, 3.10
Operating systems tested on:
Linux
Linked PRs