NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

gh-98707: Don't let --with-system-libmpdec / --with-system-expat use the vendored headers#98711

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

erlend-aasland merged 2 commits intopython:mainfromhroncok:with-system

Nov 11, 2022

Merged

gh-98707: Don't let --with-system-libmpdec / --with-system-expat use the vendored headers#98711

erlend-aasland merged 2 commits intopython:mainfromhroncok:with-system

Nov 11, 2022

Conversation

Copy link

Contributor

hroncok commentedOct 26, 2022•
edited
Loading

This was a regression in Python 3.12.0a2 that prevented Fedora doing this:

$ rm -r Modules/_decimal/libmpdec$ rm -r Modules/expat

Before building Python with --with-system-libmpdec --with-system-expat.

The errors were:

make: *** No rule to make target 'Modules/_decimal/libmpdec/basearith.h', needed by 'Modules/_decimal/_decimal.o'.  Stop.make: *** No rule to make target 'Modules/expat/ascii.h', needed by 'Modules/pyexpat.o'.  Stop.

Now the make-dependency on the headers only exists when --with-system-libmpdec / --with-system-expat isnot used.

Fixes#98707

Issue:--with-system-libmpdec / --with-system-expat still uses the vendored headers #98707

bedevere-bot added the awaiting review label

Oct 26, 2022

erlend-aasland requested review fromerlend-aasland andtiran

October 26, 2022 13:21

erlend-aasland reviewed

Oct 26, 2022

View reviewed changes

Misc/NEWS.d/next/Build/2022-10-26-12-37-52.gh-issue-98707.eVXGEx.rst OutdatedShow resolvedHide resolved

This comment was marked as resolved.

hroncokand others added2 commits

October 27, 2022 15:28

pythongh-98707: Don't let --with-system-libmpdec / --with-system-expa…

a1e927c

…t use the vendored headersThis was a regression in Python 3.12.0a2 that prevented Fedora doing this:    $ rm -r Modules/_decimal/libmpdec    $ rm -r Modules/expatBefore building Python with --with-system-libmpdec --with-system-expat.The errors were:    make: *** No rule to make target 'Modules/_decimal/libmpdec/basearith.h', needed by 'Modules/_decimal/_decimal.o'.  Stop.    make: *** No rule to make target 'Modules/expat/ascii.h', needed by 'Modules/pyexpat.o'.  Stop.Now the make-dependency on the headers only existswhen --with-system-libmpdec / --with-system-expat is **not** used.Fixespython#98707

Better changelog entry

cb7d840

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

hroncok force-pushed thewith-system branch fromab487a2 tocb7d840Compare

October 27, 2022 13:28

Copy link

Member

merwok commentedOct 27, 2022

Python PRs avoid rebases and force pushes because it creates ghost notifications and generally makes it harder for reviewers to see what has changed. All PRs are squash merged so the number and contents of commits does not matter.

(Because of another github UI problem, sometimes the final commit message is a super-long, not useful concatenation of all commit messages, but 🤷🏽)

Copy link

ContributorAuthor

hroncok commentedOct 27, 2022•
edited
Loading

Since there is no way of fixing the commit message in a new commit and I do not have merge rights, I've decided to correct the typo instead of leaving it up to the merge-committer who might not notice it. If there is a better way of ensuring the typo won't make it to the final commit message, I will gladly do it that way the next time. Sorry for the trouble.

Copy link

Contributor

erlend-aasland commentedOct 27, 2022

Since there is no way of fixing the commit message in a new commit and I do not have merge rights, I've decided to correct the typo instead of leaving it up to the merge-committer who might not notice it. If there is a better way of ensuring the typo won't make it to the final commit message, I will gladly do it that way the next time. Sorry for the trouble.

Use the PR title. IIRC, we've updated the default commit message to be the PR title.

encukou added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Nov 2, 2022

encukou added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section and removed 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labels

Nov 9, 2022

Copy link

bedevere-bot commentedNov 9, 2022

🤖 New build scheduled with the buildbot fleet by@encukou for commitcb7d840 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Nov 9, 2022

Copy link

Member

vstinner commentedNov 9, 2022

I tested different configurations: Python build works as expected.

main branch

Vendored libmpdec:

$ make distclean$ ./configure CFLAGS="-O0" && make 2>&1|tee make.log$ grep -E '^(MODULE_PYEXPAT_DEPS|MODULE__DECIMAL_DEPS|MODULE__ELEMENTTREE_DEPS)' MakefileMODULE_PYEXPAT_DEPS=$(LIBEXPAT_HEADERS) $(LIBEXPAT_A)MODULE__DECIMAL_DEPS=$(srcdir)/Modules/_decimal/docstrings.h $(LIBMPDEC_HEADERS) $(LIBMPDEC_A)MODULE__ELEMENTTREE_DEPS=$(srcdir)/Modules/pyexpat.c $(LIBEXPAT_HEADERS) $(LIBEXPAT_A)# no result: _decimal is not linked to libmpdec$ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdec

System libmpdec:

$ make distclean$ ./configure CFLAGS="-O0" --with-system-libmpdec && make 2>&1|tee make.log$ grep -E '^(MODULE_PYEXPAT_DEPS|MODULE__DECIMAL_DEPS|MODULE__ELEMENTTREE_DEPS)' MakefileMODULE_PYEXPAT_DEPS=$(LIBEXPAT_HEADERS) $(LIBEXPAT_A)MODULE__DECIMAL_DEPS=$(srcdir)/Modules/_decimal/docstrings.h $(LIBMPDEC_HEADERS) MODULE__ELEMENTTREE_DEPS=$(srcdir)/Modules/pyexpat.c $(LIBEXPAT_HEADERS) $(LIBEXPAT_A)$ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdeclibmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007f79499af000)

And LIBMPDEC_HEADERS is defined in Makefile with:

LIBMPDEC_HEADERS= \$(srcdir)/Modules/_decimal/libmpdec/basearith.h \$(srcdir)/Modules/_decimal/libmpdec/bits.h \$(srcdir)/Modules/_decimal/libmpdec/constants.h \$(srcdir)/Modules/_decimal/libmpdec/convolute.h \$(srcdir)/Modules/_decimal/libmpdec/crt.h \$(srcdir)/Modules/_decimal/libmpdec/difradix2.h \$(srcdir)/Modules/_decimal/libmpdec/fnt.h \$(srcdir)/Modules/_decimal/libmpdec/fourstep.h \$(srcdir)/Modules/_decimal/libmpdec/io.h \$(srcdir)/Modules/_decimal/libmpdec/mpalloc.h \$(srcdir)/Modules/_decimal/libmpdec/mpdecimal.h \$(srcdir)/Modules/_decimal/libmpdec/numbertheory.h \$(srcdir)/Modules/_decimal/libmpdec/sixstep.h \$(srcdir)/Modules/_decimal/libmpdec/transpose.h \$(srcdir)/Modules/_decimal/libmpdec/typearith.h \$(srcdir)/Modules/_decimal/libmpdec/umodarith.h

This PR

Vendored libmpdec:

$ make distclean$ ./configure CFLAGS="-O0" && make 2>&1|tee make.log$ grep -E '^(MODULE_PYEXPAT_DEPS|MODULE__DECIMAL_DEPS|MODULE__ELEMENTTREE_DEPS)' MakefileMODULE_PYEXPAT_DEPS=$(LIBEXPAT_HEADERS) $(LIBEXPAT_A)MODULE__DECIMAL_DEPS=$(srcdir)/Modules/_decimal/docstrings.h $(LIBMPDEC_HEADERS) $(LIBMPDEC_A)MODULE__ELEMENTTREE_DEPS=$(srcdir)/Modules/pyexpat.c $(LIBEXPAT_HEADERS) $(LIBEXPAT_A)# no result: _decimal is not linked to libmpdec$ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdec

System libmpdec withoutModules/_decimal/libmpdec/ (removed):

$ rm -rf Modules/_decimal/libmpdec/$ make distclean$ ./configure CFLAGS="-O0" --with-system-libmpdec && make 2>&1|tee make.log$ grep -E '^(MODULE_PYEXPAT_DEPS|MODULE__DECIMAL_DEPS|MODULE__ELEMENTTREE_DEPS)' MakefileMODULE_PYEXPAT_DEPS=$(LIBEXPAT_HEADERS) $(LIBEXPAT_A)MODULE__DECIMAL_DEPS=$(srcdir)/Modules/_decimal/docstrings.h MODULE__ELEMENTTREE_DEPS=$(srcdir)/Modules/pyexpat.c $(LIBEXPAT_HEADERS) $(LIBEXPAT_A)$ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdeclibmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007f3aa03ff000)

=>Good, as expected, LIBMPDEC_HEADERS no longer in MODULE__DECIMAL_DEPS.

I also tested:system libmpdec withoutModules/_decimal/libmpdec/ (removed) andsystem expat withoutModules/expat/ (removed):

$ rm -rf Modules/_decimal/libmpdec/ Modules/expat/$ make distclean$ ./configure CFLAGS="-O0" --with-system-libmpdec --with-system-expat && make 2>&1|tee make.log$ grep -E '^(MODULE_PYEXPAT_DEPS|MODULE__DECIMAL_DEPS|MODULE__ELEMENTTREE_DEPS)' MakefileMODULE_PYEXPAT_DEPS=MODULE__DECIMAL_DEPS=$(srcdir)/Modules/_decimal/docstrings.h MODULE__ELEMENTTREE_DEPS=$(srcdir)/Modules/pyexpat.c $ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdeclibmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007fe29b855000)$ ldd $(./python -c 'import pyexpat; print(pyexpat.__file__)')|grep expatlibexpat.so.1 => /lib64/libexpat.so.1 (0x00007f763abea000)

Good, as expected, MODULE_PYEXPAT_DEPS is empty.

vstinner approved these changes

Nov 9, 2022

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM.

bedevere-bot added awaiting merge and removed awaiting review labels

Nov 9, 2022

vstinner added the needs backport to 3.11only security fixes label

Nov 9, 2022

Copy link

Member

vstinner commentedNov 9, 2022

This change can be backported to Python 3.11.

Python 3.10 doesn't seem to be affected:

$ git clean -fdx$ rm -rf Modules/_decimal/libmpdec/ Modules/expat/$ ./configure CFLAGS="-O0" --with-system-libmpdec --with-system-expat$ make 2>&1|tee make.log$ ldd $(./python -c 'import _decimal; print(_decimal.__file__)')|grep libmpdeclibmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007f1edc402000)$ ldd $(./python -c 'import pyexpat; print(pyexpat.__file__)')|grep expatlibexpat.so.1 => /lib64/libexpat.so.1 (0x00007f7a66b87000)

The build works without Modules/_decimal/libmpdec/ nor Modules/expat/ directories (removed).

Copy link

Member

vstinner commentedNov 9, 2022

@erlend-aasland: Do you want to review again the PR?

I plan to merge it soon. I'm now waiting for last buildbot jobs.

Since there is no way of fixing the commit message in a new commit and I do not have merge rights, I've decided to correct the typo instead of leaving it up to the merge-committer who might not notice it.

I always done that, so I'm fine with it :-) I prefergit commit --amend to fix the commit message. Otherwise, there is a risk that the wrong commit message is picked :-(

erlend-aasland approved these changes

Nov 11, 2022

View reviewed changes

Copy link

Contributor

erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. Thanks Miro for finding and fixing this, and thanks Victor for verifying the fix :)

erlend-aasland merged commit6abec1c intopython:main

Nov 11, 2022

Copy link

Contributor

miss-islington commentedNov 11, 2022

Thanks@hroncok for the PR, and@erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot removed the awaiting merge label

Nov 11, 2022

Copy link

Contributor

miss-islington commentedNov 11, 2022

Sorry,@hroncok and@erlend-aasland, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 6abec1caffdba2e282b14fe57c6ce61974de4bbe 3.11

miss-islington assignederlend-aasland

Nov 11, 2022

Copy link

bedevere-bot commentedNov 11, 2022

GH-99391 is a backport of this pull request to the3.11 branch.

bedevere-bot removed the needs backport to 3.11only security fixes label

Nov 11, 2022

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request

Nov 11, 2022

[3.11]pythongh-98707: configure --with-system-libmpdec and --with-sy…

d97137c

…stem-expat no longer include vendored headers (pythonGH-98711).(cherry picked from commit6abec1c)Co-authored-by: Miro Hrončok <miro@hroncok.cz>

erlend-aasland mentioned this pull request

Nov 11, 2022

--with-system-libmpdec / --with-system-expat still uses the vendored headers#98707

Closed

ethanfurman pushed a commit to ethanfurman/cpython that referenced this pull request

Nov 12, 2022

pythongh-98707: configure --with-system-libmpdec and --with-system-ex…

16bd877

…pat no longer include vendored headers (python#98711)

erlend-aasland added a commit that referenced this pull request

Nov 13, 2022

[3.11]gh-98707: configure --with-system-libmpdec and --with-system-e…

eac1a63

…xpat no longer include vendored headers (GH-98711) (#99391)(cherry picked from commit6abec1c)Co-authored-by: Miro Hrončok <miro@hroncok.cz>

Labels

None yet

Movatterモバイル変換

Uh oh!

gh-98707: Don't let --with-system-libmpdec / --with-system-expat use the vendored headers#98711

gh-98707: Don't let --with-system-libmpdec / --with-system-expat use the vendored headers#98711

Uh oh!

Conversation

hroncok commentedOct 26, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

merwok commentedOct 27, 2022

Uh oh!

hroncok commentedOct 27, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commentedOct 27, 2022

Uh oh!

bedevere-bot commentedNov 9, 2022

Uh oh!

vstinner commentedNov 9, 2022

main branch

This PR

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commentedNov 9, 2022

Uh oh!

vstinner commentedNov 9, 2022

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commentedNov 11, 2022

Uh oh!

miss-islington commentedNov 11, 2022

Uh oh!

bedevere-bot commentedNov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hroncok commentedOct 26, 2022•
edited
Loading

hroncok commentedOct 27, 2022•
edited
Loading