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

Merged
merged 18 commits into from
Jul 1, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
14266c8
gh-115119: defer removal of bundled libmpdec to 3.16 (configure changes)
skirpichev May 14, 2025
39d1e6b
gh-115119: removed implicit fallback to the bundled libmpdec
skirpichev May 16, 2025
b328702
address review: remove runner version downgrade
skirpichev May 16, 2025
94ce315
+ use libmpdec-dev from PPA for PHP
skirpichev Jun 3, 2025
fed39e1
Merge branch 'master' into remove-fallback-to-bundled-mpdec/115119
skirpichev Jun 3, 2025
4c98b8d
address review: add --with-libmpdec option
skirpichev Jun 3, 2025
6cd1730
Update .github/workflows/posix-deps-apt.sh
skirpichev Jun 3, 2025
9d29df1
Apply suggestions from code review
skirpichev Jun 10, 2025
b733718
Merge branch 'master' into remove-fallback-to-bundled-mpdec/115119
skirpichev Jun 10, 2025
d53bdf3
address review: reorder list in posix-deps-apt.sh
skirpichev Jun 10, 2025
c3a1889
address review: expand --with-libmpdec description
skirpichev Jun 10, 2025
45544d9
address review: add link to option
skirpichev Jun 13, 2025
8413e6f
Merge branch 'master' into remove-fallback-to-bundled-mpdec/115119
skirpichev Jun 14, 2025
9690208
address review: redo posix-deps-apt.sh workaround (install last)
skirpichev Jun 14, 2025
f0e6757
Merge branch 'master' into remove-fallback-to-bundled-mpdec/115119
skirpichev Jun 17, 2025
1eca085
Revert "address review: add --with-libmpdec option"
skirpichev Jun 14, 2025
73f0729
address review: move warning in the configure
skirpichev Jun 17, 2025
cece14c
+1
skirpichev Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ jobs:
name: 'Check if generated files are up to date'
# Don't use ubuntu-latest but a specific version to make the job
# reproducible: to get the same tools versions (autoconf, aclocal, ...)
runs-on: ubuntu-24.04
runs-on: ubuntu-22.04
timeout-minutes: 60
needs: build-context
if: needs.build-context.outputs.run-tests == 'true'
Expand All @@ -110,7 +110,9 @@ jobs:
# Include env.pythonLocation in key to avoid changes in environment when setup-python updates Python
key: ${{ github.job }}-${{ env.IMAGE_OS_VERSION }}-${{ needs.build-context.outputs.config-hash }}-${{ env.pythonLocation }}
- name: Install dependencies
run: sudo ./.github/workflows/posix-deps-apt.sh
run: |
sudo ./.github/workflows/posix-deps-apt.sh
sudo apt-get -yq install libmpdec-dev
- name: Add ccache to PATH
run: echo "PATH=/usr/lib/ccache:$PATH" >> "$GITHUB_ENV"
- name: Configure ccache action
Expand Down
5 changes: 5 additions & 0 deletions Doc/whatsnew/3.15.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,11 @@ that may require changes to your code.
Build changes
=============

* Removed implicit fallback to the buildled 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`.
Copy link
Member

Choose a reason for 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

Copy link
Contributor Author

Choose a reason for 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.

(Contributed by Sergey B Kirpichev in :gh:`115119`.)


C API changes
=============
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Build/2025-05-16-07-46-06.gh-issue-115119.ALBgS_.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Removed implicit fallback to the buildled 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`. Patch by Sergey
B Kirpichev.
38 changes: 17 additions & 21 deletions configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 16 additions & 16 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4092,31 +4092,30 @@ fi

# Check for use of the system libmpdec library
AC_MSG_CHECKING([for --with-system-libmpdec])
AC_DEFUN([USE_BUNDLED_LIBMPDEC],
[LIBMPDEC_CFLAGS="-I\$(srcdir)/Modules/_decimal/libmpdec"
LIBMPDEC_LIBS="-lm \$(LIBMPDEC_A)"
LIBMPDEC_INTERNAL="\$(LIBMPDEC_HEADERS) \$(LIBMPDEC_A)"
have_mpdec=yes
with_system_libmpdec=no])
AC_ARG_WITH(
[system_libmpdec],
[AS_HELP_STRING(
[--with-system-libmpdec],
[build _decimal module using an installed mpdecimal library, see Doc/library/decimal.rst (default is yes)]
)],
[],
[AS_IF([test "x$with_system_libmpdec" = xno],
[USE_BUNDLED_LIBMPDEC()])],
[with_system_libmpdec="yes"])
AC_MSG_RESULT([$with_system_libmpdec])

AC_DEFUN([USE_BUNDLED_LIBMPDEC],
[LIBMPDEC_CFLAGS="-I\$(srcdir)/Modules/_decimal/libmpdec"
LIBMPDEC_LIBS="-lm \$(LIBMPDEC_A)"
LIBMPDEC_INTERNAL="\$(LIBMPDEC_HEADERS) \$(LIBMPDEC_A)"
have_mpdec=yes
with_system_libmpdec=no])

AS_VAR_IF(
[with_system_libmpdec], [yes],
[PKG_CHECK_MODULES(
[LIBMPDEC], [libmpdec >= 2.5.0], [],
[LIBMPDEC_CFLAGS=${LIBMPDEC_CFLAGS-""}
LIBMPDEC_LIBS=${LIBMPDEC_LIBS-"-lmpdec -lm"}
LIBMPDEC_INTERNAL=])],
[USE_BUNDLED_LIBMPDEC()])
LIBMPDEC_INTERNAL=])])

AS_VAR_IF([with_system_libmpdec], [yes],
[WITH_SAVE_ENV([
Expand All @@ -4132,16 +4131,17 @@ AS_VAR_IF([with_system_libmpdec], [yes],
], [const char *x = mpd_version();])],
[have_mpdec=yes],
[have_mpdec=no])
])],
[AC_MSG_WARN([m4_normalize([
the bundled copy of libmpdecimal is scheduled for removal in Python 3.15;
])])
AS_VAR_IF([with_system_libmpdec], [no],
[AC_MSG_WARN([m4_normalize([
the bundled copy of libmpdecimal is scheduled for removal in Python 3.16;
consider using a system installed mpdecimal library.])])])

AS_IF([test "$with_system_libmpdec" = "yes" && test "$have_mpdec" = "no"],
[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.


# Disable forced inlining in debug builds, see GH-94847
AS_VAR_IF(
Expand Down
Loading