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-115119: Default to --with-system-libmpdec=yes by erlend-aasland · Pull Request #118539 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 3, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Mark _decimal as missing if libmpdecimal is not found.

  • Issue: Unvendor libmpdec sources #115119

📚 Documentation preview 📚: https://cpython-previews--118539.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-115119: Default to --with-system-libmpdec=yes
268d270
Mark _decimal as missing if libmpdecimal is not found.
@bedevere-app bedevere-app Bot mentioned this pull request May 3, 2024
Unvendor libmpdec sources #115119
Closed
15 tasks done
@erlend-aasland

erlend-aasland commented May 3, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author
$ ./configure --with-system-libmpdec=no | grep -E "(decimal|mpdec)"
checking for --with-system-libmpdec... no
checking for --with-decimal-contextvar... yes
checking for decimal libmpdec machine... universal
checking for stdlib extension module _decimal... yes
$ ./configure | grep -E "(decimal|mpdec)"
checking for --with-system-libmpdec... yes
checking for libmpdec... yes
checking for --with-decimal-contextvar... yes
checking for stdlib extension module _decimal... yes
$ grep -E "_DECIMAL_(CFLAGS|LDFLAGS)=" Makefile
MODULE__DECIMAL_CFLAGS=-I/opt/homebrew/Cellar/mpdecimal/4.0.0/include
MODULE__DECIMAL_LDFLAGS=-L/opt/homebrew/Cellar/mpdecimal/4.0.0/lib -lmpdec -lm
All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 3, 2024

Copy link
Copy Markdown
Contributor Author

@zware: I'm not sure if we should fall back to the internal copy if libmpdec is missing. For now, it is marked as missing if it is missing :) We could change it so configure fails if --with-system-libmpdec=yes and no mpdecimal library was found.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland commented May 3, 2024
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 3, 2024

Copy link
Copy Markdown
Contributor Author

Things to consider:

  • Ubuntu 20.04 provides libmpdecimal 2.4 (no pkgconfig support): apt install libmpdec2
  • Ubuntu 22.04 provides libmpdecimal 2.5 (no pkgconfig support): apt install libmpdec3 (!)
  • Ubuntu 24.04 does not appear to provide libmpdecimal at all

For now, we can consider to use the bundled version for the Ubuntu CI.

All reactions

Sorry, something went wrong.

Uh oh!

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

@zware

zware commented May 3, 2024

Copy link
Copy Markdown
Member

@zware: I'm not sure if we should fall back to the internal copy if libmpdec is missing. For now, it is marked as missing if it is missing :) We could change it so configure fails if --with-system-libmpdec=yes and no mpdecimal library was found.

Right, an explicit --with-system-libmpdec argument should be followed, failing if the system library is not there. --with-system-libmpdec=no/--without-system-libmpdec should warn, and no argument with no system library available should also warn. Whatever I said in the issue was really shorthand for "do whatever we did when we ripped out libffi because that seemed to work1" which I think is basically what I've now laid out here :)

Footnotes

  1. though that one was complicated by differing timelines on Linux and macOS which we don't need to deal with here ↩

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.

erlend-aasland added 3 commits May 5, 2024 22:38
@erlend-aasland
Pull in main
45173e3
@erlend-aasland
Use the same style as the surrounding code
6f020d0
@erlend-aasland
Fail if --with-system-libmpdec is given but no library was found
fe56023
@erlend-aasland

erlend-aasland commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

Right, an explicit --with-system-libmpdec argument should be followed, failing if the system library is not there.

Yep

[...] --with-system-libmpdec=no/--without-system-libmpdec should warn, [..]

Yes, we need to warn for that case.

[...] and no argument with no system library available should also warn.

Hm, I think this case should fail, similar to how we handle --with-system-libmpdec=yes and no library found.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Address review: warn if bundled version is used
0006897
@zware

zware commented May 5, 2024

Copy link
Copy Markdown
Member

[...] and no argument with no system library available should also warn.

Hm, I think this case should fail, similar to how we handle --with-system-libmpdec=yes and no library found.

I don't want a bare ./configure to start failing if mpdecimal isn't found, since that's not going to be the behavior even after the bundled library is removed. I could maybe get behind allowing the _decimal module build to fail rather than build the bundled library in that case, but even then we should also emit a warning from configure to make the issue more obvious.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

Ok, how about this:

  • bare (no --with-system-libmpdec): warn if system library is not found, mark _decimal as missing
  • --with-system-libmpdec=yes: mark _decimal as missing
  • --with-system-libmpdec=no: use bundled version, warn that it will be removed in Python 3.15
All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland added 5 commits May 6, 2024 00:21
@erlend-aasland
Address review: warn if system library was not found
4f87c91
@erlend-aasland
For now, build with bundled libmpdec in Ubuntu CIs
f8f4ea2
@erlend-aasland
Add NEWS
8e80866
@erlend-aasland
What's New
e51c1f7
@erlend-aasland
Actually use bundled version if told to do so
a971a26
@erlend-aasland
erlend-aasland marked this pull request as ready for review May 5, 2024 22:48
@erlend-aasland
erlend-aasland requested review from corona10, ezio-melotti and hugovk as code owners May 5, 2024 22:48
@bedevere-app bedevere-app Bot added the awaiting core review label May 5, 2024
@erlend-aasland
erlend-aasland requested a review from zware May 5, 2024 22:48
zware
zware approved these changes May 6, 2024
View reviewed changes

@zware zware 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.

I have a couple of questions and qualms, but overall I'm okay with this as is. We can tweak it through the beta phase if needed.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread .github/workflows/build.yml Resolved

Uh oh!

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

Comment thread Doc/using/configure.rst Resolved

Uh oh!

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

Comment thread Doc/using/configure.rst Resolved

Uh oh!

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

Comment thread configure.ac Resolved

Uh oh!

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

Comment thread configure.ac Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 6, 2024
erlend-aasland and others added 2 commits May 6, 2024 09:02
@erlend-aasland @zware
Address review: add deprecated-removed directive regarding the remova…
e3f3e17
…l of the mpdecimal sources

Co-authored-by: Zachary Ware <[email protected]>
@erlend-aasland
Add libmpdec-dev to CI deps, remove CI hack, and enforce mpdecimal ve…
c84f2b7
…rsion check in configure
@erlend-aasland
Pull in main
161d50b
@erlend-aasland
erlend-aasland marked this pull request as draft May 6, 2024 08:51
@bedevere-app bedevere-app Bot removed the awaiting merge label May 6, 2024
@erlend-aasland
Make the mpdec link check more accurate, and use LIBS iso. LDFLAGS
1016f0d
@erlend-aasland
erlend-aasland marked this pull request as ready for review May 6, 2024 09:12
@bedevere-app bedevere-app Bot added the awaiting core review label May 6, 2024
@erlend-aasland
erlend-aasland requested a review from zware May 6, 2024 09:14
erlend-aasland
erlend-aasland commented May 6, 2024
View reviewed changes
Comment thread .github/workflows/posix-deps-apt.sh
libgdbm-dev \
libgdbm-compat-dev \
liblzma-dev \
libmpdec-dev \

@erlend-aasland erlend-aasland May 6, 2024

Copy link
Copy Markdown
Contributor Author

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: FYI, this will fail for Ubuntu 24.04, as libmpdec-dev is unavailable there.

Sorry, something went wrong.

Uh oh!

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

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@zware zware May 6, 2024

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.

I've been trying to figure out why that is, and have come up short (I get lost quickly in the Debian/Ubuntu development process). All I've found is a note from @doko42 in the Ubuntu python3.11 3.11.2-4 changelog, saying "Build with internal mpdecimal library, so that mpdecimal can be removed for bookworm." (and indeed, Debian bookworm does not have a libmpdec package either).

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction

@erlend-aasland erlend-aasland May 6, 2024

Copy link
Copy Markdown
Contributor Author

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.

Found it:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1013395

Sorry, something went wrong.

Uh oh!

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

All reactions

@erlend-aasland erlend-aasland May 6, 2024

Copy link
Copy Markdown
Contributor Author

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.

... and:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1056785

Sorry, something went wrong.

Uh oh!

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

MtkN1 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
zware
zware approved these changes May 6, 2024
View reviewed changes
Comment thread .github/workflows/posix-deps-apt.sh
libgdbm-dev \
libgdbm-compat-dev \
liblzma-dev \
libmpdec-dev \

@zware zware May 6, 2024

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.

I've been trying to figure out why that is, and have come up short (I get lost quickly in the Debian/Ubuntu development process). All I've found is a note from @doko42 in the Ubuntu python3.11 3.11.2-4 changelog, saying "Build with internal mpdecimal library, so that mpdecimal can be removed for bookworm." (and indeed, Debian bookworm does not have a libmpdec package either).

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 6, 2024
@erlend-aasland
erlend-aasland merged commit 325a1da into python:main May 6, 2024
@erlend-aasland
erlend-aasland deleted the libmpdec-default-to-system-lib branch May 6, 2024 19:16
@bedevere-app bedevere-app Bot removed the awaiting merge label May 6, 2024
@erlend-aasland

erlend-aasland commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for helping out, Zach!

zware 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.

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@erlend-aasland @zware @SonicField
pythongh-115119: Default to --with-system-libmpdec=yes (python#118539)
b62d61f
Co-authored-by: Zachary Ware <[email protected]>
@asottile

asottile commented May 11, 2024

Copy link
Copy Markdown
Contributor

fwiw this is going to be painful for ubuntu and debian which has removed the libmpdec system library entirely (as it was only used for building python). also this was done pretty close to the beta freeze so it definitely caught me off guard (and broke the deadsnakes builds!)

All reactions

Sorry, something went wrong.

Uh oh!

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

@thesamesam

thesamesam commented May 11, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

That seems to have been mentioned at #118539 (comment) already, but I'm not sure what the problem is. We didn't have libmpdec packaged in Gentoo until now but we've done it and now all is fine...

Why can't Debian/Ubuntu just set --with-system-libmpdec=no or --without-system-libmpdec (whichever it is) if they can't package libmpdec quickly?

All reactions

Sorry, something went wrong.

Uh oh!

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

@ned-deily ned-deily mentioned this pull request May 19, 2024
Try to repair oddball test bots timing out in test_int #119166
Merged
@erlend-aasland

erlend-aasland commented May 20, 2024

Copy link
Copy Markdown
Contributor Author

@thesamesam, @asottile: we're adding a fallback to the bundled version if an external libmpdec cannot be found.

FTR, I find the recent removal of libmpdec from Ubuntu/Debian very strange.

thesamesam and asottile 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner
@hugovk hugovk Awaiting requested review from hugovk hugovk is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

4 participants

@erlend-aasland @zware @asottile @thesamesam
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.