8000 build: try “python3” as a last resort for 3.x by oleavr · Pull Request #35983 · nodejs/node · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@oleavr
Copy link
Contributor
@oleavr oleavr commented Nov 5, 2020

So that Xcode's Python 3 gets picked up.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

So that Xcode's Python 3 gets picked up.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 5, 2020
@gengjiawen gengjiawen requested a review from cclauss November 7, 2020 01:12
@cclauss
Copy link
Contributor
cclauss commented Nov 7, 2020

This could cause us to launch with a Python 3.{0,1,2,3,4} that is not tested and would probably fail. There are tons of macOS users that are using this code as is.

Please try python3 --version to see which version you have and then do python3.9 --version replacing the 9 with whatever the first command printed out. If this does not work out, please provide the output of these commands.

Copy link
Contributor
@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Could cause untested versions of Python 3 to run.

@addaleax
Copy link
Member
addaleax commented Nov 7, 2020

Could cause untested versions of Python 3 to run.

Why would that be a bad thing? I think as a fallback this should be perfectly acceptable.

@cclauss
Copy link
Contributor
cclauss commented Nov 7, 2020

I think as a fallback this should be perfectly acceptable.

Why take the support risk for a change that is not needed?

command -v python3.7 >/dev/null && exec python3.7 "$0" "$@"
command -v python3.6 >/dev/null && exec python3.6 "$0" "$@"
command -v python3.5 >/dev/null && exec python3.5 "$0" "$@"
command -v python3 >/dev/null && exec python3 "$0" "$@"
Copy link
Member

Choose a reason for hiding this comment

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

@cclauss Would this be acceptable?

Suggested change
command -v python3 >/dev/null && exec python3 "$0" "$@"
command -v python3 >/dev/null && python3 -c 'import sys; assert sys.version_info >= (3,5)' > /dev/null && exec python3 "$0" "$@"

Copy link
Member

Choose a reason for hiding this comment

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

Although I guess I kind of wonder if this actually covers anything we don't already cover.

@oleavr It would be very useful to know the output of python3 --version on your system (assuming this PR is a bug fix for your own setup).

Copy link
Member

Choose a reason for hiding this comment

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

We could also just print a warning from configure.py itself, right? We’re already doing that for a the compiler/assembler/etc. versions as well

Copy link
Contributor Author
@oleavr oleavr Nov 14, 2020

Choose a reason for hiding this comment

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

@Trott The output is:

% python3 --version
Python 3.8.2

This is the Xcode-provided Python 3 on a macOS system. Xcode doesn't create a python-3.8 symlink in /usr/bin, there's only /usr/bin/python3 (which is not a symlink, but a magic forwarder program that locates the Xcode installation and pivots to the appropriate binary there).

Copy link
Contributor
@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM... All my Macs use brew so I did not properly understand the issue. Thanks for clarifying.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 17, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2020
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2020
@github-actions
Copy link
Contributor

Landed in 0ed9961...fb24f6e

@github-actions github-actions bot closed this Nov 18, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 18, 2020
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@oleavr oleavr deleted the improve-python-detection branch November 18, 2020 19:16
codebytere pushed a commit that referenced this pull request Nov 22, 2020
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs t 53AD hat have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

0