gh-115119: removed implicit fallback to the bundled libmpdec #134078
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. |
configure.ac
Outdated
| [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.
AC_MSG_ERROR? I think opting in to the pure Python version should be explicit.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Victor Stinner <vstinner@python.org>
Misc/NEWS.d/next/Build/2025-05-16-07-46-06.gh-issue-115119.ALBgS_.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Build/2025-05-16-07-46-06.gh-issue-115119.ALBgS_.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
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. |
| @@ -851,6 +851,13 @@ Libraries options | |||
|
|
|||
| .. seealso:: :option:`LIBMPDEC_CFLAGS` and :option:`LIBMPDEC_LIBS`. | |||
|
|
|||
| .. option:: --with-libmpdec | |||
There was a problem hiding this comment.
Why would we add a configure option for --with-libmpdec? We don't provide similar options for other third-party libraries.
There was a problem hiding this comment.
That's to make pure-Python configuration case for the decimal module - explicit.
In initial version - warnings was shown: #134078 (comment)
We don't provide similar options for other third-party libraries.
Are we have other similar cases? I.e. pure-Python library and it's C-extension counterpart in the stdlib.
Edit: personally I think that --with-libmpdec is not needed. In other cases (e.g. the functools module) we just silently fallback on pure-Python alternatives. I I think it's fine to do same for the decimal module at configuration time with a warning. It's pure-Python version will be always available.
There was a problem hiding this comment.
Thanks for the pointer to the earlier discussion.
Are we have other similar cases? I.e. pure-Python library and it's C-extension counterpart in the stdlib.
Not exactly the same case but there is hashlib which falls back to a built-in set of algorithms (coded in C) if the Python build was not linked with an external copy of OpenSSL's libcrypto. In that case, the make step ends with (noting other missing dependencies):
The necessary bits to build these optional modules were not found:
_gdbm _hashlib _lzma
_ssl _tkinter _zstd
To find the necessary bits, look in configure.ac and config.log.
Could not build the ssl module!
Python requires a OpenSSL 1.1.1 or newer
Checked 114 modules (35 built-in, 73 shared, 0 n/a on macosx-15.5-arm64, 0 disabled, 6 missing, 0 failed on import)
It just seems to me that adding this additional configure option is overkill especially since the goal is for the existing --with-system-libmpdec configure option to go away. In general, we should be very cautious about adding new configure options, there are too many already.
While there is the existing warning message in configure:
configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
perhaps also adding a warning at the end of the make modules step like we do for OpenSSL would be sufficient?
In any case, it would be good to update the checklist in #115119 to reflect the current plan.
There was a problem hiding this comment.
perhaps also adding a warning at the end of the make modules step like we do for OpenSSL would be sufficient?
@picnixz what do you think about reverting things back? :-) I unresolved conversation above.
Edit:
This how it looks without libmpdec:
$ ./configure -q
configure: WARNING: no system libmpdecimal found; falling back to pure-Python version for the decimal module
$ make -s
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)
In any case, it would be good to update the checklist in #115119 to reflect the current plan.
Done.
libmpdecsources #115119📚 Documentation preview 📚: https://cpython-previews--134078.org.readthedocs.build/