build: Make configure file parseable on python3 by kalrover · Pull Request #9657 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

Display python3-incompatible error message for some systems use python3 as
default.
Fix #9512

IIRC this doesn't help much since gyp still won't run on python3...?

This does seem to work with Python 2 (and give the right message with Python 3).

This approach LGTM

Maybe "Python 3 is not supported, please use Python 2.6 or 2.7"

I'd skip "at this time" -- there's been no progress in supporting Python 3.x and I don't think we should hint that it might change.

I'd also suggest the actual command to run, with a special case for macOS as it has no python2 symlink natively:

if sys.platform == 'darwin':
  print('Python 3 is not supported. Please use: python2.7 configure')
else:
  print('Python 3 is not supported. Please use: python2 configure')

@silverwind do we really need to branch for darwin? If you install python 3 on MacOS I'd also assume you know what you're doing.

@jbergstroem true, but I don't see a downside to not having. If we suggest the python2 command there's a chance it won't work, and we'd be better off not suggesting a command in that case.

I like being as specific as possible on the resolution as opposed to making people go and figure it out on their own.

@silverwind: I guess my sentiment was that that code path likely would be unreachable.

@Fishrock123 Error message used to look like:

  File "configure", line 491
    except OSError, e:
                  ^
SyntaxError: invalid syntax

and now it looks like:

Python 3.x is not compatible at this time

Seems like a decent improvement to me.

OHHHH yes that would probably be a good change to make.

LGTM although I'd change the error message to e.g. "Please use Python 2.7".

The CI (logically) doesn't test python3 so this feature is likely to regress over time.

Can you fix the long line while you're here?

@silverwind: I guess my sentiment was that that code path likely would be unreachable.

I guess it might be worth noting that we support Python 2.6 as well.

how about if sys.version_info[0] == 2 and sys.version_info[1] in [6, 7] ?

Nope, use except OSError as e

That won't parse in Python 2.

Ah, to be exact, the as keyword will parse in >= 2.6 which is the minimum we support, so it's fine I think.

It's available in 2.6 as well, https://docs.python.org/2.6/reference/compound_stmts.html#except

In addition to this, I would prefer from __future__ import print_function to be added as well.

@thefourtheye is that really required for the above type of usage in 2.6, 2.7? I guess its safer.

That would be helpful when we slowly start supporting python 3 as well. But that cannot happen anytime soon as other scripts also have to modified to support python 3. So for the time-being this is a take it or leave it suggestion.

I think from __future__ import print_function should be added after python3 supporting schedule happens.

@kalrover While I agree to not doing it now, the whole purpose of __future__ is to ease the migration process. Now that we are anyway changing all the print statements, I thought it would be better if we changed them to print function calls now itself.

LGTM pending #9657 (comment).

This has to be changed to

print('creating %s' % filename)

Thank you so much for reviewing :)

Remove redundant parenthesis

Same here

And here

LGTM. Thanks @kalrover :-)

Now that we are doing it, I like the version you proposed here. Can we include that as well?

if sys.version_info[0] != 2 or sys.version_info[1] not in (6, 7):
  sys.stdout.write("Please use either Python 2.6 or 2.7\n")
  sys.exit(1)

Seems reasonable, that way we also catch 2.5.x and older.

Display python3-compatible error message for some systems use python3 as default.

Thanks for persisting, landed in 330e63c!

Display python3-compatible error message for some systems use python3 as default. PR-URL: #9657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This does not land cleanly in v4.x LTS. Added dont-land label. Please feel free to manually backport

Display python3-compatible error message for some systems use python3 as default. PR-URL: #9657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Display python3-compatible error message for some systems use python3 as default. PR-URL: nodejs#9657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Display python3-compatible error message for some systems use python3 as default. PR-URL: #9657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 17, 2016

gibfahn suggested changes Nov 17, 2016

View reviewed changes

bnoordhuis approved these changes Nov 17, 2016

View reviewed changes

jbergstroem approved these changes Nov 17, 2016

View reviewed changes

kalrover commented Nov 18, 2016

View reviewed changes

thefourtheye suggested changes Nov 18, 2016

View reviewed changes

gibfahn approved these changes Nov 18, 2016

View reviewed changes

silverwind approved these changes Nov 18, 2016

View reviewed changes

kalrover force-pushed the fix/py3-parseable branch from 35fbe85 to ebe36f6 Compare November 21, 2016 02:31

thefourtheye suggested changes Nov 21, 2016

View reviewed changes

silverwind approved these changes Nov 21, 2016

View reviewed changes

thefourtheye approved these changes Nov 21, 2016

View reviewed changes

kalrover added 5 commits November 24, 2016 17:52

build: Make configure file parseable on python3

1fc8aae

Display python3-compatible error message for some systems use python3 as default.

Fix message & long line

6d786b5

Fix try statement

6f82f8b

Remove redundant parentheses

aff20c6

Check python 2.6 or 2.7

2303ff7

kalrover force-pushed the fix/py3-parseable branch from 1f0221c to 2303ff7 Compare November 24, 2016 08:53

silverwind approved these changes Nov 30, 2016

View reviewed changes

silverwind closed this Nov 30, 2016

silverwind mentioned this pull request Nov 30, 2016

configure fails on linux for distributions using python 3 as default #9512

Closed

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

MylesBorins added dont-land-on-v4.x labels Dec 20, 2016

thefourtheye mentioned this pull request Dec 21, 2016

v4.x: backport of "make configure file parseable on python3" #10373

Closed

2 tasks

This was referenced Dec 21, 2016

V6.9.3 proposal #10394

Merged

V4.7.1 proposal #10395

Merged

bnoordhuis mentioned this pull request Feb 7, 2017

Request: Make configure forwards compatible by putting parenties with prints #4462

Closed

build: Make configure file parseable on python3 by kalrover · Pull Request #9657 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: Make configure file parseable on python3 - #9657

build: Make configure file parseable on python3#9657kalrover wants to merge 5 commits intonodejs:masternodejs/node:masterfrom kalrover:fix/py3-parseablekalrover/node:fix/py3-parseableCopy head branch name to clipboard

Conversation

kalrover commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergstroem Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Nov 17, 2016

Uh oh!

Fishrock123 commented Nov 17, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalrover Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

build: Make configure file parseable on python3#9657
kalrover wants to merge 5 commits into
nodejs:masterfrom
kalrover:fix/py3-parseable

kalrover commented Nov 17, 2016 •
edited
Loading

Fishrock123 commented Nov 17, 2016 •
edited
Loading

silverwind Nov 17, 2016 •
edited
Loading

jbergstroem Nov 17, 2016 •
edited
Loading

jbergstroem commented Nov 17, 2016 •
edited
Loading

kalrover Nov 18, 2016 •
edited
Loading

silverwind Nov 18, 2016 •
edited
Loading