gh-115119: Removed bundled copy of the libmpdec#133964
Conversation
skirpichev
force-pushed
the
remove-bundled-mpdecimal/115119
branch
from
7cba56a to
9d6c9b5
Compare
AA-Turner
left a comment
AA-Turner
left a comment
There was a problem hiding this comment.
quick comments, haven't looked at build system changes
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
skirpichev
force-pushed
the
remove-bundled-mpdecimal/115119
branch
from
0a33f9f to
2ba6b8b
Compare
skirpichev
requested review from
a team,
StanFromIreland,
corona10,
emmatyping,
ericsnowcurrently,
erlend-aasland,
ezio-melotti,
hugovk,
sethmlarson and
webknjaz
as code owners
ned-deily
left a comment
ned-deily
left a comment
There was a problem hiding this comment.
Mac/Buildscript/build-installer.py change LGTM
| * Remove the bundled libmpdec_ decimal library from the CPython source tree, | ||
| to simplify maintenence and updates. The :mod:`decimal` module will now | ||
| unconditionally use the system's libmpdec decimal library. Also remove the | ||
| now unused :option:`!--with-system-libmpdec` :program:`configure` flag. |
There was a problem hiding this comment.
We should probably mention that binary distributions will still include the _decimal module with libmpdec compiled in.
| decimal.so is not built from a static libmpdec.a since doing so led to | ||
| failures on AIX (user report) and Windows (mixing static and dynamic CRTs | ||
| causes locale problems and more). | ||
|
|
There was a problem hiding this comment.
Nit: most of this is out of date, and there are more extra blank lines at the top of the file.
There was a problem hiding this comment.
Are you OK with removal of this?
There was a problem hiding this comment.
Either removal or paring it back to just the About section would be my preference.
There was a problem hiding this comment.
There's also a Scratch that, we still use Modules/_decimal/windows/ directory to remove, and arguably Modules/_decimal/tests/.Modules/_decimal/windows/mpdecimal.h. Still on the fence about tests/, though.
There was a problem hiding this comment.
Still on the fence about tests/, though.
Why? We don't have any tests for the decimal module in th pybenchmark. Yet, have severe regressions since 3.9+. I don't think that removal of tests is a good idea.
At least, that should go to another pr.
Co-authored-by: Zachary Ware <zachary.ware@gmail.com>
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
Reviewed the changes to sbom.spdx.json and generate_sbom.py and those changes LGTM.
zware
left a comment
zware
left a comment
There was a problem hiding this comment.
Quite close, I think, just a few more cleanups.
| decimal.so is not built from a static libmpdec.a since doing so led to | ||
| failures on AIX (user report) and Windows (mixing static and dynamic CRTs | ||
| causes locale problems and more). | ||
|
|
There was a problem hiding this comment.
Either removal or paring it back to just the About section would be my preference.
| decimal.so is not built from a static libmpdec.a since doing so led to | ||
| failures on AIX (user report) and Windows (mixing static and dynamic CRTs | ||
| causes locale problems and more). | ||
|
|
There was a problem hiding this comment.
There's also a Scratch that, we still use Modules/_decimal/windows/ directory to remove, and arguably Modules/_decimal/tests/.Modules/_decimal/windows/mpdecimal.h. Still on the fence about tests/, though.
6 participants
libmpdecsources #115119📚 Documentation preview 📚: https://cpython-previews--133964.org.readthedocs.build/