Can 'make lint-py PYTHON=python3' be a manditory Jenkins test?

[cclauss]

Is your feature request related to a problem? Please describe.
Python 2 end-of-life is one year from now. We have been working for a while and this repo's code now contains no Python 3 syntax errors or undefined names. make lint-py shows that we no longer have any syntax errors on either Python 2 or Python 3. It also shows that we have fixed all undefined names like basestring, cmp(), file, reduce(), raw_input(), unicode, xrange() that were removed in Python 3.

The port to Python 3 is not yet complete but the codebase is "syntax compatible" which is a state that we should now preserve and guard against any backsliding.

Describe the solution you'd like
After #24954 lands, it would be helpful if every Jenkins run required that make lint-py PYTHON=python2 and make lint-py PYTHON=python3 both pass. This will ensure that pull requests do not introduce syntax errors (like print without the parens) or undefined names (like raw_input()). This will help as we complete the port.

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.
Going snowboarding.

/cc @nodejs/python

[targos]

/cc @nodejs/build

[Trott]

I guess the big change this would require would be Python 3 to be installed on all our CI hosts? Or is it possible to lint for Python 3 issues using Python 2? We do already run make lint-py on all CI test runs, but I guess that only covers Python 2 issues in the current state?

[targos]

I think it would be enough to test only on one host, with a custom job. And when the build supports Python 3, switch the job to use Python 3 fully.

[BridgeAR]

I believe it would be best to move this issue to the build repo. Could someone do that (I do not have the necessary rights)? :)

[targos]

Issue moved to nodejs/build

[cclauss]

This has to be run on Python 3 because flake8 uses CPython to build an Abstract Syntax Tree and it then lints the AST. This is described in the Very Important Note on the top of http://flake8.pycqa.org

That being said, Python 3 was introduced 8+ years ago so today it is quite easy on all OSes to install both legacy Python and Python side-by-side on the same box and then do python2 -m flake8 [...] and python3 -m flake8 [...].

[Trott]

I think it would be enough to test only on one host, with a custom job. And when the build supports Python 3, switch the job to use Python 3 fully.

Oh, yeah, maybe we can just install Python 3 on the linter host(s) only and have it run there via a Jenkins config.

[cclauss]

This effort can proceed when consensus is reached because nodejs/node#24954 has landed and make lint-py PYTHON=python2 and make lint-py PYTHON=python3 both pass.

[Trott]

Adding to the Build WG meeting agenda.

[rvagg]

If it's just for linting then it should be straight forward I think since it's limited to a few hosts.

But ...

Python 2 end-of-life is one year from now

Yikes .. I didn't know this but it has huge ramifications across the board for us. Some things off the top of my head:

• node-gyp and GYP in general .. I've mostly tuned out of the many attempts to make our GYP Python 3 compatible because it's such a mess (and mostly I'm just cranky at Google for being so terrible at tools—with a combination of pathological NIMBY and "bored of this, let's invent something new"). If Python 2 is genuinely something we shouldn't be relying on in a year, then we need to have a plan to migrate the entire ecosystem to a Python 3 version of GYP. That's going to take some care and skillful planning I think. Maybe it's time to consider a more radical move for addon building? Promote CMake? Revisit Fedor's gyp.js? Something else?
• Our custom Ansible stuff is only partly Python 3 compatible, mostly it works but there are special use cases where it fails still. There's a couple of PRs and issues open on this repo about this. On my macOS machine I get Python 3 with Ansible thanks to brew, but on my Linux machine I get Python 2 with Ansible, because <reasons>.

Python 2 is still everywhere. If 2 is EOL so soon, why do we still have this kind of thing:

$ lsb_release -d
Description:	Ubuntu 18.04.1 LTS
$ head -1 $(which ansible)
#! /usr/bin/python2

Have distros thrown up their hands too? I believe it's possible to install Python 3 on older hosts we maintain, like CentOS 6. Some of them are a bit awkward because a lot of internal tooling on Linux distros use Python so conflicts are really easy.

This feels to me like a big enough problem that it needs needs something at a higher level, a strategic initiative or short-lived working group to sort out all of the issues. Not that I really want to be thinking more than I have to about this mess but the pain is only going to increase. No, I'm not volunteering to lead such an initiative, our strategy of 🙈 🙉 🙊 isn't sustainable though.

[cclauss]

https://python3statement.org

https://fedoraproject.org/wiki/FinalizingFedoraSwitchtoPython3

• This is not just for linting. We will need to run parallel tests for many months. Putting both Python 2 and Python 3 on a single machine is quite easy these days (even on CentOS 6) so my recommendation would be that we put both on lots of build machines, not just on a few.
• Check out the current version of your favorite linux distro as it probably has Python 3 installed by default and not Python 2
  • https://lwn.net/Articles/756628
  • https://travis-ci.org/Mariatta/python_versions_and_distros/builds/469363099#L215
• https://github.com/ansible/ansible is already fully supports Python 3
• https://github.com/chromium/gyp is already syntax compatible with Python 3
• https://github.com/nodejs/node-gyp needs work to become syntax compatible with Python 3
• https://github.com/nodejs/node is syntax compatible with Python 3 but its dependencies need work

Being syntax compatible != fully supporting.

[JamesMGreene]

• https://github.com/chromium/gyp is already syntax compatible with Python 3

Note that this is only as of ~1 week ago and it is still largely untested.

[cclauss]

Installing Python 3 on FreeBSD: pkg install python3

[rvagg]

https://ci.nodejs.org/job/node-test-linter/24474/console failing with python3

10:43:05 python3 -m pip install --upgrade -t tools/pip/site-packages flake8 || \
10:43:05 	python3 -m pip install --upgrade --system -t tools/pip/site-packages flake8
10:43:05 /usr/local/bin/python3: No module named pip

...

10:44:24 Running C++ linter...
10:44:25 Traceback (most recent call last):
10:44:25   File "tools/cpplint.py", line 6526, in <module>
10:44:25     main()
10:44:25   File "tools/cpplint.py", line 6510, in main
10:44:25     ProcessFile(filename.decode('utf-8'), _cpplint_state.verbose_level)
10:44:25 AttributeError: 'str' object has no attribute 'decode'
10:44:25 gmake: *** [Makefile:1262: tools/.cpplintstamp] Error 1

on master, but even if we fix on master we need a strategy for older branches - do we backport? or do we hack CI to switch python version for older branches?

[rvagg]

added py36-pip to the install list which cleaned up the first one, second one remains:

10:57:11 Running C++ linter...
10:57:12 Traceback (most recent call last):
10:57:12   File "tools/cpplint.py", line 6526, in <module>
10:57:12     main()
10:57:12   File "tools/cpplint.py", line 6510, in main
10:57:12     ProcessFile(filename.decode('utf-8'), _cpplint_state.verbose_level)
10:57:12 AttributeError: 'str' object has no attribute 'decode'
10:57:12 gmake: *** [Makefile:1262: tools/.cpplintstamp] Error 1

[cclauss]

Running an outa date version of cpplint.py

[rvagg]

so we need to fix that on master then?

[cclauss]

Why are we bothering with C++?

We merely want the flake8 jobs to run on Py2 and Py3.

This issue is about lint-py, not about all lint jobs in Makefile.

[rvagg]

because they're all tangled together:

which gmake &>/dev/null && MAKE=gmake || MAKE=make
PYTHON=python2

$MAKE lint-md-build PYTHON=$PYTHON || true
$MAKE lint-py-build PYTHON=$PYTHON || true
# If lint-ci fails, print all the interesting lines to the console.
# FreeBSD sed can't handle \s, so use gsed if it exists.
which gsed &>/dev/null && SED=gsed || SED=sed
$MAKE lint-ci PYTHON=$PYTHON || { 
  cat test-eslint.tap | grep -v '^ok\|^TAP version 13\|^1\.\.' | $SED '/^\s*$/d' && 
  exit 1; }

We run lint-ci which is:

lint-ci: lint-js-ci lint-cpp lint-py lint-md lint-addon-docs

So this doesn't sound like it's a nodejs/build problem until nodejs/node has Makefile, or cpplint.py, or whatever else sorted out. Then that's going to need to be backported to the supported release lines or we're going to need to add complex version switching logic in our linting job in CI.

[cclauss]

Let's make two variables $PYTHON2 and $PYTHON3 like in nodejs/node#25118

[cclauss]

cpplint @ https://github.com/google/styleguide and https://github.com/cpplint/cpplint are both compatible with Py2 and Py3

[cclauss]

For now, can we merely add the command make lint-py PYTHON=python3 ?

[refack]

I'm removing from agenda since the specific issue has a clear path to resolution.
Bigger issue addressed in nodejs/TSC#642.

[thefourtheye]

As per: #1644 (comment) job can't go live until lint issue is solved in node core (Ref: nodejs/node#25614)

@refack Now that we do not lint python scripts in test/fixtures (ref: nodejs/node#25639), can we push the job to live?

[mhdawson]

@refack do you already have the job ready to go and we just need to add it to the regressions runs or is there more to do beyond that assuming that we are ready to go given nodejs/node#25639

[refack]

Ready to go. I just wanted the patched code in master to get to as many people as possible.
(Still PR that were not rebased will fail linting, so I was hoping to minimize noise)

[mhdawson]

Ok so you are planning to turn it on in a few days or a week at most?

[refack]

Ok so you are planning to turn it on in a few days or a week at most?

At most. I'll send a PSA now.

[refack]

Activated for node-test-linter (part of node-test-pull-request) -> https://ci.nodejs.org/job/node-test-linter/25180/console

Testing job for node-linter (part of node-test-pull-request-lite-pipeline) -> https://ci.nodejs.org/job/refack-node-linter/2/console

[cclauss]

Thanks massively... This will help us avoid any backsliding.

[Trott]

Closable now? Or is there still more to be done?

[refack]

Closable now? Or is there still more to be done?

The pipeline variant hasn't gone live yet...

[cclauss]

Can we please get this resolves and closed? 303 daze left...

[Trott]

(Adding build-agenda so it gets discussed at the next Build meeting if there isn't any significant progress between now and then.)

[refack]

IMO fixing the pipeline job has low ROI. It's mostly a convenience. The main job (https://ci.nodejs.org/job/node-test-linter/) runs python linting with Python3, so we have regression testing.

If someone wants to follow up with porting https://github.com/nodejs/build/blob/master/jenkins/pipelines/node-linter.jenkinsfile, that will be very helpful.

[cclauss]

@rvagg @thefourtheye How do we progress this?