pantsbuild / binaries

A temporary home for pants static binaries and scripts
16 stars 37 forks source link

Upgrade isort to 4.3.4. #76

Closed jsirois closed 6 years ago

jsirois commented 6 years ago

This adds support for more python3 stdlib symbols.

Also introduce a build script.

I found these instructions in the pants repo late in the game: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/tasks/python_isort.py#L25-L29

In practice they do not look necessary. I was able to do the following to get my local pants running on isort 4.3.4:

binaries $ ./build_isort.sh 4.3.4
binaries $ mkdir -p ~/.cache/pants/bin/isort/4.3.4
binaries $ cp build-support/scripts/isort/4.3.4/isort.pex ~/.cache/pants/bin/isort/4.3.4/isort.pex
binaries $ cd ~/dev/pantsbuild/pants
pants $ vi src/python/pants/base/parse_context.py
pants $ git diff
diff --git a/src/python/pants/base/parse_context.py b/src/python/pants/base/parse_context.py
index 278467514..4af400ee1 100644
--- a/src/python/pants/base/parse_context.py
+++ b/src/python/pants/base/parse_context.py
@@ -5,6 +5,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals

 import functools
+
 import threading
 from builtins import object

pants $ PANTS_ISORT_VERSION=4.3.4 ./build-support/bin/isort.sh -f
Fixing /home/jsirois/dev/pantsbuild/jsirois-pants/src/python/pants/base/parse_context.py

pants $
jsirois commented 6 years ago

N.B. This is a bit of a straw man. As noted, I found the instructions in the pants repo late after I had started down the path of making a binary like any other over here. I'm not convinced either approach is better, but perhaps the 'TODO: Explain why we don't invoke isort directly.' comment in the IsortPythonTask class docs can be explained out loud.

wisechengyi commented 6 years ago

Hey @jsirois sorry I am responsible for TODO: Explain why we don't invoke isort directly. In the end the task was made up in hackweek :P

isort for python2 has import side effect breaking pants' run_tracker's hook https://github.com/timothycrosley/isort/issues/456

but it might work with python3 as noted in the ticket.

jsirois commented 6 years ago

Aha, thanks @wisechengyi I read invoke directly as build a pex directly which is what this PR does. What then is this for?:

wisechengyi commented 6 years ago

isort.py is the same as the script used to invoke isort after a pip install isort.

$ cat /opt/example/bin/isort
#!/opt/example/opt/python/bin/python2.7

# -*- coding: utf-8 -*-
import re
import sys

from isort.main import main

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

build a pex directly which is what this PR does

Yeah what you did should work too, which doesn't necessarily have to follow ^

Please let me know if anything else is unclear.

stuhood commented 6 years ago

(looks like you've got reviews for the rest, so will ignore)

But as to building a pex for isort: one thing that we could certainly do is have pants build an isort pex dynamically, the same way we do for pytest. I've suggested this to Chris as a possible path forward for python linting in general.

jsirois commented 6 years ago

we could certainly do is have pants build an isort pex dynamically,

Absolutely - this was a confusing adventure and it would be great to bootstrap tools for languages pants understands via pants. I'll take a look at that.