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

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

Merged
merged 3 commits into from
Jul 7, 2016

Conversation

rouge8
Copy link
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.

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.
@@ -444,7 +444,7 @@ def execute_pkg_resources(spec):
else:
# setuptools < 11.3
runner = entry.load(require=False)
runner()
sys.exit(runner())
Copy link
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
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
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
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
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
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