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

tools: from io import StringIO in ninja.py #29371

Closed
wants to merge 1 commit into from

Conversation

@cclauss
Copy link
Contributor

commented Aug 29, 2019

cStringIO was removed in Python 3 so this PR advocates that we use the io module instead. As reported #25789 (comment), #29196 (comment), and #29236 (comment) this issue is being seen by Windows users who are experimenting with Python 3.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@cclauss cclauss requested a review from joaocgreis Aug 29, 2019

@cclauss cclauss added the python label Aug 29, 2019

@cclauss cclauss requested review from sam-github and gengjiawen Aug 29, 2019

@cclauss cclauss changed the title Py3 ninja.py tools: from io import StringIO in ninja.py Aug 29, 2019

@nodejs-github-bot

This comment has been minimized.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Aside: if you push fix-ups, it's easier for people to recognize them as such when you prefix them with fixup! or squash!. They're not arbitrary prefixes, git rebase -i recognizes them.

Someone might come along now and be unsure whether the two commits in this PR need to be squashed first or merged separately.

@cclauss cclauss force-pushed the cclauss:Py3-ninja.py branch from 331b268 to 64fa594 Aug 30, 2019

@nodejs-github-bot

This comment has been minimized.

@gengjiawen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

This happened on macOS platform too.There are other issues on macOS too. We may need to test more platform on JenkinsCI or travis.

@richardlau

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

This happened on macOS platform too.There are other issues on macOS too. We may need to test more platform on JenkinsCI or travis.

This file is only used when using Ninja, which I don't think any of our CI (Travis or Jenkins) uses.

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@gengjiawen If you are interested to spend some time on adding macOS and Windows to .travis.yml, you might take some inspiration from: https://docs.travis-ci.com/user/languages/python/#running-python-tests-on-multiple-operating-systems

@nodejs-github-bot

This comment has been minimized.

@gengjiawen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

This happened on macOS platform too.There are other issues on macOS too. We may need to test more platform on JenkinsCI or travis.

This file is only used when using Ninja, which I don't think any of our CI (Travis or Jenkins) uses.

I mean we need to test Python3 compatibility in other Platform like macOS.

Also the crash happened in config phase, python3 ./configure.py

Traceback (most recent call last):
  File "./configure.py", line 1716, in <module>
    run_gyp(gyp_args)
  File "tools/gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 523, in gyp_main
    options.duplicate_basename_check)
  File "tools/gyp/pylib/gyp/__init__.py", line 107, in Load
    generator.CalculateVariables(default_variables, params)
  File "tools/gyp/pylib/gyp/generator/make.py", line 81, in CalculateVariables
    import gyp.generator.xcode as xcode_generator
  File "tools/gyp/pylib/gyp/generator/xcode.py", line 10, in <module>
    import gyp.xcode_ninja
  File "tools/gyp/pylib/gyp/xcode_ninja.py", line 16, in <module>
    import gyp.generator.ninja
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 23, in <module>
    from cStringIO import StringIO
ModuleNotFoundError: No module named 'cStringIO'
@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

That is the line that this PR fixes!!

@gengjiawen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

That is the line that this PR fixes!!

yeap, but on macOS will still failed with this patch, maybe add macOS to travisCI:

Traceback (most recent call last):
  File "./configure.py", line 1716, in <module>
    run_gyp(gyp_args)
  File "tools/gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 523, in gyp_main
    options.duplicate_basename_check)
  File "tools/gyp/pylib/gyp/__init__.py", line 139, in Load
    params['parallel'], params['root_targets'])
  File "tools/gyp/pylib/gyp/input.py", line 2779, in Load
    variables, includes, depth, check, True)
  File "tools/gyp/pylib/gyp/input.py", line 459, in LoadTargetBuildFile
    includes, depth, check, load_dependencies)
  File "tools/gyp/pylib/gyp/input.py", line 408, in LoadTargetBuildFile
    build_file_data, PHASE_EARLY, variables, build_file_path)
  File "tools/gyp/pylib/gyp/input.py", line 1290, in ProcessVariablesAndConditionsInDict
    build_file)
  File "tools/gyp/pylib/gyp/input.py", line 1305, in ProcessVariablesAndConditionsInList
    ProcessVariablesAndConditionsInDict(item, phase, variables, build_file)
  File "tools/gyp/pylib/gyp/input.py", line 1264, in ProcessVariablesAndConditionsInDict
    ProcessConditionsInDict(the_dict, phase, variables, build_file)
  File "tools/gyp/pylib/gyp/input.py", line 1137, in ProcessConditionsInDict
    build_file)
  File "tools/gyp/pylib/gyp/input.py", line 1061, in EvalCondition
    cond_expr, true_dict, false_dict, phase, variables, build_file)
  File "tools/gyp/pylib/gyp/input.py", line 1087, in EvalSingleCondition
    if eval(ast_code, {'__builtins__': None}, variables):
  File "<string>", line 1, in <module>
TypeError: '>=' not supported between instances of 'int' and 'str' while loading dependencies of /Users/daniel/code/node/node.gyp while trying to load /Users/daniel/code/node/node.gyp
@lpinca
lpinca approved these changes Aug 30, 2019

@targos targos added the author ready label Aug 31, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Landed in 020c2ea

@Trott Trott closed this Sep 3, 2019

Trott added a commit that referenced this pull request Sep 3, 2019
tools: use 'from io import StringIO' in ninja.py
PR-URL: #29371
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This change broke ./configure --ninja:

$ ./configure --ninja 
INFO: Using floating patch "tools/icu/patches/64/source/common/putil.cpp" from "tools/icu"
INFO: Using floating patch "tools/icu/patches/64/source/i18n/dtptngen.cpp" from "tools/icu"
Traceback (most recent call last):
  File "./configure", line 28, in <module>
    import configure
  File "/home/mzasso/git/nodejs/node/configure.py", line 1716, in <module>
    run_gyp(gyp_args)
  File "tools/gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "tools/gyp/pylib/gyp/__init__.py", line 532, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 2490, in GenerateOutput
    config_name)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 2395, in GenerateOutputForConfig
    target = writer.WriteSpec(spec, config_name, generator_flags)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 483, in WriteSpec
    spec)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 921, in WriteSources
    precompiled_header, spec)
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 980, in WriteSourcesForArch
    [Define(d, self.flavor) for d in defines])
  File "tools/gyp/pylib/gyp/generator/ninja.py", line 1585, in WriteVariableList
    ninja_file.variable(var, ' '.join(values))
  File "tools/gyp/pylib/gyp/ninja_syntax.py", line 35, in variable
    self._line('%s = %s' % (key, value), indent)
  File "tools/gyp/pylib/gyp/ninja_syntax.py", line 139, in _line
    self.output.write(leading_space + text[0:space] + ' $\n')
TypeError: unicode argument expected, got 'str'
sam-github added a commit that referenced this pull request Sep 3, 2019
gyp: make StringIO work in ninja.py
Allow both Python 2 and 3 to access StringIO.

This fixes `./configure --ninja`, which was broken by
#29371.

See: #29371 (comment)

PR-URL: #29414
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@cclauss cclauss deleted the cclauss:Py3-ninja.py branch Sep 3, 2019

BridgeAR added a commit that referenced this pull request Sep 3, 2019
tools: use 'from io import StringIO' in ninja.py
PR-URL: #29371
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
tools: use 'from io import StringIO' in ninja.py
PR-URL: #29371
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
tools: use 'from io import StringIO' in ninja.py
PR-URL: nodejs#29371
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
gyp: make StringIO work in ninja.py
Allow both Python 2 and 3 to access StringIO.

This fixes `./configure --ninja`, which was broken by
nodejs#29371.

See: nodejs#29371 (comment)

PR-URL: nodejs#29414
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
tools: use 'from io import StringIO' in ninja.py
PR-URL: nodejs#29371
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
gyp: make StringIO work in ninja.py
Allow both Python 2 and 3 to access StringIO.

This fixes `./configure --ninja`, which was broken by
nodejs#29371.

See: nodejs#29371 (comment)

PR-URL: nodejs#29414
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.