Makefile: Add lint-python which uses flake8 #21952

cclauss · 2018-07-24T06:24:15Z

Add a lint-python section to Makefile which pip installs flake8 and then executes:

flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics

Based on the experimentation done at https://travis-ci.com/nodejs/node/builds/79706150 - #21942

These tests currently exclude several directories and files:

./deps/npm/node_modules/node-gyp -- this repo currently has no automated testing
./deps/v8 -- contains several Python 3 only features such as async def
./src/noperfctr_macros.py -- contains "macros" which Python AST views as syntax errors
./src/notrace_macros.py -- contains "macros" which Python AST views as syntax errors
./tools -- contains "macros" which Python AST views as syntax errors

Hopefully this list can be reduced in future PRs with code modifications or the use of # noqa

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

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

mscdex · 2018-07-24T06:38:57Z

Is it not possible to bundle the linting library instead, like we do for eslint? That way we don't need to make system-wide changes (e.g. implicitly upgrading pip)?

cclauss · 2018-07-24T07:22:47Z

We could remove pip install —upgrade pip.

cclauss · 2018-07-24T08:32:39Z

@mscdex Does that last commit cover what you wanted or did I misunderstand your request?

Is the automated testing actually doing a "make lint"? If so, how do I view the output?

https://github.com/nodejs/node/runs/8932739 does not seem to contain the words "lint-python" or "flake8".

targos · 2018-07-24T11:49:32Z

Makefile

+# Flag the build if there are Python syntax errors or undefined names
+lint-python: lint-python
+	@echo "Running Python linter on $(shell $(PYTHON) --version)..."
+	@$(PYTHON) -m pip install --upgrade flake8


This doesn't work on my system when not run as root:

OSError: [Errno 13] Permission denied: '/usr/lib/python2.7/site-packages/enum34-1.1.6.dist-info' make[1]: *** [Makefile:1212: lint-python] Error 2

Can you please try to add --user to the command and let me know if that works?

👌 It works with --user

targos · 2018-07-24T12:04:02Z

Exclusions don't seem to work. I see many errors coming from deps/v8 in the output

cclauss · 2018-07-24T12:27:11Z

You see these errors where? Locally or in server-based build log?

targos · 2018-07-24T12:31:26Z

Locally, with make lint

refack · 2018-07-24T12:53:11Z

/cc @nodejs/python @nodejs/build-files

refack

Should split build code to a lint-python-build target, and add conditions (maype if $(shell python -m flake8 --version)
remove @ for traceability
IMHO setting should go into tox.ini and not stored in the Makefile

refack

see comments

refack · 2018-07-24T13:11:54Z

Hello @cclauss and thank you for the contribution 🥇
I made a few comments about the current implementation, but a part from that I'm not so sure this should run as part of the lint target (probably just the lint-ci target). We have a goal of optimizing the build time, for developers running locally, and separately on the CI cluster. The churn of the python files in the project is very low, so IMHO we could skip running this for every change...
Also we should simply exclude the whole of the /deps/ dir since those files are vendored in from 3rd party repos (almost everything) same for /tools/gyp/, as well as the /src/ and /lib/ dirs since they should not contain any python files.

cclauss · 2018-07-24T18:01:31Z

I implemented all suggestions except:

the if statement to jump from lint-python to lint-python-build (I could not get it right)
tox.ini because I am not a fan of another level of indirection and it robs the power to create a second pass thru flake8 with no --select but with --exit-zero on to find PEP8 violations.
Slight changes on the --excludes and a lint-python-all-files that has no --exclude

richardlau · 2018-07-24T18:05:56Z

Makefile

+.PHONY: lint-python
+# Lints the Python code with flake8.
+# Flag the build if there are Python syntax errors or undefined names
+lint-python: lint-python


lint-python: lint-python-build

?

mscdex · 2018-07-24T20:06:23Z

Does that last commit cover what you wanted or did I misunderstand your request?

What I meant was include the 'flake8' module in the repo, like we do with eslint for js files, to avoid having to install it system/user-wide (which I would find to be unexpected behavior).

jasnell · 2018-09-10T18:25:00Z

What's the status on this one?

cclauss · 2018-10-21T08:45:29Z

@mscdex it is expected behavior for Python package. "Vendoring in" is quite rare in the Python world.

@refack Could you please advise here. It has been three months...

cclauss · 2018-10-22T10:26:00Z

That is lots of approval... https://blog.github.com/2018-10-21-october21-incident-report

refack · 2018-10-22T13:20:18Z

@nodejs/build-files @nodejs/python PTAL
CI: https://ci.nodejs.org/job/node-test-pull-request/18041/

PR-URL: nodejs#21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2018-10-24T21:21:34Z

Congratulations 🎉
Landed in 0c39290
@cclauss, thank you very much for the contribution.

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 24, 2018

cclauss mentioned this pull request Jul 24, 2018

Travis CI: Add flake8 jobs for Python 2.7 and Python 3.7 #21953

Closed

4 tasks

targos reviewed Jul 24, 2018

View reviewed changes

refack approved these changes Jul 24, 2018

View reviewed changes

refack suggested changes Jul 24, 2018

View reviewed changes

refack added tools Issues and PRs related to the tools directory. python PRs and issues that require attention from people who are familiar with Python. labels Jul 24, 2018

refack self-assigned this Jul 24, 2018

richardlau reviewed Jul 24, 2018

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

refack force-pushed the add-flake8-to-testing branch from a4f0ae4 to 4458162 Compare October 21, 2018 23:48

refack approved these changes Oct 21, 2018

View reviewed changes

refack approved these changes Oct 22, 2018

View reviewed changes

refack removed the stalled Issues and PRs that are stalled. label Oct 22, 2018

refack force-pushed the add-flake8-to-testing branch from 4458162 to a1b6029 Compare October 24, 2018 20:39

refack force-pushed the add-flake8-to-testing branch from a1b6029 to 604890b Compare October 24, 2018 20:58

refack force-pushed the add-flake8-to-testing branch from 604890b to 9a690c3 Compare October 24, 2018 21:06

build: add lint-py which uses flake8

0c39290

PR-URL: nodejs#21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

refack force-pushed the add-flake8-to-testing branch from 9a690c3 to 0c39290 Compare October 24, 2018 21:19

refack merged commit 0c39290 into nodejs:master Oct 24, 2018

cclauss deleted the add-flake8-to-testing branch October 24, 2018 21:31

refack removed their assignment Oct 24, 2018

targos pushed a commit that referenced this pull request Oct 26, 2018

build: add lint-py which uses flake8

eb35d0d

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

targos pushed a commit that referenced this pull request Nov 1, 2018

build: add lint-py which uses flake8

b8f3bb1

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 26, 2018

build: add lint-py which uses flake8

aed45d7

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added dont-land-on-v8.x labels Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

rvagg pushed a commit that referenced this pull request Nov 28, 2018

build: add lint-py which uses flake8

77756dc

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 29, 2018

build: add lint-py which uses flake8

bf61fe5

PR-URL: #21952 Reviewed-By: Refael Ackermann <refack@gmail.com>

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile: Add lint-python which uses flake8 #21952

Makefile: Add lint-python which uses flake8 #21952

cclauss commented Jul 24, 2018 •

edited

Loading

mscdex commented Jul 24, 2018

cclauss commented Jul 24, 2018 •

edited

Loading

cclauss commented Jul 24, 2018 •

edited

Loading

targos Jul 24, 2018

cclauss Jul 24, 2018

targos Jul 24, 2018

targos commented Jul 24, 2018

cclauss commented Jul 24, 2018

targos commented Jul 24, 2018

refack commented Jul 24, 2018

refack left a comment

refack left a comment

refack commented Jul 24, 2018 •

edited

Loading

cclauss commented Jul 24, 2018 •

edited

Loading

richardlau Jul 24, 2018

mscdex commented Jul 24, 2018

jasnell commented Sep 10, 2018

cclauss commented Oct 21, 2018

cclauss commented Oct 22, 2018

refack commented Oct 22, 2018 •

edited

Loading

refack commented Oct 24, 2018

Makefile: Add lint-python which uses flake8 #21952

Makefile: Add lint-python which uses flake8 #21952

Conversation

cclauss commented Jul 24, 2018 • edited Loading

Checklist

mscdex commented Jul 24, 2018

cclauss commented Jul 24, 2018 • edited Loading

cclauss commented Jul 24, 2018 • edited Loading

targos Jul 24, 2018

Choose a reason for hiding this comment

cclauss Jul 24, 2018

Choose a reason for hiding this comment

targos Jul 24, 2018

Choose a reason for hiding this comment

targos commented Jul 24, 2018

cclauss commented Jul 24, 2018

targos commented Jul 24, 2018

refack commented Jul 24, 2018

refack left a comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 24, 2018 • edited Loading

cclauss commented Jul 24, 2018 • edited Loading

richardlau Jul 24, 2018

Choose a reason for hiding this comment

mscdex commented Jul 24, 2018

jasnell commented Sep 10, 2018

cclauss commented Oct 21, 2018

cclauss commented Oct 22, 2018

refack commented Oct 22, 2018 • edited Loading

refack commented Oct 24, 2018

cclauss commented Jul 24, 2018 •

edited

Loading

cclauss commented Jul 24, 2018 •

edited

Loading

cclauss commented Jul 24, 2018 •

edited

Loading

refack commented Jul 24, 2018 •

edited

Loading

cclauss commented Jul 24, 2018 •

edited

Loading

refack commented Oct 22, 2018 •

edited

Loading