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

Use Python3 compatible syntax. #126

Closed
wants to merge 2 commits into from
Closed

Use Python3 compatible syntax. #126

wants to merge 2 commits into from

Conversation

alex
Copy link
Contributor

@alex alex commented Feb 7, 2019

This code continues to run correctly under Python2.

Changes include:

  • Modernized octal literals
  • Print as a function
  • 'as' syntax for handling exceptions
  • Remove of 'L' long suffixes

@oliverchang
Copy link
Collaborator

thanks! that was quick :) can you please run python butler.py lint and python butler.py format ?

@inferno-chromium
Copy link
Collaborator

We will add this in the documentation on how to do linting and formatting.

@alex
Copy link
Contributor Author

alex commented Feb 7, 2019

Looking into getting that set up -- is there a chance of a CI environment that will run/verify these? The setup scripts for butler.py require some pretty significant mutation of the system (extra PPAs/apt repos, new packages, it attempts to upgrade virtualenv in a way that's not safe for my system, etc.), so it'll take me a bit of time before I think I can properly run this locally.

@oliverchang
Copy link
Collaborator

Not yet -- we have a CI environment but right now it's lacking lint capability. I just filed #127 and hope to get it added very soon.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM assuming that the removal of the print statement in src/python/tests/appengine/libs/handler_test.py is acceptable.

@cclauss cclauss mentioned this pull request Feb 8, 2019
@inferno-chromium
Copy link
Collaborator

/gcbrun

@inferno-chromium
Copy link
Collaborator

fails in integration tests, can only be triggered by us until CI is up hopefully next week. but usually as soon as you upload, we will see it and add a trigger. here is the failure.

Step #1: ======================================================================
Step #1: ERROR: test_libfuzzerize_corpus (tests.core.bot.fuzzers.afl.afl_launcher_integration_test.TestLauncherMinijail)
Step #1: ----------------------------------------------------------------------
Step #1: Traceback (most recent call last):
Step #1: File "/workspace/src/python/tests/core/bot/fuzzers/afl/afl_launcher_integration_test.py", line 79, in call_f
Step #1: result = f(self, *args, **kwargs)
Step #1: File "/usr/local/lib/python2.7/dist-packages/mock/mock.py", line 1305, in patched
Step #1: return func(*args, **keywargs)
Step #1: File "/workspace/src/python/tests/core/bot/fuzzers/afl/afl_launcher_integration_test.py", line 460, in test_libfuzzerize_corpus
Step #1: self._test_libfuzzerize_corpus(mock_get_timeout)
Step #1: File "/workspace/src/python/tests/core/bot/fuzzers/afl/afl_launcher_integration_test.py", line 209, in _test_libfuzzerize_corpus
Step #1: run_launcher(testcase_path, 'test_fuzzer', input_corpus)
Step #1: File "/workspace/src/python/tests/core/bot/fuzzers/afl/afl_launcher_integration_test.py", line 113, in run_launcher
Step #1: launcher.main(['launcher.py'] + list(args))
Step #1: File "/workspace/src/python/bot/fuzzers/afl/launcher.py", line 1403, in main
Step #1: print('Command: {0}\n'
Step #1: AttributeError: 'NoneType' object has no attribute 'format'
Step #1:

@alex
Copy link
Contributor Author

alex commented Feb 9, 2019

That's amazing. I've done a lot of these, and I've never seen that particular error.

@cclauss
Copy link
Contributor

cclauss commented Feb 9, 2019

@alex What tool did you use? Futurize, Modernize, etc.?

@inferno-chromium
Copy link
Collaborator

/gcbrun

1 similar comment
@oliverchang
Copy link
Collaborator

/gcbrun

@oliverchang
Copy link
Collaborator

Could you please merge the latest changes from master into this PR? We added lint support in our CI but this PR is still based on an older revision which didn't have that :)

This code continues to run correctly under Python2.

Changes include:
- Modernized octal literals
- Print as a function
- 'as' syntax for handling exceptions
- Remove of 'L' long suffixes
@oliverchang
Copy link
Collaborator

/gcbrun

@evverx
Copy link

evverx commented Feb 12, 2019

This PR looks like a good place to discuss Python :-)

I've just added ClusterFuzz to LGTM (https://lgtm.com/projects/g/google/clusterfuzz/alerts/?mode=tree) and it currently thinks that ClusterFuzz is a Python3 project. Would it be OK if I added a file named .lgtm.yml to the root directory here to help it to figure out that actually ClusterFuzz is a Python2 project?

By the way, it's also possible to enable automatic code review if that sounds interesting.

@inferno-chromium
Copy link
Collaborator

@evverx - is LGTM.com any good for python code ? what sort of issues does it find ? i thought it would make sense for more native c/c++ code.

@cclauss
Copy link
Contributor

cclauss commented Feb 12, 2019

@evverx For legacy Python, the .lgtm.yml file should contain:

extraction:
  python:
    python_setup:
      version: 2

google clusterfuzz languages

@evverx
Copy link

evverx commented Feb 12, 2019

@inferno-chromium all the rules can be found at https://lgtm.com/search?q=language%3Apython&t=rules, but as far as I can tell, out of the box it can't find anything that can't be found with other tools and if that's already covered by the CI, then automatic code review will be redundant. I think adding .lgtm.yml would be useful anyway because it will help external contributors stumbling upon the project on LGTM to focus on what is important :-)

@evverx
Copy link

evverx commented Feb 12, 2019

@cclauss I have been bothering the team behind LGTM for about a year :-) and among other things they told me how to enforce python2. But thank you!

@inferno-chromium
Copy link
Collaborator

@evverx - sure go ahead and submit a pr for python 2.

@jonathanmetzman
Copy link
Collaborator

@evverx Actually, Abhishek and I spoke. No need to do a PR (sorry!). We'll think about this and add this file if we decide we want to use LGTM.

Thanks for the help!

@evverx
Copy link

evverx commented Feb 12, 2019

@jonathanmetzman the project is already there and, hopefully, will be analyzed continuously anyway. In the long term it's probably better to stick to python3 there. Plus after this PR is merged and half the alerts are gone, the graphs will look really cool :-)

@oliverchang
Copy link
Collaborator

@alex did you manage to get python butler.py lint and python butler.py format running? There were some errors from the last run:

Step #1 - "Lint": Running: pylint src/python/bot/fuzzers/afl/launcher.py
Step #1 - "Lint": | Using config file /workspace/.pylintrc
Step #1 - "Lint": | ************* Module workspace.src.python.bot.fuzzers.afl.launcher
Step #1 - "Lint": | C:1406, 0: Wrong hanging indentation (add 1 space).
Step #1 - "Lint": | engine_common.get_command_quoted(command), BOT_NAME,
Step #1 - "Lint": | ^| (bad-continuation)
Step #1 - "Lint": | C:1407, 0: Wrong hanging indentation (add 1 space).
Step #1 - "Lint": | fuzz_result.time_executed))
Step #1 - "Lint": | ^| (bad-continuation)
Step #1 - "Lint": | 

Also, there is now a conflict so you'll need to resolve them :/

@cclauss
Copy link
Contributor

cclauss commented Feb 20, 2019

@alex Could you please add the two spaces to placate the linter?

@inferno-chromium
Copy link
Collaborator

This is done in 6738dd6 closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants