GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-98707: Don't let --with-system-libmpdec / --with-system-expat use the vendored headers by hroncok · Pull Request #98711 · python/cpython

Conversation

@hroncok

@hroncok hroncok commented Oct 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Oct 26, 2022
@erlend-aasland
erlend-aasland requested review from erlend-aasland and tiran October 26, 2022 13:21
erlend-aasland
erlend-aasland reviewed Oct 26, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Build/2022-10-26-12-37-52.gh-issue-98707.eVXGEx.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@hroncok

This comment was marked as resolved.

Sign in to view
hroncok and others added 2 commits October 27, 2022 15:28
@hroncok
pythongh-98707: Don't let --with-system-libmpdec / --with-system-expa…
a1e927c 
…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
@hroncok @erlend-aasland
Better changelog entry
cb7d840 
Co-authored-by: Erlend E. Aasland <[email protected]>
@hroncok
hroncok force-pushed the with-system branch from ab487a2 to cb7d840 Compare October 27, 2022 13:28
@merwok

merwok commented Oct 27, 2022

Copy link
Copy Markdown
Member

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 🤷🏽)
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hroncok

hroncok commented Oct 27, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Oct 27, 2022

Copy link
Copy Markdown
Contributor

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.
hroncok and AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2022
@encukou 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

bedevere-bot commented Nov 9, 2022

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2022
@vstinner

vstinner commented Nov 9, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Nov 9, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Nov 9, 2022
@vstinner vstinner added the needs backport to 3.11 only security fixes label Nov 9, 2022
@vstinner

vstinner commented Nov 9, 2022

Copy link
Copy Markdown
Member

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Nov 9, 2022

Copy link
Copy Markdown
Member

@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 :-(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland approved these changes Nov 11, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland
erlend-aasland merged commit 6abec1c into python:main Nov 11, 2022
@miss-islington

miss-islington commented Nov 11, 2022

Copy link
Copy Markdown
Contributor

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 11, 2022
@miss-islington

miss-islington commented Nov 11, 2022

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned erlend-aasland Nov 11, 2022
@bedevere-bot

bedevere-bot commented Nov 11, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 11, 2022
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Nov 11, 2022
@hroncok @erlend-aasland
[3.11] pythongh-98707: configure --with-system-libmpdec and --with-sy…
d97137c 
…stem-expat no longer include vendored headers (pythonGH-98711).

(cherry picked from commit 6abec1c)

Co-authored-by: Miro Hrončok <[email protected]>
@erlend-aasland 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
@hroncok @ethanfurman
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
@erlend-aasland @hroncok
[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 commit 6abec1c)

Co-authored-by: Miro Hrončok <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@erlend-aasland erlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@hroncok @merwok @erlend-aasland @bedevere-bot @vstinner @miss-islington @encukou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.