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.

Sign up for GitHub

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

Jump to bottom

build: try “python3” as a last resort for 3.x #35983

Closed
wants to merge 1 commit into from
Closed

build: try “python3” as a last resort for 3.x #35983

wants to merge 1 commit into from

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

@oleavr
build: try “python3” as a last resort for 3.x
49c83e4 
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
@cclauss
Copy link
Contributor

cclauss commented Nov 7, 2020 •
edited
Loading

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.

cclauss
cclauss requested changes Nov 7, 2020
View reviewed changes
Copy link
Contributor

@cclauss cclauss left a comment •
edited
Loading

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 •
edited
Loading

I think as a fallback this should be perfectly acceptable.

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

Trott
Trott reviewed Nov 14, 2020
View reviewed changes
configure
@@ -10,6 +10,7 @@ command -v python3.8 >/dev/null && exec python3.8 "$0" "$@"
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 •
edited
Loading

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

cclauss
cclauss approved these changes Nov 14, 2020
View reviewed changes
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

CI: https://ci.nodejs.org/job/node-test-pull-request/34430/

@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
@oleavr @nodejs-github-bot
build: try “python3” as a last resort for 3.x
fb24f6e 
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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
@oleavr @codebytere
build: try “python3” as a last resort for 3.x
60874ba 
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@oleavr @BethGriggs
build: try “python3” as a last resort for 3.x
5c5275a 
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@oleavr @BethGriggs
build: try “python3” as a last resort for 3.x
00c0f6d 
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@oleavr @BethGriggs
build: try “python3” as a last resort for 3.x
304e269 
So that Xcode's Python 3 gets picked up.

PR-URL: #35983
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@cclauss cclauss mentioned this pull request Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@cclauss cclauss cclauss approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oleavr @cclauss @addaleax @nodejs-github-bot @Trott @lpinca @gengjiawen @aduh95