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

build: add python3.10 alias to AC_PATH_PROGS call in configure #23182

Merged
merged 1 commit into from Oct 5, 2021

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Oct 5, 2021

Python 3.10 is now released, and has been available as a beta/rc in distros for a little while already.

Python 3.10 is now relased, and has been available as a beta/rc in
distros for a little while already.
@maflcko
Copy link
Member

maflcko commented Oct 5, 2021

Might as well add 3.11 to buy us 6 more months of having not to touch this again?

@maflcko
Copy link
Member

maflcko commented Oct 5, 2021

cr ACK ef15c57

@laanwj
Copy link
Member

laanwj commented Oct 5, 2021

Might as well add 3.11 to buy us 6 more months of having not to touch this again?

I sometimes wonder if we can do something smarter than hardcoding all the versions… (also why this order? why do we prefer lower versions)

That said, I'm good on kicking this particular can down the road one more time, doesn't seem something worth spending a lot of time on.
Code review ACK ef15c57

@maflcko
Copy link
Member

maflcko commented Oct 5, 2021

This patch will only makes a difference if python3 (or python) refer to python 3.5 or earlier and none of python3.6 - python3.9 are available, but python3.10 is. I don't think such a system exists in practice.

why this order? why do we prefer lower versions

My guess is that it makes it easier for devs to find incompatibilities with the minimum supported version.

@laanwj
Copy link
Member

laanwj commented Oct 5, 2021

My guess is that it makes it easier for devs to find incompatibilities with the minimum supported version.

Good point.

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 ef15c57, I have reviewed the code and it looks OK, I agree it can be merged.

UPDATE: #23182 (comment)

@maflcko maflcko merged commit 371f0ae into bitcoin:master Oct 5, 2021
@@ -107,7 +107,7 @@ AC_PATH_TOOL(GCOV, gcov)
AC_PATH_TOOL(LLVM_COV, llvm-cov)
AC_PATH_PROG(LCOV, lcov)
dnl Python 3.6 is specified in .python-version and should be used if available, see doc/dependencies.md
AC_PATH_PROGS([PYTHON], [python3.6 python3.7 python3.8 python3.9 python3 python])
AC_PATH_PROGS([PYTHON], [python3.6 python3.7 python3.8 python3.9, python3.10, python3 python])
Copy link
Member

@hebasto hebasto Oct 5, 2021

Choose a reason for hiding this comment

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

Commas actually break checking, and they must be dropped.

AC_PATH_PROGS won't find python3.9 and python3.10 executables.

Copy link
Member

Choose a reason for hiding this comment

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

@fanquake Are you testing the review process? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

👀. Will be addressed in a follow up shortly.

Copy link
Member

Choose a reason for hiding this comment

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

Included the fix here: #23270, lmk if you'd like to take a different approach.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 5, 2021
…l in configure

ef15c57 build: add python3.10 alias to AC_PATH_PROGS call in configure (fanquake)

Pull request description:

  Python 3.10 is [now released](https://pythoninsider.blogspot.com/2021/10/python-3100-is-available.html), and has been available as a beta/rc in distros for a little while already.

ACKs for top commit:
  MarcoFalke:
    cr ACK ef15c57
  laanwj:
    Code review ACK ef15c57
  hebasto:
    ACK ef15c57, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 4d503d43dfbe210ac6180c63276d4ebe1aa39b0ada2a36c1292634f86603c385179b3034759441401a9ee5022f7687ccd5f3432be5c4e63f2eba7ddb76f5cbb9
@fanquake fanquake deleted the python_3_10_configure branch October 8, 2021 00:25
@luke-jr
Copy link
Member

luke-jr commented Oct 11, 2021

why this order? why do we prefer lower versions

We can't possibly know in advance if newer versions will break our scripts?

fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 20, 2021
23182 was broken. Fix up the changes, and add python3.11 as suggested.
@fanquake
Copy link
Member Author

Ultimately fixed in #23317.

maflcko pushed a commit that referenced this pull request Oct 20, 2021
a78137e build: fix python detection post #23182 (fanquake)

Pull request description:

  #23182 was broken. Fixup the changes, and add python3.11 as suggested. Was going to include this in other changes, but no point leaving this broken any longer.

ACKs for top commit:
  MarcoFalke:
    cr ACK a78137e
  hebasto:
    ACK a78137e, tested on Ubuntu Impish 21.10 running `./configure`:

Tree-SHA512: f77cbea76710617eaea85787351a707cc2dcfb7e5962fc6d63ea11e737ee96cb2a496a2a4bb5a147b37ba87b0428977d9295ea053e25417ea13f43137c959919
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 20, 2021
23182 was broken. Fix up the changes, and add python3.11 as suggested.
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021
Python 3.10 is now relased, and has been available as a beta/rc in
distros for a little while already.

Github-Pull: bitcoin#23182
Rebased-From: ef15c57
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021
23182 was broken. Fix up the changes, and add python3.11 as suggested.

Github-Pull: bitcoin#23317
Rebased-From: a78137e
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants