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: better support for python3 systems #14737

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Aug 10, 2017

Improve support for systems where python is actually python3.

Not all systems have a python2 binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

This would need to be applied to other python scripts as well but I figured
I'd start with a single script in case everyone hates the approach.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 10, 2017
@vsemozhetbyt vsemozhetbyt added the python PRs and issues that require attention from people who are familiar with Python. label Aug 10, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I’m okay with this.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Couple of suggestions

Also branch name should have been I-shpy-with-my-little-eye-something-beginning-with-python2

@@ -1,4 +1,14 @@
#!/usr/bin/env python
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

This will stop syntax highlighting from working. Not the end of the world I guess.

configure Outdated
which python2 >/dev/null && exec python2 "$0" "$@"
exec python "$0" "$@"
''' "$0" "$@"
]

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a del _ or _ = None so that the _ variable doesn't propagate through the script. I doubt it'll conflict with anything (or cause a performance hit), but it would at least clarify that the python _ variable creation is a byproduct of the shell script.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that if you insist but my thinking was that this snippet will be copy/pasted across a number of files and should therefore be as succinct as possible.

Copy link
Member

Choose a reason for hiding this comment

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

If you're copy pasting an extra line doesn't matter right?

I find it clearer, so yes please.

# Locate python2 interpreter and re-execute the script. Note that the
# mix of single and double quotes is intentional, as is the fact that
# the ] goes on a new line.
_=[ 'exec' '/bin/sh' '-c' '''
Copy link
Member

@gibfahn gibfahn Aug 12, 2017

Choose a reason for hiding this comment

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

Is changing $_ definitely okay in all shells? I know it's normally not user-modifiable, but IDK about csh and other arcane shells. I checked zsh, bash, and dash.

Copy link
Member Author

Choose a reason for hiding this comment

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

/bin/sh is always a Bourne-compatible shell, so it should be okay.

# the ] goes on a new line.
_=[ 'exec' '/bin/sh' '-c' '''
which python2.7 >/dev/null && exec python2.7 "$0" "$@"
which python2 >/dev/null && exec python2 "$0" "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check for python2.6 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I deliberately didn't add python2.6 for two reasons:

  1. I wasn't sure if it should go before or after python2, and
  2. We supported python2.6 because of centos 5 but since we no longer support that platform...

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I guess dropping python2.6 should be a separate PR.

@gibfahn
Copy link
Member

gibfahn commented Aug 12, 2017

Ben this is witchcraft, did you just come up with this? I can't get over how many python specific things it relies on.

@bnoordhuis
Copy link
Member Author

I got the idea from a perl script. :-) It's a lot easier in perl though because just about any shell syntax is also valid perl.

Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.
@bnoordhuis
Copy link
Member Author

Added a del statement. New CI for good measure: https://ci.nodejs.org/job/node-test-pull-request/9706/

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

jasnell pushed a commit that referenced this pull request Aug 23, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: #14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

Landed in c6da5c8

@jasnell jasnell closed this Aug 23, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: nodejs/node#14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: nodejs/node#14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: #14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: #14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
Improve support for systems where `python` is actually `python3`.

Not all systems have a `python2` binary, so simply updating the shebang
won't work.

What we can do is apply some cleverness: start life as a shell script,
locate the python binary, then re-execute the script but this time as
python code.

Special care is taken to ensure that spaces in arguments are passed on
verbatim.

PR-URL: #14737
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed on v6.x. Please let me know if it should be backed out

exec python "$0" "$@"
''' "$0" "$@"
]
del _

Copy link

@c9n c9n Jul 19, 2020

Choose a reason for hiding this comment

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

@bnoordhuis I'm not very clear how it works. Could you explain a bit more about this block for me? Thanks.

_=[ 'exec' '/bin/sh' '-c' '''
which python2.7 >/dev/null && exec python2.7 "$0" "$@"
which python2 >/dev/null && exec python2 "$0" "$@"
exec python "$0" "$@"
''' "$0" "$@"
]
del _

Copy link
Member Author

Choose a reason for hiding this comment

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

@c9n The _=[ ... ] is first evaluated as shell script that re-executes the whole script as python code when it finds the appropriate interpreter. As python code, it's effectively a no-op: it assigns to _ an array of strings that isn't otherwise used.

Copy link

Choose a reason for hiding this comment

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

@bnoordhuis Got it! Thank you so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants