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

Pex exits with correct code when using entrypoint #605

Merged

Conversation

illicitonion
Copy link
Contributor

Currently the exit code from an entrypoint is swallowed, so running pex
always exits 0.

Tested with:

tox -e py27-package
dist/pex27 -e pytest:main pytest -D /path/to/failing/test/dir

before this PR, this exits 0, after it exits 1.

I'm not sure how to properly integration test this, as it is in the
outer-most wrapper layer of the pex binary, so looks like it's bypassed
by the way we tend to test things?

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

For the CI failure, this keeps the spirit of the existing failing test:

diff --git a/tests/test_integration.py b/tests/test_integration.py
index 36bee7a..aa2362e 100644
--- a/tests/test_integration.py
+++ b/tests/test_integration.py
@@ -1022,10 +1022,12 @@ def pex_with_entrypoints(entry_point):
   """)
 
   my_app = dedent("""
-    from setuptools.sandbox import run_setup
-
     def do_something():
-      return run_setup
+      try:
+        from setuptools.sandbox import run_setup
+        return 0
+      except:
+        return 1
 
     if __name__ == '__main__':
       do_something()

And this is an exact parallel for setting up the test you lay out in the description:
https://github.com/pantsbuild/pex/blob/884b4596a3d2d8a862fea566d1a6d3344b56e538/tests/test_integration.py#L826-L837

from setuptools.sandbox import run_setup
return 0
except:
return 1

if __name__ == '__main__':
do_something()
Copy link
Member

Choose a reason for hiding this comment

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

You beat me to it. As you see fit: #606
The do_something() should probably be used to sys.exit.

Currently the exit code from an entrypoint is swallowed, so running pex
always exits 0.

Tested with:
```
tox -e py27-package
dist/pex27 -e pytest:main pytest -D /path/to/failing/test/dir
```

before this PR, this exits 0, after it exits 1.

I'm not sure how to properly integration test this, as it is in the
outer-most wrapper layer of the pex binary, so looks like it's bypassed
by the way we tend to test things?
@illicitonion illicitonion force-pushed the dwagnerhall/exit_from_entry_point branch from b72e767 to 4bbfade Compare October 19, 2018 16:43
tester_path = os.path.join(output_dir, 'tester.py')
results = run_pex_command(['pytest==3.9.1',
'-e', 'pytest:main',
'-D', output_dir,
Copy link
Member

Choose a reason for hiding this comment

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

Don't embed the source to test in the pex, instead maybe just: subprocess.call([pex_path, os.path.join(output_dir, 'tester.py')]).

@@ -854,7 +854,7 @@ def test_fail():
'-o', pex_path])
results.assert_success()

assert subprocess.call([pex_path, tester_path]) == 1
assert subprocess.call([pex_path, os.path.realpath(tester_path)]) == 1
Copy link
Member

Choose a reason for hiding this comment

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

Lovely, eh?

@jsirois jsirois merged commit eac0233 into pex-tool:master Oct 19, 2018
@illicitonion illicitonion deleted the dwagnerhall/exit_from_entry_point branch October 20, 2018 00:02
@jsirois jsirois mentioned this pull request Nov 3, 2018
@@ -348,7 +346,9 @@ def execute(self):
self.patch_sys(pex_inherit_path)
working_set = self._activate()
self.patch_pkg_resources(working_set)
self._wrap_coverage(self._wrap_profiling, self._execute)
exit_code = self._wrap_coverage(self._wrap_profiling, self._execute)
Copy link
Member

Choose a reason for hiding this comment

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

This was never correct. Who knows is the entry point function even returns an int? The real problem was ever using the pytest:main entrypoint. We should have been using -c pytest or else -m pytest. Pex needs to provide some guidance in its --help and docs, but using an entrypoint function (foo.bar:baz) implies you know the function does its own sys.exits to indicate failure or else raises. If it just returns some value, whether an int or a string or a what have you, PEX really can't just presume and pas that to sys.exit.

Copy link
Member

Choose a reason for hiding this comment

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

The right path may be to keep this behavior but actually document it. Pex could say "If you pass an entrypoint function and it returns anything, we'll assume that's an int that represents the exit code of the process that should be used."

It looks like sys.exit behavior is a bit whacky and gracefully supports the present use :/:

$ python -c 'import sys; sys.exit("Hello")'; echo $?
Hello
1
$ python -c 'import sys; sys.exit([])'; echo $?
[]
1
$ python -c 'import sys; sys.exit(["a"])'; echo $?
['a']
1
$ python -c 'import sys; sys.exit(True)'; echo $?
1
$ python -c 'import sys; sys.exit(False)'; echo $?
0

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.

3 participants