Optionally validate entry point at build time #521

wisechengyi · 2018-07-09T00:19:54Z

Resolves #508

Problem

Current behavior allows building a pex that cannot execute:

[omerta ~]$ pex requests -e qqqqqqqqqqqq -o test.pex
[omerta ~]$ ./test.pex
Traceback (most recent call last):
  File ".bootstrap/_pex/pex.py", line 367, in execute
  File ".bootstrap/_pex/pex.py", line 293, in _wrap_coverage
  File ".bootstrap/_pex/pex.py", line 325, in _wrap_profiling
  File ".bootstrap/_pex/pex.py", line 410, in _execute
  File ".bootstrap/_pex/pex.py", line 468, in execute_entry
  File ".bootstrap/_pex/pex.py", line 473, in execute_module
  File "/Users/kwilson/Python/CPython-2.7.13/lib/python2.7/runpy.py", line 182, in run_module
    mod_name, loader, code, fname = _get_module_details(mod_name)
  File "/Users/kwilson/Python/CPython-2.7.13/lib/python2.7/runpy.py", line 107, in _get_module_details
    raise error(format(e))
ImportError: No module named qqqqqqqqqqqq

Solution

Verify entry point at build time. E.g. a.b.c:m means we will try to do from a.b.c import m in a separate process.

Result

$ find hello
hello
hello/test.py
hello/tree.py

# Invalid module
$ python bin.py -D hello -e invalid.module  -o x.pex --validate-entry-point
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named invalid.module
Traceback (most recent call last):
  File "bin.py", line 758, in <module>
    main()
  File "bin.py", line 743, in main
    pex_builder.build(tmp_name, verify_entry_point=options.validate_ep)
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 529, in build
    self.freeze(bytecode_compile=bytecode_compile, verify_entry_point=verify_entry_point)
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 514, in freeze
    self._verify_entry_point()
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 263, in _verify_entry_point
    raise self.InvalidEntryPoint('Failed to:`{}`'.format(import_statement))
pex.pex_builder.InvalidEntryPoint: Failed to:`import invalid.module`

# invalid method
$ python bin.py -D hello -e test:invalid_method  -o x.pex --validate-entry-point
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: cannot import name invalid_method
Traceback (most recent call last):
  File "bin.py", line 758, in <module>
    main()
  File "bin.py", line 743, in main
    pex_builder.build(tmp_name, verify_entry_point=options.validate_ep)
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 529, in build
    self.freeze(bytecode_compile=bytecode_compile, verify_entry_point=verify_entry_point)
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 514, in freeze
    self._verify_entry_point()
  File "/Users/yic/workspace/pex/pex/pex_builder.py", line 263, in _verify_entry_point
    raise self.InvalidEntryPoint('Failed to:`{}`'.format(import_statement))
pex.pex_builder.InvalidEntryPoint: Failed to:`from test import invalid_method`

# without the flag, invalid method still works
$ python bin.py -D hello -e test:invalid_method  -o x.pex

wisechengyi · 2018-07-09T00:21:08Z

Hi @kwlzn please kindly take an initial look for sanity. I am still working on tests, so will ping when it's fully ready.

jsirois

This will be a useful feature but I think there are a few fundamental issues here to address.

jsirois · 2018-07-09T01:25:09Z

pex/pex_builder.py

@@ -238,6 +303,7 @@ def set_entry_point(self, entry_point):
    The entry point may also be specified via ``PEXBuilder.set_executable``.
    """
    self._ensure_unfrozen('Setting an entry point')
+    self._verify_entry_point(entry_point)


Since this is a builder, order of building is not guaranteed and you can't verify until after a freeze since not all requirements or files may yet be added.

Any pointer for a better place?

In freeze, where you know there can be no more code or requirement additions.

jsirois · 2018-07-09T01:27:08Z

pex/pex_builder.py

+                                   .format(ep_method, file_candidates))
+
+  def _gather_file_candidates(self, entry_point_elements):
+    # 1. Find the files matching a.b.c:m, which is a/b/c.py


It could also be a/b/c/__init__.py, ... or be a dynamically generated module! I think you really have to try to import the symbol after a freeze to avoid all the corner cases.

Importing the code feels quite sketchy, even out of process, because they could have bad side effect or even security concerns.

How do you feel about putting verifying entry point behind an option, stating that it would only check for static method in

a/b/c.py

a/b/c/__init__.py
and allow people to skip static check by disabling it?

If validation is opt-in, it seems like import would be the way to go. The user is going to run the pex after all and suffer the side-effects soon anyhow when they do so! Basically, this is forced to be a half measure one way or the other, so the choice comes down to avoiding side-effects at build time (they'll happen at runtime) for the X% of cases that have this pathology, or not supporting Y% of cases that have bizarre dynamic module generation. I'm really not sure what these percentages are or what is the better way to go. Import just seems attractive since it's robust: ie it's what pex does at runtime. Parsing is new logic.

Yeah that's fair. Importing could also get around ast parsing difference between python 2 and 3.

So I am going to do the importing, and then put it behind an option with default disabled.

wisechengyi

@jsirois ready for a second round sanity check. test still incoming.

wisechengyi · 2018-07-18T05:09:19Z

pex/pex_builder.py

    if bytecode_compile:
      self._precompile_source()
    self._frozen = True

-  def build(self, filename, bytecode_compile=True):
+  def build(self, filename, bytecode_compile=True, verify_entry_point=False):


This is the interface pants will call, correct?

changing it in a way to keep backwards compatibility.

Yes, but see above.

jsirois

Looks good, but a cleanup suggestion re subprocess / Executor and a bigger change idea to consider.

jsirois · 2018-07-18T16:32:01Z

pex/pex_builder.py

+    else:
+      raise self.InvalidEntryPoint("Fail to parse: `{}`".format(entry_point))
+
+    args = [self._interpreter.binary, '-c', import_statement]


Execution via subprocess was consolidated to Executor, so this code should use that instead of subprocess directly.

A bit more true to the runtime environment the entry point will be executed in would be something like:

with named_temporary_file() as fp: fp.write(import_statement) fp.close() result = PEX(self.chroot().path()).run([fp.name], env={'PEX_INTERPRETER': '1'}) if result != 0: raise self.InvalidEntryPoint('Failed to validate entrypoint {}'.format(entry_point))

This though unfortunately creates a circular PEX <-> PEXBuilder dep. Better I think to lift the check to the PEX constructor which could be plumbed both from pex.bin.pex and from API users (Pants).

Moved the verification step into class PEX(object):

def __init__(self, ..., verify_entry_point=False):

jsirois · 2018-07-18T16:32:33Z

pex/pex_builder.py

    if bytecode_compile:
      self._precompile_source()
    self._frozen = True

-  def build(self, filename, bytecode_compile=True):
+  def build(self, filename, bytecode_compile=True, verify_entry_point=False):


Yes, but see above.

jsirois

It looks like the one non-isort/non-style CI shard failure is legit but not related to this PR.

LGTM mod one latent bug pointed out re not using the builder's interpreter to perform the import check and completion of test work.

jsirois · 2018-07-19T17:29:29Z

pex/pex.py

+      ep_method = ep_split[1]
+      import_statement = 'from {} import {}'.format(ep_module, ep_method)
+    else:
+      raise InvalidEntryPoint("Fail to parse: `{}`".format(entry_point))


Failure or Failed would make more sense here.

jsirois · 2018-07-19T17:30:39Z

pex/bin/pex.py

@@ -741,7 +753,7 @@ def main(args=None):
    pex_builder.freeze()

    log('Running PEX file at %s with args %s' % (pex_builder.path(), cmdline), v=options.verbosity)
-    pex = PEX(pex_builder.path(), interpreter=pex_builder.interpreter)
+    pex = PEX(pex_builder.path(), interpreter=pex_builder.interpreter, verify_entry_point=options.validate_ep)


jsirois · 2018-07-19T17:31:34Z

pex/bin/pex.py

+      default=False,
+      action='store_true',
+      help='Validate the entry point by importing it in separate process. Warning: this could have '
+           'side effect. For example, entry point `a.b.c:m` will translate to '


'side effects.

jsirois · 2018-07-19T17:40:58Z

pex/bin/pex.py

@@ -732,6 +741,9 @@ def main(args=None):
      tmp_name = options.pex_name + '~'
      safe_delete(tmp_name)
      pex_builder.build(tmp_name)
+      if options.validate_ep:


You could be more uniform, more correct and fail faster with:

pex_builder = build_pex(reqs, options, resolver_options_builder) pex_builder.freeze() pex = PEX(pex_builder.path(), interpreter=pex_builder.interpreter, verify_entry_point=options.validate_ep) if options.pex_name is not None: ...

As things stand, in the options.pex_name branch, the pex is zipped up before the entrypoint validation which could be wasted work. Worse though is that the builder's custom interpreter, if any, is not used. This could be a problem if the current running interpreter does not match what the built pex needs and syntax differs (a 2/3 split).

Moved things up to fail fast.

jsirois · 2018-07-19T17:42:10Z

pex/pex.py

@@ -520,3 +522,31 @@ def run(self, args=(), with_chroot=False, blocking=True, setsid=False, **kwargs)
                                    stderr=kwargs.pop('stderr', None),
                                    **kwargs)
    return process.wait() if blocking else process
+
+  def do_entry_point_verification(self):


Given comments above re uniformity in performing the validation, I'd make this _private.

wisechengyi · 2018-07-20T22:56:53Z

looks like PEX.run would ignore -c arg.

$ python '/var/folders/yt/pxdd3gp96530xh_116kskmkh0000gn/T/tmp8rQWfk' -c 'print 123'
hello

executed the entry point (print hello) rather than -c which prints 123.
Going to try

with named_temporary_file() as fp:
  fp.write(import_statement)
  fp.close()
  result = PEX(self.chroot().path()).run([fp.name], env={'PEX_INTERPRETER': '1'})
  if result != 0:
    raise self.InvalidEntryPoint('Failed to validate entrypoint {}'.format(entry_point))

as suggested earlier.

wisechengyi · 2018-07-20T23:36:12Z

Updated. Tests incoming.

wisechengyi · 2018-07-21T05:15:07Z

Tests added. Ready for final review.

jsirois

Thanks Yi!

There was no proncipled reason to use it. Neither the originating commit or its originiating issue refer to Pillow and the tests themselves don't implicate anything being required except a 3rdparty lib. See: + pex-tool#521 + pex-tool#508

wisechengyi added 7 commits July 6, 2018 21:44

initial

6a45d41

can verify file existence

df5e696

can verify both

1e5c2c4

works fine

67e7010

cmt

bf58ade

rev init

fec3647

nit

89eaf6d

jsirois requested changes Jul 9, 2018

View reviewed changes

wisechengyi added 5 commits July 17, 2018 17:11

use import to check entry point validity

b07e726

fmt

1756370

option

f9136a9

flag works

883d13a

reorder sig

ca5fa38

wisechengyi force-pushed the vm_pr branch from ee4b3a5 to dd9ea02 Compare July 18, 2018 04:56

diff module and method

a2cd12a

wisechengyi force-pushed the vm_pr branch from dd9ea02 to 020974a Compare July 18, 2018 05:06

fmt

738dd48

wisechengyi force-pushed the vm_pr branch from 020974a to da83fbf Compare July 18, 2018 05:08

wisechengyi commented Jul 18, 2018

View reviewed changes

add tests

1e23269

wisechengyi force-pushed the vm_pr branch from da83fbf to 7ed1ba3 Compare July 18, 2018 06:07

fix comment

394bb7f

wisechengyi force-pushed the vm_pr branch from 7ed1ba3 to 706cad6 Compare July 18, 2018 06:11

refactor tests

973cdf1

wisechengyi force-pushed the vm_pr branch from 706cad6 to bdb46e0 Compare July 18, 2018 06:31

wisechengyi added 2 commits July 17, 2018 23:33

minor fix

c255c28

pr

5fd3791

wisechengyi force-pushed the vm_pr branch from bdb46e0 to 5fd3791 Compare July 18, 2018 06:33

jsirois reviewed Jul 18, 2018

View reviewed changes

jsirois reviewed Jul 19, 2018

View reviewed changes

wisechengyi added 2 commits July 20, 2018 14:39

fmt

b60cc7b

minor

4d07117

wisechengyi added 4 commits July 20, 2018 16:27

execute a file containing import

70701bb

remove dup code

6ef2990

privatize do_entry_point_verification

94de45c

fmt

08bb959

wisechengyi force-pushed the vm_pr branch from 99b19c0 to 0851e62 Compare July 20, 2018 23:33

wisechengyi added 4 commits July 20, 2018 17:17

test_pex.py starts to work

469ea27

pex tests

7f1c478

integration tests

f60595a

style

ac0bf83

wisechengyi force-pushed the vm_pr branch from 0851e62 to 93298a4 Compare July 21, 2018 04:34

encoding

23eb42e

wisechengyi force-pushed the vm_pr branch from 93298a4 to bbdad93 Compare July 21, 2018 04:49

further fix encoding

9903ec0

wisechengyi force-pushed the vm_pr branch from bbdad93 to 8a71b1e Compare July 21, 2018 04:54

wisechengyi added 2 commits July 20, 2018 21:58

skip pypy

8ccf87c

for pr

9ec096a

wisechengyi force-pushed the vm_pr branch from 8a71b1e to 9ec096a Compare July 21, 2018 05:00

wisechengyi requested review from kwlzn and CMLivingston July 21, 2018 05:22

jsirois approved these changes Jul 21, 2018

View reviewed changes

wisechengyi changed the title ~~Validate entry point at build time~~ Optionally validate entry point at build time Jul 23, 2018

wisechengyi merged commit d1f946c into master Jul 23, 2018

wisechengyi deleted the vm_pr branch July 23, 2018 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally validate entry point at build time #521

Optionally validate entry point at build time #521

wisechengyi commented Jul 9, 2018 •

edited

wisechengyi commented Jul 9, 2018

jsirois left a comment

jsirois Jul 9, 2018

wisechengyi Jul 9, 2018

jsirois Jul 9, 2018

jsirois Jul 9, 2018

wisechengyi Jul 9, 2018

jsirois Jul 9, 2018

wisechengyi Jul 9, 2018 •

edited

wisechengyi left a comment

wisechengyi Jul 18, 2018

jsirois Jul 18, 2018

jsirois left a comment

jsirois Jul 18, 2018

wisechengyi Jul 18, 2018

jsirois Jul 18, 2018

jsirois left a comment

jsirois Jul 19, 2018

wisechengyi Jul 20, 2018

jsirois Jul 19, 2018 •

edited

wisechengyi Jul 20, 2018

jsirois Jul 19, 2018

wisechengyi Jul 20, 2018

jsirois Jul 19, 2018 •

edited

wisechengyi Jul 20, 2018

jsirois Jul 19, 2018

wisechengyi Jul 20, 2018

wisechengyi commented Jul 20, 2018 •

edited

wisechengyi commented Jul 20, 2018

wisechengyi commented Jul 21, 2018

jsirois left a comment

Optionally validate entry point at build time #521

Optionally validate entry point at build time #521

Conversation

wisechengyi commented Jul 9, 2018 • edited

Problem

Solution

Result

wisechengyi commented Jul 9, 2018

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi Jul 9, 2018 • edited

Choose a reason for hiding this comment

wisechengyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi commented Jul 20, 2018 • edited

wisechengyi commented Jul 20, 2018

wisechengyi commented Jul 21, 2018

jsirois left a comment

Choose a reason for hiding this comment

wisechengyi commented Jul 9, 2018 •

edited

wisechengyi Jul 9, 2018 •

edited

jsirois Jul 19, 2018 •

edited

jsirois Jul 19, 2018 •

edited

wisechengyi commented Jul 20, 2018 •

edited