Update to libmpdec-2.5.0

[skrah]

BPO	40874
Nosy	@rhettinger, @doko42, @mdickinson, @pitrou, @skrah, @ambv, @asottile, @miss-islington
PRs	bpo-40874: Update to libmpdec-2.5.0 (GH-20652) #20652 [3.9] bpo-40874: Update to libmpdec-2.5.0 (GH-20652) #20654 bpo-40874 Update the required libmpdec version for the decimal module #21202 [3.9] bpo-40874 Update the required libmpdec version for the decimal module (GH-21202) #21203

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/skrah'
closed_at = <Date 2020-07-03.17:46:22.856>
created_at = <Date 2020-06-05.16:29:23.307>
labels = ['extension-modules', 'type-bug', '3.9', '3.10']
title = 'Update to libmpdec-2.5.0'
updated_at = <Date 2020-07-06.11:31:49.619>
user = 'https://github.com/skrah'

bugs.python.org fields:

activity = <Date 2020-07-06.11:31:49.619>
actor = 'skrah'
assignee = 'skrah'
closed = True
closed_date = <Date 2020-07-03.17:46:22.856>
closer = 'skrah'
components = ['Extension Modules']
creation = <Date 2020-06-05.16:29:23.307>
creator = 'skrah'
dependencies = []
files = []
hgrepos = []
issue_num = 40874
keywords = ['patch']
message_count = 58.0
messages = ['370766', '370774', '370780', '372530', '372532', '372534', '372579', '372580', '372581', '372584', '372585', '372586', '372587', '372589', '372591', '372593', '372597', '372598', '372608', '372609', '372613', '372614', '372615', '372616', '372617', '372618', '372620', '372623', '372626', '372629', '372632', '372635', '372917', '372918', '372919', '372920', '372921', '372922', '372924', '372926', '372928', '372930', '372931', '372932', '372935', '372943', '372944', '372945', '372946', '372947', '372948', '373001', '373003', '373005', '373021', '373093', '373094', '373096']
nosy_count = 8.0
nosy_names = ['rhettinger', 'doko', 'mark.dickinson', 'pitrou', 'skrah', 'lukasz.langa', 'Anthony Sottile', 'miss-islington']
pr_nums = ['20652', '20654', '21202', '21203']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue40874'
versions = ['Python 3.9', 'Python 3.10']

[skrah]

Synopsis: There are no relevant new features for _decimal, but it would be too much work/error prone to have divergent code in libmpdec-2.5.0 and Python 3.9, especially for the Linux distributions.

I'll release libmpdec-2.5.0/libmpdec++-2.5.0 in a month or so. The standalone lib needs the new versions of mpd_qsqrt() and mpd_qdiv(), because it allows identical result/input args. This is not needed for _decimal, but the distributions should have the correct version.

In detail
=========

• Use Google style guide for header guards and includes.

• Update mpdecimal.h for C++11.

• Use minimum set of includes.

• Whitespace fixes.

• Add annotations to suppress false positives from static analyzers.

• Small rewrite in base conversion functions to suppress false positives
  from static analyzers.

• MSVC: make libmpdec /W4 warning free and replace UNUSED with void casts.

• MSVC: C++ fixes in vccompat.h.

• Make a couple of quiet functions safe for being called with a dirty status
  (irrelevant for _decimal and not recommended anyway -- always set the status
  to 0 before calling a quiet function).

• Add the sqrt/div versions that are already in the Python libmpdec but not
  in the upstream libmpdec. Also make them safe for identical result/operand
  arguments (irrelevant for _decimal, since Decimals are immutable).

New functions for the upcoming libmpdec++ (unused in _decimal)
==============================================================

• mpd_qset_string_exact()

• mpd_qset_i64_exact()

• mpd_qset_u64_exact()

• mpd_qcopy_cxx()

[skrah]

New changeset 087d612 by Stefan Krah in branch 'master':
bpo-40874: Update to libmpdec-2.5.0 (GH-20652)
087d612

[skrah]

New changeset 83bff88 by Miss Islington (bot) in branch '3.9':
bpo-40874: Update to libmpdec-2.5.0 (GH-20652)
83bff88

[skrah]

New changeset 8bea91b by Stefan Krah in branch 'master':
bpo-40874 Update the required libmpdec version for the decimal module (GH-21202)
8bea91b

[skrah]

New changeset 119de0e by Miss Islington (bot) in branch '3.9':
bpo-40874 Update the required libmpdec version for the decimal module (GH-21202)
119de0e

[skrah]

A very brief guide for all users of --with-system-libmpdec:

Python 3.7 and Python 3.8 both require the release with this sha256sum:

83c628b90f009470981cf084c5418329c88b19835d8af3691b930afccb7d79c7 mpdecimal-2.4.2.tar.gz

Python 3.9 requires the release with this sha256sum:

15417edc8e12a57d1d9d75fa7e3f22b158a3b98f44db9d694cfd2acde8dfa0ca mpdecimal-2.5.0.tar.gz

[asottile]

this breaks builds for ubuntu, I'd suggest reverting this (especially because it appears to build fine without this patch)

2020-06-29T08:52:56.8303672Z x86_64-linux-gnu-gcc -pthread -fPIC -Wdate-time -D_FORTIFY_SOURCE=2 -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -g -fdebug-prefix-map=/tmp/code=. -specs=/usr/share/dpkg/no-pie-compile.specs -fstack-protector -Wformat -Werror=format-security -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I../Include/internal -DCONFIG_64=1 -DASM=1 -I../Include -IObjects -IPython -I. -I/usr/include/x86_64-linux-gnu -I/tmp/code/Include -I/tmp/code/build-static -c /tmp/code/Modules/_decimal/_decimal.c -o build/temp.linux-x86_64-3.9/tmp/code/Modules/_decimal/_decimal.o
2020-06-29T08:52:56.9003112Z /tmp/code/Modules/_decimal/_decimal.c:40:4: error: #error "libmpdec version >= 2.5.0 required"
2020-06-29T08:52:56.9005053Z #error "libmpdec version >= 2.5.0 required"
2020-06-29T08:52:56.9006190Z ^~~~~

[asottile]

especially this late in the beta period for 3.9 -- it would be unfortunate for 3.10 but it's an explicit break of feature freeze for 3.9

[skrah]

Ubuntu is my main system, and it does not break the build.

If you use --with-system-libmpdec, you need to keep in sync with
the external libmpdec.

[asottile]

reverting this patch passes all the tests, what's the motivation and why were there no code reviews for this?

[skrah]

Please name a buildbot that does not pass.

[skrah]

Or is this CoC bait again?

[asottile]

I package pythons for ubuntu:

https://github.com/deadsnakes/python3.9-nightly/actions/runs/151286686
https://github.com/deadsnakes/python3.10-nightly/actions/runs/151287821

[skrah]

If Python is packaged with **system** libmpdec, you can only use
the official Ubuntu Python/libmpdec version combination.

Which are packaged by Matthias Klose, who is on the CC list here.

Otherwise, why use the system libmpdec at all and not the version
shipped with Python?

If I install into a venv, I also don't use the system libmpdec.

[asottile]

Otherwise, why use the system libmpdec at all and not the version
shipped with Python?

the packages are faithful reproductions of upstream packages, deviating from those introduces surprises for downstreams

If I install into a venv, I also don't use the system libmpdec.

that's simply not true:

$ python3.9 -m venv vvv
$ vvv/bin/python -c 'import _decimal, subprocess; subprocess.check_call(("ldd", _decimal.__file__))'
	linux-vdso.so.1 (0x00007fff429ec000)
	libmpdec.so.2 => /lib/x86_64-linux-gnu/libmpdec.so.2 (0x00007fcaeae03000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fcaeade0000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fcaeabee000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fcaeaa9f000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fcaeae7f000)

[skrah]

You are talking to the author of libmpdec *and* the _decimal module.
Perhaps this problem has occurred to me, too:

$ diff -ur mpdecimal-2.5.0/libmpdec cpython-commit/Modules/_decimal/libmpdec/
Only in mpdecimal-2.5.0/libmpdec: .objs
Only in mpdecimal-2.5.0/libmpdec: Makefile.in
Only in mpdecimal-2.5.0/libmpdec: Makefile.vc
diff -ur mpdecimal-2.5.0/libmpdec/README.txt cpython-commit/Modules/_decimal/libmpdec/README.txt
--- mpdecimal-2.5.0/libmpdec/README.txt 2020-06-27 21:41:49.000000000 +0200
+++ cpython-commit/Modules/_decimal/libmpdec/README.txt 2020-06-29 13:46:45.379299458 +0200
@@ -8,6 +8,9 @@
 Mike Cowlishaw/IBM's General Decimal Arithmetic Specification.
 
 
+Files required for the Python _decimal module
+

=============================================
+
Core files for small and medium precision arithmetic
----------------------------------------------------

Only in mpdecimal-2.5.0/libmpdec: bench.c
Only in mpdecimal-2.5.0/libmpdec: bench_full.c
Only in mpdecimal-2.5.0/libmpdec: examples
Only in cpython-commit/Modules/_decimal/libmpdec/: mpdecimal.h
Only in mpdecimal-2.5.0/libmpdec: mpdecimal.h.in
Only in mpdecimal-2.5.0/libmpdec: mpdecimal32vc.h
Only in mpdecimal-2.5.0/libmpdec: mpdecimal64vc.h
Only in mpdecimal-2.5.0/libmpdec: mpsignal.c

You are the one here who wants to ship a non-recommended libmpdec
version with Python-3.9.

[skrah]

This is no release blocker. Version 2.5.0 has been in 3.9 for a long time, and people should use the correct version.

[skrah]

--with-system-libmpdec is a **long term** tool for distributions.

Pinning the version number ensures that they use the correct version.

[ambv]

Stefan brought libmpdec-2.5.0 to 3.9 shortly before Beta 2. Due to us being busy with the importlib fiasco, this went under our radar. It shouldn't have. It's a large chunk of refactored code merged without review after the beta freeze. Betas aren't for changing style guides and language standards, etc. etc.

Sadly, given that this already got released as part of Beta 2 and Beta 3 shortly after, I think at this point it's pointless to revert it. And the new PR simply requires a shared version to match the one that we are bundling now in sources, reverting just this one is out of the question.

Or is this CoC bait again?

Stefan, please, don't be like that. What purpose does this serve?

Anthony noted a new failure related to your unreviewed and under-documented commit. You slipped in libmpdec-2.5.0 during the beta freeze without any discussion. It catches up to you right now. No reason to shoot the messenger.

[asottile]

Łukasz: what would you recommend for downstream packagers?

I have essentially two options (assuming this isn't reverted in cpython master which I believe makes the most sense since cpython still works fine with older libmpdec):

• revert this individual commit as a patch
• fork further from debian's packaging and use the vendored libmpdec (this potentially carries other negative side-effects since it makes the packages unlike upstream's -- and potentially susceptible to vendor/security drift since we don't carry patches for vendored components (but would get them from system libmpdec))

I'd rather not carry that patch indefinitely if possible -- especially when this cannot be built in a compatible way on software that's a mere 2 months old (ubuntu 20.04). For similar libraries (ssl, ffi, etc.) this would be a very major break in building.

[skrah]

I was accused of breaking the release, which is false. It is outside
of a release manager's authority to claim that an *experimental* and
*nightly* build that uses a flag in an unintended manner needs counts
as breakage.

[skrah]

Raymond, Mark, Antoine: If you think this should be reverted, I'll
revert it.

[skrah]

backwards incompatible changes.

It is not a backwards incompatible change. Slamming a 3.9 into
a nightly build has never been supported.

[doko42]

I'm +-0 on if that should be integrated into 3.9. Only a few people are using --with-system-libmpdec. However the way that mpdecimal 2.4 and 2.5 are released, they are not usable for Debian or Ubuntu for the transition from 3.8 to 3.9. For that, both 3.8 and 3.9 have to be available on the systems for the transition period, and that's just not possible without mpdecimal changing it's soname, or building one Python version with the internal libmpdec.

[skrah]

Since this issue has been brought to the attention of the CoC
committee. Here is how *I* report issues:

https://bugs.python.org/issue40223#msg372578
https://bugs.python.org/issue40223#msg372637
google/sanitizers#1257

Note that I do not go straight into accusing people, especially
in uncertain cases. I never have any problems with my bug reports.

[skrah]

Matthias, to tell the truth, I was never sure about the soname.
I read this:

https://www.debian.org/doc/debian-policy/ch-sharedlibs.html

"""
The SONAME and binary package name need not, and indeed normally should not, change if new interfaces are added but none are removed or changed, since this will not break binaries linked against the old shared library. Correct versioning of dependencies on the newer shared library by binaries that use the new interfaces is handled via the symbols or shlibs system (see Dependencies between the library and other packages).
"""

I took the interface to mean ABI, which did not change. Also this:

"""Every time the shared library ABI changes in a way that may break binaries linked against older versions of the shared library, the SONAME of the library and the corresponding name for the binary package containing the runtime shared library should change. Normally, this means the SONAME should change any time an interface is removed from the shared library or the signature of an interface (the number of parameters or the types of parameters that it takes, for example) is changed. This practice is vital to allowing clean upgrades from older versions of the package and clean transitions between the old ABI and new ABI without having to upgrade every affected package simultaneously.
"""

So I left the soname at 2.

[skrah]

In the mean time may I request that you follow our protocol for code review.

Ah, who reviews libffi? It is just updated. Not counting the thousands of other cases people commit unreviewed, like in changing the C-API.

And no, Christian, that isn't "whataboutism", that is comparative analysis.

[skrah]

both 3.8 and 3.9 have to be available on the systems for the transition period.

If sonames can be incremented for libraries even if they are ABI compatible, how about using up as many as you need for the Debian
package? Next time when I release mpdecimal, I'll ask you about
the highest version that's in use at Debian and increment it by
one.

[ambv]

And a release manager who has no libmpdec expertise or authority took the side of the "bug" reporter without much thought.

What is this elusive "authority" you keep bringing up?

Note that I do not go straight into accusing people, especially in uncertain cases.

Neither did Anthony. He observed breakage in his builds and reported it. He noted that the change happened during the beta freeze which is documented to only allow bug fixes:

https://devguide.python.org/devcycle/#beta

Anthony's only fault here was depending on the system libmpdec which you claim is invalid use. As he explained, he did this to mirror behavior of the official Python packages. And it worked for the first three betas. His surprise breakage report wasn't unreasonable, let alone "petulant". Compare with your own responses which to many of us read unnecessarily defensive.

Nobody is challenging your competence. The problem is entirely with the timing and making non-bugfix changes during the beta phase. Bringing up credentials, track records, or listing professional networks doesn't change that.

Finally, while Raymond and Antoine are welcome to voice their opinions on the matter, your change is landing in 3.9.0b4 which I'm about to announce. So we won't be reverting it. In the future let's make sure we stick to the release calendar to avoid similar heat. If we need to bend a rule or two, that's okay, it happens. Making a fellow core developer stamp your change in such case will increase visibility, and is a good practice regardless, required for example in avionics software.

[skrah]

Łukasz, which one is nicer?

reverting this patch passes all the tests, what's the motivation and why were there no code reviews for this?

or:

Yeah, I already felt a bit guilty about adding you -- it could be a compiler bug or an actual overflow. My bet is also that the reordering exposes an existing overflow. The reordering itself certainly looks correct to me.

I'm always astonished that some of the CoC proponents (and reporters)
have little idea about actual politeness, a fact which is a main source
of friction.

Hint: I'd prefer actual politeness.

[skrah]

Finally, about the Debian issue, of course you could also link 3.8
against the static lib.

[skrah]

Finally, while Raymond and Antoine are welcome to voice their opinions on the matter, your change is landing in 3.9.0b4 which I'm about to announce. So we won't be reverting it. In the future let's make sure we stick to the release calendar to avoid similar heat. If we need to bend a rule or two, that's okay, it happens. Making a fellow core developer stamp your change in such case will increase visibility, and is a good practice regardless, required for example in avionics software.

I've added Antoine, Mark and Raymond because they know the history of
libmpdec, unlike people who came later.

Like for libffi, it is not feasible to get review, because there is
no time. This would effectively mean that nothing ever changes.

The alternative is to trust that the zero fault situation continues.

Or download *one* of the gigantic test suites, which I have laboriously
updated for this release:

http://www.bytereef.org/software/mpdecimal/releases/mpdecimal-testit-2.5.0.tar.gz

The second one isn't even published.

So again, just clamoring for review (which often is just rubber
stamping) leads to nothing but scoring a few cheap points.

[tiran]

Are you saying that you are not follow advises and guidelines of the developer guide?

[skrah]

How witty!

[pitrou]

Hmm, I'm only taking notice of this comment thread now.

(sorry, but due to spam filtering issues I only receive bpo e-mail notifications intermittently... and that's despite having tried two separate e-mail providers which otherwise give me no problems :-/)

In any case, I would have had a hard time giving a competent opinion on this issue. But I'm a bit saddened by how heated the discussion went. Hopefully this is all over.

[skrah]

In any case, I would have had a hard time giving a competent opinion on this issue.

Essentially it's a really simple Linux packaging issue for the
external libmpdec. To have the exact same behavior for the external
libmpdec as for the included libmpdec, packagers must use:

3.8 <--> 2.4.2
3.9 <--> 2.5.0

ArchLinux had no problems. Debian, and by extension Ubuntu, requires
3.8 and 3.9 to be on the same system during a transitional period, as
pointed out in msg372928 (which is really the most important message
of this whole thread).

The commit that pinned _decimal to libmpdec version 2.5.0 broke this
use case, but there are workarounds.

My stance is that it is important that libmpdec is pinned so distros
don't use a divergent version. Since there are multiple mitigations
for Debian, I don't feel particularly guilty.

Review of the commit that pinned 2.5.0 would have led to the exact
same outcome: I would have pointed that out on GitHub.

Note that with the Debian scheme there is never a good time to
update libmpdec, regardless of the release cycle.

[pitrou]

Thanks for the clarification. I agree this does not seem to be a very big deal, if slightly annoying for the packager who will have to deal with it.

[skrah]

Thanks for taking a look, Antoine.

IIRC, the version pinning already accommodates Debian:

#if !defined(MPD_VERSION_HEX) || MPD_VERSION_HEX < 0x02050000
  #error "libmpdec version >= 2.5.0 required"
#endif

In the first libmpdec versions, I had a stricter equality check,
and I *think* (but I'm not 100% sure) that it was Matthias who
requested the relaxation.

Based on that, perhaps Debian should just use 2.5.0 for both
3.8 and 3.9 in the transition period.

I'm more comfortable with 3.8 using 2.5.0 than 3.9 using 2.4.2.

[skrah]

As noted in the first message of this thread, the sqrt-max-prec feature
(requested by Mark and Tim) was already in 3.9 long before the beta
freeze.

I'm not sure why this is not clear from the original message.

That fix is safe for Python, but not for the standalone libmpdec.

The standalone libmpdec had to be updated, and was updated according
to the Debian-friendly way requested by Matthias himself.

Note that a pinning issue in another area of Python has surfaced in
the last 24 hours. I wonder if the reaction will be a strong as here.

[skrah]

The standalone libmpdec had to be updated, and was updated according
to the Debian-friendly way requested by Matthias himself.

Not updated, of course the sqrt-max-prec *had never been* in
the standalone libmpdec, it is new in 2.5.0.

[skrah]

More than that, I had *promised* Matthias privately to release a
new libmpdec for the sqrt-max-prec feature a couple of months ago.

I request that further packaging issues will be dealt with primarily
by Matthias and myself.