Skip to content
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.

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

Bump minimum python version to 3.7 #26226

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 2, 2022

While there is nothing that requires a bump, it may require less maintenance to drop python3.6 support. Python3.7 is available through the package manager on all currently supported operating systems.

@kristapsk
Copy link
Contributor

Concept ACK, Python 3.6 had EOL 23 Dec 2021. https://endoflife.date/python

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 2, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto, jamesob
Concept ACK kristapsk, theStack, jarolrod, fanquake, fjahr
Stale ACK 1440000bytes, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)

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.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

doc/dependencies.md Outdated Show resolved Hide resolved
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK modulo #26226 (comment)

It would be useful to include a build system release note when dependencies like this are updated, rather than leaving it hidden among the change-log sea; but that's aside this pr.

@fanquake
Copy link
Member

fanquake commented Oct 3, 2022

Concept ACK for 25.x

@maflcko maflcko force-pushed the 2210-py37- branch 3 times, most recently from fa0a0bb to 19361d0 Compare October 3, 2022 10:08
@maflcko
Copy link
Member Author

maflcko commented Oct 3, 2022

Concept ACK, Python 3.6 had EOL 23 Dec 2021. https://endoflife.date/python

EOL of python shouldn't affect us, as we only use it to run the tests. Though, operating systems that ship with it going EOL would affect us.

@fjahr
Copy link
Contributor

fjahr commented Oct 3, 2022

I don't get the argument to do this now when nothing has changed since #24017...

@Sjors
Copy link
Member

Sjors commented Oct 4, 2022

No strong opinions. Paging @luke-jr.

@maflcko maflcko marked this pull request as draft October 4, 2022 09:53
@maflcko
Copy link
Member Author

maflcko commented Oct 4, 2022

I don't get the argument to do this now when nothing has changed since #24017...

Right, there is no rush, so I guess I better close this.

It just has to be done at some point in the future and I thought having a working patch couldn't hurt.

@luke-jr
Copy link
Member

luke-jr commented Oct 4, 2022

No strong opinions. Paging @luke-jr.

Thanks. I did take a look at this the other day. My notes are that 3.6 was for RHEL, but it appears RHEL has since moved on.

Personally, 3.6 is slightly more convenient to me since I have no easy way to get 3.7 anymore for testing (nor 3.6, but it's already installed) - Gentoo's oldest is 3.8. But I can adapt if need be.

Copy link
Member

@jamesob jamesob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@@ -12,7 +12,7 @@

def git_grep(params: [], error_msg: ""):
try:
output = subprocess.check_output(["git", "grep", *params], universal_newlines=True, encoding="utf8")
output = subprocess.check_output(["git", "grep", *params], text=True, encoding="utf8")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think encoding is now unnecessary to specify in this line (and many others below) because text ensures stdout is a string and strings in Python 3 are by default unicode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, no. This only changed in Python 3.7, see https://docs.python.org/3/library/os.html#utf8-mode

I guess text/universal_newlines is not needed when specifying encoding, so might as well remove them completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't they be needed? Windows still has non-standard newlines.

@maflcko
Copy link
Member Author

maflcko commented Oct 5, 2022

Closing for now until there is a more pressing reason

@maflcko maflcko closed this Oct 5, 2022
@maflcko maflcko deleted the 2210-py37-🔐 branch October 5, 2022 13:22
@jamesob
Copy link
Member

jamesob commented Jan 17, 2023

I hereby volunteer to provide the code necessary to generate a container sufficient to run the functional tests for anyone both (i) yelling and (ii) running an ancient version of Python. :)

@maflcko
Copy link
Member Author

maflcko commented Jan 17, 2023

Right, Ubuntu Bionic will be EOL before the next release anyway. Centos Stream 8 runs until May 31st, 2024 , but they can install python38, I guess 🤷‍♂️

@maflcko
Copy link
Member Author

maflcko commented Jan 17, 2023

(Force pushed minimum to 3.7.16 and "maximum" to 3.12)

@maflcko maflcko added this to the 25.0 milestone Jan 17, 2023
@achow101
Copy link
Member

Since #26716 changes our CI to use pyenv's python, I think the changes to the container and installed packages in fa33242 end up being irrelevant?

@maflcko
Copy link
Member Author

maflcko commented Jan 17, 2023

The ci/lint system is separate from the ci/test system, so you should be able to get a test failure by reverting the image or packages in ci/test

@achow101
Copy link
Member

ACK fa1dbe0

ci/test/04_install.sh Outdated Show resolved Hide resolved
MarcoFalke added 3 commits January 18, 2023 12:59
This reverts commit be59bd1
because the changes are no longer needed.
-BEGIN VERIFY SCRIPT-
 sed -i 's/universal_newlines/text/g' $(git grep -l universal_newlines)
-END VERIFY SCRIPT-
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa8fe5b

@jamesob
Copy link
Member

jamesob commented Jan 18, 2023

ACK fa8fe5b

Looks ready to go to me!

@jamesob
Copy link
Member

jamesob commented Jan 18, 2023

As a follow-up I'm betting we can drop all the changes that use pyenv to build Python from source, since Python 3.7 comes with buster by default?

@maflcko
Copy link
Member Author

maflcko commented Jan 18, 2023

No, we also need git2.34 or later, which is only shipped in Ubuntu Jammy

@maflcko maflcko merged commit aef8b4f into bitcoin:master Jan 18, 2023
@maflcko maflcko deleted the 2210-py37-🔐 branch January 18, 2023 15:48
fanquake added a commit to fanquake/core-review that referenced this pull request Jan 18, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 19, 2023
@willcl-ark
Copy link
Member

Post merge ACK.

FWIW my version of pyenv did not contain 3.7.16 in the list of versions available to install. Easily fixed though by running pyenv update to update both pyenv itself, and also the list of available versions for install.

@RandyMcMillan
Copy link
Contributor

#27130 (comment)

To maintain consistency across macOS x86 and Arm64 maybe the minimum python3 version should be python@3.8

Screen Shot 2023-02-20 at 1 00 41 PM

ref: #26226

kwvg added a commit to kwvg/dash that referenced this pull request May 10, 2023
kwvg added a commit to kwvg/dash that referenced this pull request May 10, 2023
PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request May 11, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.