-
-
Notifications
You must be signed in to change notification settings - Fork 31k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-115399: Upgrade bundled libexpat to 2.6.0 #115431
Conversation
configure.ac
Outdated
@@ -3720,7 +3720,7 @@ AS_VAR_IF([with_system_expat], [yes], [ | |||
LIBEXPAT_LDFLAGS=${LIBEXPAT_LDFLAGS-"-lexpat"} | |||
LIBEXPAT_INTERNAL= | |||
], [ | |||
LIBEXPAT_CFLAGS="-I\$(srcdir)/Modules/expat" | |||
LIBEXPAT_CFLAGS="-I\$(srcdir)/Modules/expat -DXML_GE=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to make a choice, error message from the Windows builds that are now failing cuz I need to update them as well:
XML_GE (for general entities) must be defined, non-empty, either 1 or 0 (0 to disable, 1 to enable; 1 is a common default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably update the default value in line 3719 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's that?
@ambv "GE" is short for general entities. The flag is documented in detail at https://libexpat.github.io/doc/api/latest/#XML_GE .
@sethmlarson I would have expected (meaning considered likely) a new line #define XML_GE 1
to go into CPython's own expat_config.h
. Did you avoid that file on purpose? Its sibling XML_DTD
seems to live there.
You should probably update the default value in line 3719 as well.
For anyone else wondering where (or what the line content is), that seems to be:
Line 3719 in 514b1c9
LIBEXPAT_CFLAGS=${LIBEXPAT_CFLAGS-""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hartwork Thank you for this, I don't know how but my grep XML_DTD
turned up no results. That's definitely where it should go.
@sethmlarson good call, yes. On a sidenote |
4c20480
to
f41fbfc
Compare
Thanks for the guidance @hartwork! I rearranged the configuration to do as you said. I also split the update itself into a separate commit from the SBOM update (which removes |
Modules/expat/expat_external.h
Outdated
|
||
/* Namespace external symbols to allow multiple libexpat version to | ||
co-exist. */ | ||
#include "pyexpatns.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sethmlarson could it be that this removal is accidental?
PS: Here's the Dockerfile that made me spot this, based on earlier version #98742 (review):
# Copyright (c) 2022-2024 Sebastian Pipping <[email protected]>
# Licensed under the Apache License version 2.0
FROM alpine
RUN apk add --update \
diffutils \
git \
sed \
&& \
git clone --depth 1 https://github.com/python/cpython cpython-main \
&& \
( cd cpython-main && git rev-parse HEAD ) \
&& \
git clone --depth 1 --branch libexpat-2.6.0 https://github.com/sethmlarson/cpython cpython-pr \
&& \
( cd cpython-pr && git rev-parse HEAD ) \
&& \
git config --global advice.detachedHead false \
&& \
git clone --depth 1 --branch R_2_5_0 https://github.com/libexpat/libexpat libexpat_2_5_0 \
&& \
git clone --depth 1 --branch R_2_6_0 https://github.com/libexpat/libexpat libexpat_2_6_0 \
&& \
diff -r -u libexpat_2_5_0/expat/lib/ cpython-main/Modules/expat/ | tee 2-5-0.diff \
&& \
diff -r -u libexpat_2_6_0/expat/lib/ cpython-pr/Modules/expat/ | tee 2-6-0.diff \
&& \
sed -e '/^Only in /d' -e '/^\(+++\|---\) /d' -e '/^diff /d' -i 2-5-0.diff 2-6-0.diff \
&& \
diff -u 2-5-0.diff 2-6-0.diff \
&& \
echo 'Diff is good.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugovk It feels like now we have to automate this diff comparison. Do you have any good ideas on when to trigger the workflow smartly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corona10 did you mean me or @hugovk?
To automate this, you'd need to extract an old and a new version from the diff between the push HEAD and the base commit and so on. I'm not sure if Expat releases are frequent enough to build automation like that and keep it working.
Pull request #100539 is about something related but different, but maybe still of interest to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corona10 PS: maybe a cheap version to cover the core need (unless misunderstood) would be to make CI assert that that file still includes file pyexpatns.h
using grep? That would be orders of magnitude simpler 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, I've updated my commit to not remove these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b25685e
to
0d3fb31
Compare
Backport to 3.11 and prior of this PR: #115468 3.12 will be handled automatically once this PR merges. |
Thanks @sethmlarson for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
(cherry picked from commit 4b2d178) Co-authored-by: Seth Michael Larson <[email protected]>
GH-115469 is a backport of this pull request to the 3.12 branch. |
Closes #115399
XML_GE
definition toLIBEXPAT_CFLAGS
since 2.6.0 requires us to choose one, I set it to1
to keep general entities support. Our test suite didn't fail either way with this setting but I'm assuming we want to keep support?cc @hartwork