Skip to content

gh-115119: removed implicit fallback to the bundled libmpdec #134078

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Jul 1, 2025
Merged

gh-115119: removed implicit fallback to the bundled libmpdec #134078

merged 18 commits into from
Jul 1, 2025
+57 −45

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented May 16, 2025 •
edited by github-actions bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request May 16, 2025
Open
15 tasks
@skirpichev
Copy link
Contributor Author

skirpichev commented May 16, 2025 •
edited
Loading

On top of configure changes in #133997.

N/B: all linux jobs, except for changed (not sure if it worth) - have no system libmpdec. For MacOS we run tests with system libmpdec.

@skirpichev skirpichev marked this pull request as ready for review May 16, 2025 06:59
AA-Turner
AA-Turner reviewed May 16, 2025
View reviewed changes
configure.ac Outdated
Comment on lines 4141 to 4144
[AC_MSG_WARN([m4_normalize([
no system libmpdecimal found; falling back to bundled libmpdecimal
(deprecated and scheduled for removal in Python 3.15)])])
USE_BUNDLED_LIBMPDEC()])
no system libmpdecimal found; falling back to pure-Python version
for the decimal module])])
AS_VAR_SET([py_cv_module_]_decimal, [n/a])])
Copy link
Member

Choose a reason for hiding this comment

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

AC_MSG_ERROR? I think opting in to the pure Python version should be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AC_MSG_ERROR ?

Well, that could be an option.

Though, more complex wrt implementation: all linux jobs will fail, unless we either provide system libmpdec or change ./configure invocations to use a new option.

I think opting in to the pure Python version should be explicit.

I'm not sure it's useful. After all, the pure-Python version is always available as the _pydecimal.py.

Did you suggest a new option like --with-purepython-decimal?

Copy link
Member

Choose a reason for hiding this comment

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

A warning is actually ok, but users may not see it and then, they would have a slow version of decimal. OTOH, an error might be too abrupt but at least the tansition would be more explicit and would force people to upgrade if they want an efficient way to do it.

I'm +0.25 for AC_MSG_ERROR just to force people to update. We can add an option --allow-fallback-to-pydecimal to handle CI possible failures, though this means that devs should be aware that their C code might not be tested in the CI due to that.

@skirpichev skirpichev requested review from vstinner and picnixz May 30, 2025 05:41
picnixz

This comment was marked as resolved.

Sign in to view

@skirpichev skirpichev mentioned this pull request Jun 2, 2025
Open
vstinner
vstinner reviewed Jun 3, 2025
View reviewed changes
vstinner
vstinner reviewed Jun 3, 2025
View reviewed changes
@skirpichev @vstinner
Update .github/workflows/posix-deps-apt.sh
6cd1730 
Co-authored-by: Victor Stinner <vstinner@python.org>
hugovk
hugovk reviewed Jun 9, 2025
View reviewed changes
@skirpichev skirpichev requested a review from hugovk June 10, 2025 02:15
vstinner
vstinner reviewed Jun 10, 2025
View reviewed changes
@hugovk hugovk removed their request for review June 10, 2025 15:56
@vstinner
Copy link
Member

@AA-Turner @picnixz @hugovk: Do you think that this change is ready to be merged?

cc @erlend-aasland @corona10 who modify configure tools sometimes.

picnixz
picnixz approved these changes Jun 13, 2025
View reviewed changes
@picnixz
Copy link
Member

picnixz commented Jun 13, 2025

I see a double review but I'm on mobile. Sorry if it got double posted. I'm ok with this change, modulo some wording nits.

ned-deily
ned-deily reviewed Jun 13, 2025
View reviewed changes
@skirpichev skirpichev force-pushed the remove-fallback-to-bundled-mpdec/115119 branch 2 times, most recently from 2422e7d to 9690208 Compare June 16, 2025 15:03
@vstinner

This comment was marked as resolved.

Sign in to view
@vstinner
Copy link
Member

I think that I now prefer PR gh-135568: don't add --with-libmpdec option. I don't think that ./configure should fail if mpdecimal library is missing, but a warning should be written at the end of the ./configure script.

@skirpichev
Copy link
Contributor Author

warning should be written at the end of the ./configure script.

Done. I moved this after _decimal module check at the end.

JFR:

  1. No system libmpdec, default:
$ ./configure --with-pydebug 2>&1|grep -i mpd
checking for --with-system-libmpdec... yes
checking for libmpdec >= 2.5.0... no
checking for rl_compdisp_func_t... yes
configure: WARNING: no system libmpdecimal found; falling back to pure-Python version for the decimal module
$ make -s
Written build/lib.linux-x86_64-3.15/_sysconfigdata_d_linux_x86_64-linux-gnu.py
Written build/lib.linux-x86_64-3.15/_sysconfig_vars_d_linux_x86_64-linux-gnu.json
The necessary bits to build these optional modules were not found:
_decimal
To find the necessary bits, look in configure.ac and config.log.

Checked 114 modules (35 built-in, 77 shared, 1 n/a on linux-x86_64, 0 disabled, 1 missing, 0 failed on import)
  1. No system libmpdec, with bundled:
$ ./configure --with-pydebug --without-system-libmpdec 2>&1|grep -i mpd
checking for --with-system-libmpdec... no
configure: WARNING: the bundled copy of libmpdecimal is scheduled for removal in Python 3.16; consider using a system installed mpdecimal library.
checking for decimal libmpdec machine... uint128
checking for rl_compdisp_func_t... yes
  1. System libmpdec, with bundled:
$ ./configure --with-pydebug --without-system-libmpdec 2>&1|grep -i mpd
checking for --with-system-libmpdec... no
configure: WARNING: the bundled copy of libmpdecimal is scheduled for removal in Python 3.16; consider using a system installed mpdecimal library.
checking for decimal libmpdec machine... uint128
checking for rl_compdisp_func_t... yes
  1. System libmpdec, default:
$ ./configure --with-pydebug 2>&1|grep -i mpd
checking for --with-system-libmpdec... yes
checking for libmpdec >= 2.5.0... yes
checking for rl_compdisp_func_t... yes

@skirpichev skirpichev requested review from vstinner and ned-deily June 17, 2025 13:54
ned-deily
ned-deily approved these changes Jun 18, 2025
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. I think this solution makes for a better user experience.

@vstinner
Copy link
Member

I am no longer sure if I like this change or not. My main worry is that some users may build Python without _decimal and would silently get a 100x slower decimal module. On the other hand, I don't consider that decimal is part of the most important stdlib modules, so it's ok if decimal is (way) slower. Many users don't use decimal at all.

I would prefer to keep mpdecimal as an optional dependency of Python. And so it's good that the configure script doesn't fail if the dependency is missing.

So at the end, I think that I like this change :-)

vstinner
vstinner approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@vstinner 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

@skirpichev skirpichev requested a review from picnixz June 28, 2025 16:37
@vstinner vstinner merged commit 17cf0a3 into python:main Jul 1, 2025
39 checks passed
@vstinner
Copy link
Member

vstinner commented Jul 1, 2025

Merged, thank you.

@skirpichev skirpichev deleted the remove-fallback-to-bundled-mpdec/115119 branch July 1, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk left review comments

@AA-Turner AA-Turner AA-Turner left review comments

@vstinner vstinner vstinner approved these changes

@ned-deily ned-deily ned-deily approved these changes

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@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

@picnixz picnixz Awaiting requested review from picnixz

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

Successfully merging this pull request may close these issues.
6 participants
@skirpichev @vstinner @picnixz @hugovk @ned-deily @AA-Turner