GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-115119: removed implicit fallback to the bundled libmpdec by skirpichev · Pull Request #134078 · python/cpython Conversation
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| [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])]) |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
2422e7d to
9690208
Compare
|
I think that I now prefer PR gh-135568: don't add |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Done. I moved this after JFR:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I am no longer sure if I like this change or not. My main worry is that some users may build Python without 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 :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merged, thank you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| * Removed implicit fallback to the bundled copy of the ``libmpdec`` library. | ||
| Now this should be explicitly enabled with :option:`--with-system-libmpdec` | ||
| set to ``no`` or with :option:`!--without-system-libmpdec`. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Does the devguide need updating? For example the macOS build instructions say to use --with-system-libmpdec, do we want people to stop using that for 3.15+?
https://devguide.python.org/getting-started/setup-building/#install-dependencies
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think we can just drop this option. It's specified in 3.13+ instructions (where it's default to "yes"). I'll prepare a patch.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Should we recommend installing libmpdec dependency in https://docs.python.org/dev/using/configure.html#build-requirements ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hmm, a good question. Here we list deps, required to build some optional modules. Though, for the decimal we always have a fallback to the pure-Python module. I think that for now we can document libmpdec dependency just like for SQLite/Tk/Tcl and not mention details (that we have a fallback).
I'm second to this. We have a lot of issues, related to differences for C-coded vs pure-Python version of the decimal module. The libmpdec now is a mature project and I don't see big reasons to keep the old pure-Python version. Nobody will prefer that in the real world - it's just adds a maintenance burden. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Ok, I created #136239 to document the dependency. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…thon#134078) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
…thon#134078) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
…thon#134078) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
…thon#134078) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
…thon#134078) Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.