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: Make configure file parseable on python3 #9657

Closed
wants to merge 5 commits into from

Conversation

kalrover
Copy link
Contributor

@kalrover kalrover commented Nov 17, 2016

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

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

Fishrock123 commented Nov 17, 2016

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

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.

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

This approach LGTM

@@ -1,13 +1,17 @@
#!/usr/bin/env python

import sys
if sys.version_info >= (3, 0):
sys.stdout.write("Python 3.x is not compatible at this time\n")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@silverwind silverwind Nov 17, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

@jbergstroem jbergstroem Nov 17, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@gibfahn
Copy link
Member

gibfahn commented Nov 17, 2016

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

@Fishrock123
Copy link
Contributor

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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.

@@ -1142,7 +1147,7 @@ def configure_intl(o):
os.rename(tmp_icu, icu_full_path)
shutil.rmtree(icu_tmp_path)
else:
print ' Error: --with-icu-source=%s did not result in an "icu" dir.' % with_icu_source
print(' Error: --with-icu-source=%s did not result in an "icu" dir.' % with_icu_source)
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -1,13 +1,17 @@
#!/usr/bin/env python

import sys
if sys.version_info >= (3, 0):
sys.stdout.write("Python 3.x is not compatible at this time\n")
Copy link
Member

Choose a reason for hiding this comment

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

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

@jbergstroem
Copy link
Member

jbergstroem commented Nov 17, 2016

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

@@ -2,7 +2,7 @@

import sys
if sys.version_info >= (3, 0):
Copy link
Contributor Author

@kalrover kalrover Nov 18, 2016

Choose a reason for hiding this comment

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

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

@@ -488,7 +492,8 @@ def pkg_config(pkg):
shlex.split(pkg_config) + ['--silence-errors', flag, pkg],
stdout=subprocess.PIPE)
val = proc.communicate()[0].strip()
except OSError, e:
except OSError:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, use except OSError as e

Copy link
Contributor

Choose a reason for hiding this comment

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

That won't parse in Python 2.

Copy link
Contributor

@silverwind silverwind Nov 18, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -524,12 +529,12 @@ def get_version_helper(cc, regexp):
proc = subprocess.Popen(shlex.split(cc) + ['-v'], stdin=subprocess.PIPE,
stderr=subprocess.PIPE, stdout=subprocess.PIPE)
except OSError:
print '''Node.js configure error: No acceptable C compiler found!
print('''Node.js configure error: No acceptable C compiler found!
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

LGTM pending #9657 (comment).

@@ -949,7 +953,7 @@ def configure_static(o):

def write(filename, data):
filename = os.path.join(root_dir, filename)
print 'creating ', filename
print('creating ', filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be changed to

print('creating %s' % filename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you so much for reviewing :)

@@ -1108,17 +1112,17 @@ def configure_intl(o):
# --with-icu-source processing
# now, check that they didn't pass --with-icu-source=deps/icu
elif with_icu_source and os.path.abspath(icu_full_path) == os.path.abspath(with_icu_source):
print 'Ignoring redundant --with-icu-source=%s' % (with_icu_source)
print('Ignoring redundant --with-icu-source=%s' % (with_icu_source))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove redundant parenthesis

with_icu_source = None
# if with_icu_source is still set, try to use it.
if with_icu_source:
if os.path.isdir(icu_full_path):
print 'Deleting old ICU source: %s' % (icu_full_path)
print('Deleting old ICU source: %s' % (icu_full_path))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove redundant parenthesis

@@ -1151,22 +1156,22 @@ def configure_intl(o):
# ICU source dir relative to tools/icu (for .gyp file)
o['variables']['icu_path'] = icu_full_path
if not os.path.isdir(icu_full_path):
print '* ECMA-402 (Intl) support didn\'t find ICU in %s..' % (icu_full_path)
print('* ECMA-402 (Intl) support didn\'t find ICU in %s..' % (icu_full_path))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

sys.exit(1)
else:
print '* Using ICU in %s' % (icu_full_path)
print('* Using ICU in %s' % (icu_full_path))
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @kalrover :-)

@@ -1,13 +1,17 @@
#!/usr/bin/env python

import sys
if sys.version_info >= (3, 0):
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@silverwind
Copy link
Contributor

Thanks for persisting, landed in 330e63c!

@silverwind silverwind closed this Nov 30, 2016
silverwind pushed a commit that referenced this pull request Nov 30, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 5, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
thefourtheye pushed a commit to thefourtheye/io.js that referenced this pull request Dec 21, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: nodejs#9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Display python3-compatible error message for some systems use python3 as
default.

PR-URL: #9657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This was referenced Dec 21, 2016
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants