Skip to content

Don't ignore exit codes when using setuptools entry points#280

Merged
kwlzn merged 3 commits into
pex-tool:masterfrom
rouge8:gh-137-exit-codes
Jul 7, 2016
Merged

Don't ignore exit codes when using setuptools entry points#280
kwlzn merged 3 commits into
pex-tool:masterfrom
rouge8:gh-137-exit-codes

Conversation

@rouge8
Copy link
Copy Markdown
Contributor

@rouge8 rouge8 commented Jul 6, 2016

Setuptools does not require console_scripts entry points to call sys.exit() (or similar) to exit non-zero:

The functions you specify are called with no arguments, and their return value is passed to sys.exit(), so you can return an errorlevel or message to print to stderr

This patch updates pex to follow the same behavior.

Fixes #137.

rouge8 added 2 commits July 5, 2016 20:33
Setuptools does not require `console_scripts` entry points to call
`sys.exit()` (or similar) to exit non-zero:

    The functions you specify are called with no arguments, and their
    return value is passed to `sys.exit()`, so you can return an
    errorlevel or message to print to stderr

This patch updates pex to follow the same behavior.

Fixes pex-tool#137.
Comment thread pex/pex.py Outdated
# setuptools < 11.3
runner = entry.load(require=False)
runner()
sys.exit(runner())
Copy link
Copy Markdown
Contributor

@kwlzn kwlzn Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an awkward place to sys.exit - esp as potentially called by a library consumer. is there no better path to plumbing the return value/execution context and sys.exit'ing at the end of PEX.execute()?

at a minimum, moving this to either PEX.execute_script() or its calling branch in PEX._execute() would make slightly more sense to me given the context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but most packages will already have done a sys.exit() or raise SystemExit here anyways, including pex. I can still move it if you'd like

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit moving it so this should be ready to merge either way

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is the generic case of non-console script entry point execution to consider, esp as used as a library - moving it to execute_script() works for me tho.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, definitely did not consider that

@kwlzn kwlzn merged commit 40ebd65 into pex-tool:master Jul 7, 2016
@kwlzn
Copy link
Copy Markdown
Contributor

kwlzn commented Jul 7, 2016

thanks for the PR @rouge8 !

@kwlzn kwlzn mentioned this pull request Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants