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

hroncok · 2022-10-26T10:38:31Z

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 is not used.

Fixes #98707

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

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

…t use the vendored headers 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 is **not** used. Fixes python#98707

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

merwok · 2022-10-27T14:46:01Z

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 🤷🏽)

hroncok · 2022-10-27T14:51:15Z

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.

erlend-aasland · 2022-10-27T15:20:09Z

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.

bedevere-bot · 2022-11-09T13:49:53Z

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

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

vstinner · 2022-11-09T14:42:34Z

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)' Makefile
MODULE_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)' Makefile
MODULE_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 libmpdec
	libmpdec.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)' Makefile
MODULE_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 without Modules/_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)' Makefile
MODULE_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 libmpdec
	libmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007f3aa03ff000)

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

I also tested: system libmpdec without Modules/_decimal/libmpdec/ (removed) and system expat without Modules/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)' Makefile
MODULE_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 libmpdec
	libmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007fe29b855000)

$ ldd $(./python -c 'import pyexpat; print(pyexpat.__file__)')|grep expat
	libexpat.so.1 => /lib64/libexpat.so.1 (0x00007f763abea000)

Good, as expected, MODULE_PYEXPAT_DEPS is empty.

vstinner

LGTM.

vstinner · 2022-11-09T14:49:28Z

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 libmpdec
	libmpdec.so.3 => /lib64/libmpdec.so.3 (0x00007f1edc402000)
$ ldd $(./python -c 'import pyexpat; print(pyexpat.__file__)')|grep expat
	libexpat.so.1 => /lib64/libexpat.so.1 (0x00007f7a66b87000)

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

vstinner · 2022-11-09T14:56:22Z

@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 prefer git commit --amend to fix the commit message. Otherwise, there is a risk that the wrong commit message is picked :-(

erlend-aasland

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

miss-islington · 2022-11-11T18:59:33Z

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!

miss-islington · 2022-11-11T18:59:53Z

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

bedevere-bot · 2022-11-11T19:05:59Z

GH-99391 is a backport of this pull request to the 3.11 branch.

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

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

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

bedevere-bot added the awaiting review label Oct 26, 2022

erlend-aasland requested review from erlend-aasland and tiran 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 Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

hroncok and others added 2 commits October 27, 2022 15:28

Better changelog entry

cb7d840

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

hroncok force-pushed the with-system branch from ab487a2 to cb7d840 Compare October 27, 2022 13:28

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2022

encukou added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section and removed 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 9, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2022

vstinner approved these changes Nov 9, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 9, 2022

vstinner added the needs backport to 3.11 only security fixes label Nov 9, 2022

erlend-aasland approved these changes Nov 11, 2022

View reviewed changes

erlend-aasland merged commit 6abec1c into python:main Nov 11, 2022

bedevere-bot removed the awaiting merge label Nov 11, 2022

miss-islington assigned erlend-aasland Nov 11, 2022

bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 11, 2022

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)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

hroncok commented Oct 26, 2022 •

edited

Loading

This comment was marked as resolved.

merwok commented Oct 27, 2022

hroncok commented Oct 27, 2022 •

edited

Loading

erlend-aasland commented Oct 27, 2022

bedevere-bot commented Nov 9, 2022

vstinner commented Nov 9, 2022

vstinner left a comment

vstinner commented Nov 9, 2022

vstinner commented Nov 9, 2022

erlend-aasland left a comment

miss-islington commented Nov 11, 2022

miss-islington commented Nov 11, 2022

bedevere-bot commented Nov 11, 2022

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

Conversation

hroncok commented Oct 26, 2022 • edited Loading

This comment was marked as resolved.

merwok commented Oct 27, 2022

hroncok commented Oct 27, 2022 • edited Loading

erlend-aasland commented Oct 27, 2022

bedevere-bot commented Nov 9, 2022

vstinner commented Nov 9, 2022

main branch

This PR

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Nov 9, 2022

vstinner commented Nov 9, 2022

erlend-aasland left a comment

Choose a reason for hiding this comment

miss-islington commented Nov 11, 2022

miss-islington commented Nov 11, 2022

bedevere-bot commented Nov 11, 2022

hroncok commented Oct 26, 2022 •

edited

Loading

hroncok commented Oct 27, 2022 •

edited

Loading