Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-115119: Default to --with-system-libmpdec=yes#118539

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

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedMay 3, 2024•
edited
Loading

Mark _decimal as missing if libmpdecimal is not found.

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

Mark _decimal as missing if libmpdecimal is not found.
@bedevere-appbedevere-appbot mentioned this pull requestMay 3, 2024
15 tasks
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedMay 3, 2024•
edited
Loading

$./configure --with-system-libmpdec=no| grep -E"(decimal|mpdec)"checking for --with-system-libmpdec... nochecking for --with-decimal-contextvar... yeschecking for decimal libmpdec machine... universalchecking for stdlib extension module _decimal... yes
$./configure| grep -E"(decimal|mpdec)"checking for --with-system-libmpdec... yeschecking for libmpdec... yeschecking for --with-decimal-contextvar... yeschecking for stdlib extension module _decimal... yes$grep -E"_DECIMAL_(CFLAGS|LDFLAGS)=" MakefileMODULE__DECIMAL_CFLAGS=-I/opt/homebrew/Cellar/mpdecimal/4.0.0/includeMODULE__DECIMAL_LDFLAGS=-L/opt/homebrew/Cellar/mpdecimal/4.0.0/lib -lmpdec -lm

@erlend-aasland
Copy link
ContributorAuthor

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

@erlend-aasland
Copy link
ContributorAuthor

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.

@zware
Copy link
Member

@zware: I'm not sure if we should fall back to the internal copy if libmpdec is missing. For now, it is marked asmissing if it is missing :) We could change it soconfigure 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 outlibffi 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

@erlend-aasland
Copy link
ContributorAuthor

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.

@zware
Copy link
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 ifmpdecimal 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 fromconfigure to make the issue more obvious.

@erlend-aasland
Copy link
ContributorAuthor

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

Copy link
Member

@zwarezware left a 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.

erlend-aaslandand others added2 commitsMay 6, 2024 09:02
…l of the mpdecimal sourcesCo-authored-by: Zachary Ware <zachary.ware@gmail.com>
@erlend-aaslanderlend-aasland marked this pull request as draftMay 6, 2024 08:51
@erlend-aaslanderlend-aasland marked this pull request as ready for reviewMay 6, 2024 09:12
@erlend-aaslanderlend-aasland requested a review fromzwareMay 6, 2024 09:14
@@ -15,6 +15,7 @@ apt-get -yq install \
libgdbm-dev \
libgdbm-compat-dev \
liblzma-dev \
libmpdec-dev \
Copy link
ContributorAuthor

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, aslibmpdec-dev is unavailable there.

hugovk reacted with thumbs up emoji
Copy link
Member

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 outwhy 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 Ubuntupython3.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).

erlend-aasland reacted with eyes emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

MtkN1 reacted with thumbs up emoji
@@ -15,6 +15,7 @@ apt-get -yq install \
libgdbm-dev \
libgdbm-compat-dev \
liblzma-dev \
libmpdec-dev \
Copy link
Member

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 outwhy 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 Ubuntupython3.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).

erlend-aasland reacted with eyes emoji
@erlend-aaslanderlend-aasland merged commit325a1da intopython:mainMay 6, 2024
39 checks passed
@erlend-aaslanderlend-aasland deleted the libmpdec-default-to-system-lib branchMay 6, 2024 19:16
@erlend-aasland
Copy link
ContributorAuthor

Thanks for helping out, Zach!

zware reacted with thumbs up emoji

SonicField pushed a commit to SonicField/cpython that referenced this pull requestMay 8, 2024
Co-authored-by: Zachary Ware <zachary.ware@gmail.com>
@asottile
Copy link
Contributor

fwiw this is going to be painful for ubuntu and debian which has removed thelibmpdec 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!)

@thesamesam
Copy link
Contributor

thesamesam commentedMay 11, 2024•
edited
Loading

That seems to have been mentioned at#118539 (comment) already, but I'm not sure what the problem is. We didn't havelibmpdec 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?

@erlend-aasland
Copy link
ContributorAuthor

@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

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

@zwarezwarezware approved these changes

@corona10corona10Awaiting requested review from corona10corona10 is a code owner

@ezio-melottiezio-melottiAwaiting requested review from ezio-melottiezio-melotti is a code owner

@hugovkhugovkAwaiting requested review from hugovkhugovk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@erlend-aasland@zware@asottile@thesamesam
[8]ページ先頭
©2009-2025 Movatter.jp