There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
Fix all maintenance issues by bumping the minimum.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fanquake, RandyMcMillan, fjahr |
| Concept ACK | stickies-v, jamesob |
| Stale ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
9@@ -10,7 +10,7 @@ You can find installation instructions in the `build-*.md` file for your platfor
10 | [Automake](https://www.gnu.org/software/automake/) | [1.13](https://github.com/bitcoin/bitcoin/pull/18290) |
11 | [Clang](https://clang.llvm.org) | [8.0](https://github.com/bitcoin/bitcoin/pull/24164) |
12 | [GCC](https://gcc.gnu.org) | [8.1](https://github.com/bitcoin/bitcoin/pull/23060) |
13-| [Python](https://www.python.org) (tests) | [3.7](https://github.com/bitcoin/bitcoin/pull/26226) |
14+| [Python](https://www.python.org) (scripts, tests) | [3.8](https://github.com/bitcoin/bitcoin/pull/xyz) |
0| [Python](https://www.python.org) (scripts, tests) | [3.8](https://github.com/bitcoin/bitcoin/pull/27483) |
Concept ACK
I’m not sure why we’re switching to focal instead of bullseye for the qt5 ci, but I don’t really have a view either way. Just pointing it out.
I’m not sure why we’re switching to focal instead of bullseye
The reason is that bullseye does not have a single of the needed packages:
(Also, bullseye will be EOL a year earlier than focal)
Unrelated: Looks like there are a few bugs with GCC libstdc++-9:
./util/bitdeque.h:240:5: error: exception specification of explicitly defaulted move constructor does not match the calculated one (https://cirrus-ci.com/task/6721414045106176?logs=ci#L3390)So I worked around them.
Also, switch ci_native_qt5 to g++-9 (from g++-8) to work around bugs.
This should be fine, because the i686_centos task still checks for g++-8
compatibility.
See
https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1513477050
for the list of bugs.
python:3.8-buster?
Should also bump the lint Dockerfile to python:3.8-buster?
buster is EOL and unmaintained, which is one of the reasons I created this pull, as can be seen in the pull request description. I don’t really understand why the lint Dockerfile isn’t simply using the exact same distro and setup like the lint CI. Using something else is just going to make it less reproducible and harder to maintain. Though, those changes should probably be made in a separate follow-up pull.
This bump should not be needed, see discussion starting at
https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517739626
buster is EOL and unmaintained,
In this context, I don’t think that makes any difference, as the container is just an interpreter, so as long as it’s downloadable, it should work (no apt installing packages etc).
I don’t really understand why the lint Dockerfile isn’t simply using the exact same distro and setup like the lint CI.
No idea. Maybe ask @jamesob.
I was mostly pointing it out because it seemed odd to not just bump it as well. Not a blocker if someone else is going to followup.
it should work (no apt installing packages etc).
It calls RUN /install.sh, which installs packages
It calls RUN /install.sh, which installs packages
Right. Although it doesn’t actually need to do that, as all the packages we need are already available in the base python3.8-buster image.
ACK fac395e
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
This is ok to merge but when I wrote that comment I meant to remove the modinv() function completely, e.g. https://github.com/fjahr/bitcoin/commit/75b8ba524d3dc957910bc8c0a4d1dd2b8ceaa426. You can pull this commit in here or we merge this now and I will open it as a follow-up. I think the comment in modinv() isn’t accurate anymore after this change because python seems to use Exponentiation by Squaring and we don’t really need a one-line function that and we also don’t need to test native python code.
I think it is best for you to open a follow-up,
Lets do that.
Milestone
26.0