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

Improve error message for isort failure in IsortRun #7462

Merged
merged 6 commits into from Mar 30, 2019

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 30, 2019

Problem

When ./pants fmt.isort fails, its failure message is difficult to parse, especially because it starts with the command, rather than human readable English introducing what happened. For example:

FAILURE: /Users/eric/.pyenv/shims/python3.6 /Users/eric/.cache/pants/python/CPython-3.6.8/d6bcba436bdefbc27a69dd880d6ee5716e004764/isort-c201e856817dc9ae802d18e3ec45a3bfa4c2ac11.pex --check-only src/python/pants/util/tarutil.py src/python/pants/util/retry.py src/python/pants/util/osutil.py src/python/pants/util/memo.py src/python/pants/util/process_handler.py src/python/pants/util/strutil.py src/python/pants/util/py2_compat.py src/python/pants/util/meta.py src/python/pants/util/xml_parser.py src/python/pants/util/objects.py src/python/pants/util/rwbuf.py src/python/pants/util/collections_abc_backport.py ... exited non-zero (1).

FAILURE

Solution

Invert the error message to be more readable.

Result

For example:

FAILURE: Exited with return code 1 while running `/Users/eric/.pyenv/shims/python3.6 /Users/eric/.cache/pants/python/CPython-3.6.8/d6bcba436bdefbc27a69dd880d6ee5716e004764/isort-c201e856817dc9ae802d18e3ec45a3bfa4c2ac11.pex --check-only src/python/pants/util/tarutil.py src/python/pants/util/retry.py src/python/pants/util/osutil.py src/python/pants/util/memo.py src/python/pants/util/process_handler.py src/python/pants/util/strutil.py src/python/pants/util/py2_compat.py src/python/pants/util/meta.py src/python/pants/util/xml_parser.py src/python/pants/util/objects.py src/python/pants/util/rwbuf.py src/python/pants/util/collections_abc_backport.py`.

FAILURE

Eric-Arellano added some commits Mar 30, 2019

Better output in TaskError
The isort command is extremely verbose and not very relevant. What really matters are the args used.
@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

This shouldn't be a console task as things stand. I'm almost positive folks install it as a dependency of / directly in the compile goal for example. Any task not being run directly like that should never be a console task. I know this is vague but hopefully makes sense. If it helps, in v2 the console rule would be fmt, the implementations like isort would be plain rules returning result objects encapsulating the report of what they did. The fmt root console rule would print all the aggregate results it received from isort, gofmt, etc.

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Here's a rule for you, if the task is installed in a non-singleton goal in v1 it can't be a console task.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Bummer, but good to know.

Is there any way to not swallow stdout and stderr when using ./pants --quiet in this case? That's really my main motivation. I'd love for build-support/bin/isort.sh to give better output. In the setup repo, I wrote a Python 3 script to do this, and Danny encouraged me to upstream those improvements to Pants itself.

Even if no, hope we can keep this change to make the TaskError a bit more readable: https://github.com/pantsbuild/pants/pull/7462/files#diff-71900004479b8668c33e31f7159d67ecR67.

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

I'm pretty sure there is no canned way. The TaskError improvement stands on its own however.

@Eric-Arellano Eric-Arellano changed the title WIP: Convert IsortRun into a ConsoleTask for more focused output to user Improve error message when IsortRun fails Mar 30, 2019

@Eric-Arellano Eric-Arellano changed the title Improve error message when IsortRun fails Improve error message for isort failure in IsortRun Mar 30, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Mar 30, 2019

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Oops. I disagree on the error message. You can actually directly run the prior message. You can't with yours.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Bummer. Once we close out the blacklist and get Py3 on every CI shard, I'll copy my port of isort.sh to Python 3 over to here, which parses the output so that it's more readable for us.

--

Updated error message to keep the whole command. Think this is still an improvement, albeit much smaller scope than originally intended.

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

To be clear, the existing message is structured:
<path to python actually used> <path to _loose_ isort PEX dir (not a directly executable PEX zip)> <args>

@Eric-Arellano Eric-Arellano merged commit baaaffd into pantsbuild:master Mar 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:better-isort branch Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.