Dogfood `./pants fmt.isort`.

Review Request #4289 — Created Oct. 8, 2016 and submitted

jsirois
pants
jsirois/isort/dogfood
3748, 3940
3daac82...
pants-reviews
benjyw, wisechengyi
This switches from using isort in a venv to the isort formatting task
pants ships with. It also switches to a `./pants changed` pipeline to
speed up the common small change case.

 build-support/bin/isort.sh | 62 +-------------------------------------------------
 1 file changed, 1 insertion(+), 61 deletions(-)

I used this sequence to test things out in both standalone and git
hook modes (command prompts beginning with ^ indicate a non-zero exit
code in the command above):

(master *+) $ git diff
diff --git a/src/python/pants/base/build_environment.py b/src/python/pants/base/build_environment.py
index e3d0c4e..62c64ca 100644
--- a/src/python/pants/base/build_environment.py
+++ b/src/python/pants/base/build_environment.py
@@ -7,12 +7,12 @@ from __future__ import (absolute_import, division, generators, nested_scopes, pr

 import logging
 import os
-import sys

 from pants.base.build_root import BuildRoot
 from pants.scm.scm import Scm
 from pants.version import VERSION as _VERSION

+import sys

 logger = logging.getLogger(__name__)

(master *+) $ ./build-support/bin/isort.sh 

15:21:12 00:00 [main]
               See a report at: http://localhost:46177/run/pants_run_2016_10_07_15_21_12_821_da2e56b439c54d428dcd9b7825f1713e
15:21:12 00:00   [setup]
15:21:12 00:00     [parse]
               Executing tasks in goals: fmt
15:21:12 00:00   [fmt]
15:21:13 00:01     [isort]ERROR: /home/jsirois/dev/pantsbuild/pants/src/python/pants/base/build_environment.py Imports are incorrectly sorted.

FAILURE: /home/jsirois/.cache/pants/scripts/isort/4.2.5/isort.pex --check-only src/python/pants/util/dirutil.py src/python/pants/version.py src/python/pants/scm/scm.py src/python/pants/util/memo.py src/python/pants/util/strutil.py src/python/pants/base/build_root.py src/python/pants/util/contextutil.py src/python/pants/util/meta.py src/python/pants/base/build_environment.py src/python/pants/scm/git.py src/python/pants/util/tarutil.py ... exited non-zero (1).


15:21:14 00:02   [complete]
               FAILURE
^(master *+) $ git commit -am 'Should fail sort check.'
Checking packages
Checking imports

15:21:36 00:00 [main]
...
               FAILURE

To fix import sort order, run `build-support/bin/isort.sh -f`
^(master *+) $ build-support/bin/isort.sh -f

15:21:54 00:00 [main]
...
               SUCCESS
(master +) $ git commit -m 'Dogfood pants fmt.isort in our scripts.'
Checking packages
Checking imports
Checking headers
Success
[master f09eae6] Dogfood pants fmt.isort in our scripts.
 1 file changed, 36 insertions(+), 96 deletions(-)
 rewrite build-support/bin/isort.sh (71%)

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/165941654

  1. Thanks for doing this! I assumed there was a reason we hadn't so far. But =you that not dogfooding our own stuff is not good.

  2. build-support/bin/isort.sh (Diff revision 1)
     
     

    Nice. In the future it would be good if fmt.isort relied directly on regular pants invalidation for this, but that's for another day.

    1. Agreed.
    2. There was nothing tracking this so filed https://github.com/pantsbuild/pants/issues/3942
  3. 
      
  1. Yi - I'm going to wait on your eyeballs before submitting this.  You ran into issues in the past and I want to make sure this addresses those.
  2. 
      
  1. Sorry about the delay.

    I was going to suggest ./pants fmt.isort $(./pants changed --sep=' ') -- ..., but given fmt.isort will do :: on empty args, that's probably not a good idea. We may want to change its behavior in the future like list.

    [tw-mbp-yic pants (fid)]$ ./pants list
    WARN] The behavior of `./pants list` (no explicit targets) will soon become a no-op. To remove this warning, please specify one or more explicit target specs (e.g. `./pants list ::`).
    ...
    
    1. The xargs -I conveniently short-circuits when there is no input to substitute, so right now the RHS of the pipeline only fires up pants if there are 1 or more changed targets. I think this satisfies what you were looking for.

    2. Yup that's good!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 61ed3d0d5254ed7b138f9b3b31b7c4e7f93396e5
Author: John Sirois <john.sirois@gmail.com>
Date:   Mon Oct 10 12:02:52 2016 -0600

    Dogfood `./pants fmt.isort`.
    
    This switches from using isort in a venv to the isort formatting task
    pants ships with. It also switches to a `./pants changed` pipeline to
    speed up the common small change case.
    
    Testing Done:
    I used this sequence to test things out in both standalone and git
    hook modes (command prompts beginning with `^` indicate a non-zero exit
    code in the command above):
    ```
    (master *+) $ git diff
    diff --git a/src/python/pants/base/build_environment.py b/src/python/pants/base/build_environment.py
    index e3d0c4e..62c64ca 100644
    --- a/src/python/pants/base/build_environment.py
    +++ b/src/python/pants/base/build_environment.py
    @@ -7,12 +7,12 @@ from __future__ import (absolute_import, division, generators, nested_scopes, pr
    
     import logging
     import os
    -import sys
    
     from pants.base.build_root import BuildRoot
     from pants.scm.scm import Scm
     from pants.version import VERSION as _VERSION
    
    +import sys
    
     logger = logging.getLogger(__name__)
    
    (master *+) $ ./build-support/bin/isort.sh
    
    15:21:12 00:00 [main]
                   See a report at: http://localhost:46177/run/pants_run_2016_10_07_15_21_12_821_da2e56b439c54d428dcd9b7825f1713e
    15:21:12 00:00   [setup]
    15:21:12 00:00     [parse]
                   Executing tasks in goals: fmt
    15:21:12 00:00   [fmt]
    15:21:13 00:01     [isort]ERROR: /home/jsirois/dev/pantsbuild/pants/src/python/pants/base/build_environment.py Imports are incorrectly sorted.
    
    FAILURE: /home/jsirois/.cache/pants/scripts/isort/4.2.5/isort.pex --check-only src/python/pants/util/dirutil.py src/python/pants/version.py src/python/pants/scm/scm.py src/python/pants/util/memo.py src/python/pants/util/strutil.py src/python/pants/base/build_root.py src/python/pants/util/contextutil.py src/python/pants/util/meta.py src/python/pants/base/build_environment.py src/python/pants/scm/git.py src/python/pants/util/tarutil.py ... exited non-zero (1).
    
    15:21:14 00:02   [complete]
                   FAILURE
    ^(master *+) $ git commit -am 'Should fail sort check.'
    Checking packages
    Checking imports
    
    15:21:36 00:00 [main]
    ...
                   FAILURE
    
    To fix import sort order, run `build-support/bin/isort.sh -f`
    ^(master *+) $ build-support/bin/isort.sh -f
    
    15:21:54 00:00 [main]
    ...
                   SUCCESS
    (master +) $ git commit -m 'Dogfood pants fmt.isort in our scripts.'
    Checking packages
    Checking imports
    Checking headers
    Success
    [master f09eae6] Dogfood pants fmt.isort in our scripts.
     1 file changed, 36 insertions(+), 96 deletions(-)
     rewrite build-support/bin/isort.sh (71%)
    ```
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/165941654
    
    Bugs closed: 3748, 3940
    
    Reviewed at https://rbcommons.com/s/twitter/r/4289/
Loading...