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

Some fixes for Python 3 #23669

Closed
wants to merge 5 commits into from
Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Oct 15, 2018

Python 3 compatibility fixes as discussed in #23659 (comment)

These changes get the test scores equivalent on the following two tests.

  • python2 -m flake8 . --exclude=./deps/v8,./deps/npm/node_modules/node-gyp --count --show-source --statistics --select=E901,E999,F821,F822,F823
  • python3 -m flake8 . --exclude=./deps/v8,./deps/npm/node_modules/node-gyp --count --show-source --statistics --select=E901,E999,F821,F822,F823

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

  • F821: undefined name name
  • F822: undefined name name in __all__
  • F823: local variable name referenced before assignment
  • E901: SyntaxError or IndentationError
  • E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

@refack @eirnym

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory. labels Oct 15, 2018
@refack refack self-assigned this Oct 15, 2018
@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Oct 16, 2018
@Trott
Copy link
Member

Trott commented Oct 16, 2018

The change to the one file in deps will need to be upstreamed to OpenSSL rather than changed in our source code directly.

@cclauss
Copy link
Contributor Author

cclauss commented Oct 16, 2018

Done in openssl/openssl#7409

@silverwind
Copy link
Contributor

Generally LGTM, but isn't pylint the go-to python linter nowadays? If so I'd suggest not introducing pyflakes specific comments.

@refack
Copy link
Contributor

refack commented Oct 16, 2018

Generally LGTM, but isn't pylint the go-to python linter nowadays? If so I'd suggest not introducing pyflakes specific comments.

The popular linter these days is flake8 🤷‍♂️
For the time being I'm coordinating the GYP changes in nodejs/node-gyp, then I'm planning on running GYP's original test suite to get some coverage. So actually it might be better to remove the GYP changes from this PR.

@refack refack added the wip Issues and PRs that are still a work in progress. label Oct 16, 2018
@cclauss
Copy link
Contributor Author

cclauss commented Oct 17, 2018

@silverwind PyLint in its default configuration does not do very will on the following...

import sys
abc = collections.namedtuple("abc", "a b c")  # Missing import
hi = ur"hello"                     # Illegal in Python 3
for i in xrange(2):                # Illegal in Python 3
    long_i = long(i)               # Illegal in Python 3
    print i                        # Illegal in Python 3
    if isinstance(i, basestring):  # Illegal in Python 3
        print(unicode(i))          # Illegal in Python 3
reload(sys)                        # Illegal in Python 3
sys.setdefaultencoding("utf-8")    # Illegal in Python 3

async def async_print(s):          # Illegal in Python 2
    print(s, end=" ")              # Illegal in Python 2

@@ -45,7 +46,7 @@ def main():

cmd = ([os.path.abspath(os.path.join(THIS_DIR, FUZZER))] + sys.argv[2:]
+ ["-artifact_prefix=" + corpora[1] + "/"] + corpora)
print " ".join(cmd)
print(" ".join(cmd))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged in upstream in openssl/openssl#7409 but retained here because openssl in not pip installed but is vendored in.

@silverwind
Copy link
Contributor

silverwind commented Oct 17, 2018

PyLint in its default configuration does not do very will on the following

Yes, IIRC, pylint needs quite a bit of configuration to be useful. Anyways, I'd appreciate if you'd remove linter-specific comments, we can decide on a python linter later on, this PR should be about fixing compatibilty only, the lint result doesn't need to be 100% clean.

@@ -10,6 +10,8 @@
:copyright: Copyright 2013 by the Jinja team, see AUTHORS.
:license: BSD, see LICENSE for details.
"""
# flake8: noqa

Copy link
Contributor

Choose a reason for hiding this comment

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

suggesting to drop these based on my earlier comment.

Copy link
Contributor

@refack refack Oct 23, 2018

Choose a reason for hiding this comment

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

@refack
Copy link
Contributor

refack commented Nov 19, 2018

@cclauss do you mind if I close this issue as superseded?

@refack refack added superseded and removed build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. labels Nov 19, 2018
@refack refack removed tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress. labels Nov 19, 2018
@cclauss cclauss closed this Nov 19, 2018
@cclauss cclauss deleted the some-Python3-fixes branch November 19, 2018 21:27
@refack refack removed their assignment Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

6 participants