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()

New changeset 087d612efebe7c64e5f079b07e0454111859830e by Stefan Krah in branch 'master':
bpo-40874: Update to libmpdec-2.5.0 (GH-20652)
https://github.com/python/cpython/commit/087d612efebe7c64e5f079b07e0454111859830e

New changeset 83bff88b4b16fb30491faa9263bbd6f3df4bab56 by Miss Islington (bot) in branch '3.9':
bpo-40874: Update to libmpdec-2.5.0 (GH-20652)
https://github.com/python/cpython/commit/83bff88b4b16fb30491faa9263bbd6f3df4bab56

New changeset 8bea91b5e9ea07ca93958e131b436024f0b1b1cf by Stefan Krah in branch 'master':
bpo-40874 Update the required libmpdec version for the decimal module (GH-21202)
https://github.com/python/cpython/commit/8bea91b5e9ea07ca93958e131b436024f0b1b1cf

New changeset 119de0eba839993cf6a909dba5d60202ad5566d6 by Miss Islington (bot) in branch '3.9':
bpo-40874 Update the required libmpdec version for the decimal module (GH-21202) 
https://github.com/python/cpython/commit/119de0eba839993cf6a909dba5d60202ad5566d6

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

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     ^~~~~

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

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.

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

Please name a buildbot that does not pass.

Or is this CoC bait again?

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

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.

> 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)

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.

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.

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

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

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.

Ł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.

> Anthony noted a new failure related to your unreviewed and under-documented commit.

He claimed a failure in Ubuntu (in a manner that I took as petulant),
which isn't the case.  It is a failure in a custom Ubuntu distro
that uses --with-system-libmpdec in an unintended way.


> You slipped in libmpdec-2.5.0 during the beta freeze without any discussion.

I did not slip it in at all. You were on the nosy list from the
very start of this issue, and so was Matthias.


> It catches up to you right now. No reason to shoot the messenger.


It catches up to people who use --with-system-libmpdec in an
unintended manner.

It is intended for a scenario like merging Python 3.9 into Sid,
then testing, then stable.

> unreviewed and under-documented commit.

libmpdec has been one of the few zero fault areas of Python.
Please stop spreading FUD.

> > unreviewed and under-documented commit.
>
> libmpdec has been one of the few zero fault areas of Python.
> Please stop spreading FUD.

This has nothing to do with your excellent fault rate (lack of any issues).

The commit had no Misc/NEWS.d blurb and therefore was undocumented. I have opened bpo-41161 to address the issue. There was also no approval or review from another core dev on GH-20652.

> This has nothing to do with your excellent fault rate (lack of any issues).

I sounds like that though for random people who read this issue and think
that Łukasz is the grand release manager who puts a person in his place.

That was incredibly inappropriate.  libmpdec has **found** issues in
decNumber that were acknowledged by Cowlishaw.

If one's reputation is questioned every time one opens an issue here
(notably by people who are not subject experts at all), then I *have*
to respond in no uncertain terms.

> I have opened bpo-41161 to address the issue.

Thanks, that is a more considerate approach, I'll add the note!

Also note that people did not react at all to the fact that
coroutine storage was not thread safe across several releases.

No one asked for News entry.

But "breaking" a fringe distro seems to be a major disaster.

> Also note that people did not react at all to the fact that
> coroutine storage was not thread safe across several releases.

That's whataboutism.

> No one asked for News entry.

The bot did ask you to add a news entry. According to https://devguide.python.org/triaging/#github-labels-for-prs the "skip news" label should only be used in trivial cases like fixing a typo.

> But "breaking" a fringe distro seems to be a major disaster.

I find your choice of words hurtful and it demeaning.

Other core developer go through great length to keep backwards compatibility for older or less commonly used systems. In bpo-40810 Erlend researched SQLite version numbers before support for support for ancient SQLite was dropped. I'm spending a considerable effort to keep Python working with unsupported but still widely used OpenSSL and LibreSSL versions.

> The bot did ask you to add a news entry.

And I deliberately did not, out of politeness. Two release managers
were added and they did not ask.

> Other core developer go through great length to keep backwards compatibility for older or less commonly used systems.

Ok, I take that back then.  I go through great lengths to keep
libmpdec and Python in sync.

Major correction: Victor *did* ask for a news entry, but otherwise
I would not have added one.

> That was incredibly inappropriate.

You'll have to let the readers of this thread judge that for themselves.

You're right that you added me to nosy. I missed it as I've been busy with other things at the time. You let *3 hours* pass between opening this issue and merging the original backport PR into Python 3.9.

Python users and fellow core developers value your contributions. I personally could not have done them myself. We only ask you to try to play along. For example by letting other core devs stamp non-trivial non-bugfix contributions late in the release cycle. Let's not make this unnecessarily hostile.

> You'll have to let the readers of this thread judge that for themselves.

Ask Cowlishaw or the mpfr developers to read this thread.


As for politeness, msg372581 was entirely polite and directly answered
by an inappropriate and petulant msg372584.

This no way to talk to an experienced core developer.

Since this is an ongoing problem:  When I submitted the decNumber
patches to Cowlishaw, he asked me if I would be interested in
maintaining decNumber.  I declined at the time due to time constraints.

Had I accepted, I'd control 2/3 of the decimal market now, the Intel
library being the remaining 1/3.


I'm unwilling to justify my competence to random bug reporters in
this forum, especially when I'm nice to them initially.

It is instructive that ArchLinux quietly and professionally packaged
mpdecimal-2.5.0 hours after the release:

https://www.archlinux.org/packages/?sort=&q=mpdecimal&maintainer=&flagged=


This is in stark contrast with Python-dev, where a maintainer of an *experimental*
*nightly* Ubuntu package that abuses --with-system-libmpdec claimed that "Ubuntu
builds are broken".  And a release manager who has no libmpdec expertise or authority
took the side of the "bug" reporter without much thought.


I presume that ArchLinux still follows the "shut up and hack" philosophy.

As soon as a Python release enters beta phase, all dependencies are locked. Since Python 3.9.0-b1 supported --with-system-libmpdec with libmpdec-2.4.2, all future releases of 3.9.0 until EOL of 3.9 branch have to support 2.4.2. Since 3.9.0-b3 does not work with libmpdec-2.4.2 you have introduced a regression.

It's always possible to drop support for older versions of a dependency. A developer has to make a formal case for deprecation, provide technical arguments why an old version of a dependency cannot be support without great effort, and get written permission from the release manager. It's the release manager's discretion what they consider a regression. It's well in their power and authority to refuse and even revert a change if a release manager considers a change a regression. A developer can call upon the steering council to mediate.

Any kind of personal attacks on community members, core developers, or release manage is not acceptable way to resolve a dispute.

I was the one being attacked in this issue, while releasing a zero
fault library whose release procedures resemble those found in
avionics software.

Christian, you are also completely ignoring the original attack
of Anthony, so you are biased and there is no point continuing.

> I was the one being attacked in this issue, while releasing a zero
fault library whose release procedures resemble those found in
avionics software.

I have reported this incident to the steering council and Code of Conduct working group. I let them decide who attacked whom.

In the mean time may I request that you follow our protocol for code review and backwards incompatible changes? There is no "Quod licet Iovi, non licet bovi" in Python (except that RM is close to the power of Zeus). Everybody has to follow the rules no matter who you are.

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.

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

> backwards incompatible changes.

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

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.

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
   https://github.com/google/sanitizers/issues/1257


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

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.

> 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.

> 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.

> 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.

Ł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.

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

> 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.

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

How witty!

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.

> 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.

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.

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.

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.

> 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.

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.