Repository: linkedin/shiv

Issue:
allow use generic "/usr/bin/env python3.6" shebang?
Hello,

I played a bit with shiv and noticed that it always writes the full path to a python interpreter (sys.executable or user provided one) in the zipapp's shebang. Which means, if I try to run the same pyz on a different machine where python3.6 is installed in a different location, it won't run.

https://github.com/linkedin/shiv/blob/29fb7c316fa7b1f9111cade730b2168a2c4f4a6e/src/shiv/cli.py#L56

https://github.com/linkedin/shiv/blob/29fb7c316fa7b1f9111cade730b2168a2c4f4a6e/src/shiv/builder.py#L39-L43

https://github.com/linkedin/shiv/blob/29fb7c316fa7b1f9111cade730b2168a2c4f4a6e/src/shiv/pip.py#L46

With [pex](https://pex.readthedocs.io/en/stable/buildingpex.html#invoking-the-pex-utility), one can specify the interpreter to use either as absolute path or just the name of a Python interpreter within the environment.

It would be nice if shiv also allowed to do that.

Thank you

Discussion:
warsaw: What do you think about keeping `-sE` if `sys.executable` (the default) is used?
sixninetynine: @warsaw I thought about that too, but ultimately felt like it would be confusing... say someone didn't want `-sE`, they'd have to explicitly set `--python` to be (effectively) the same as `sys.executable`.
warsaw: True, but OTOH if they just want the defaults (which should be the most common case, right?) they'll have to use `--python` to add the isolation.

I guess it's a 6 vs 1/2 dozen thing, so as long as the change is well communicated and the documentation is up to date, I don't have a strong opinion either way.
warsaw: Re: Internally - will an outside consumer know what that means?  Maybe just suggest that for better isolation from the system's and user's Python environment, these flags can be used?

Re: default use of the flags - I'm not sure whether it's non-typical because folks don't think about it, or whether they really do no want isolation in general.

None of this is a blocker for me though.  Just thoughts about usability.
sixninetynine: @warsaw I updated the wording in the history doc to be more clear, but rather than try and tack on `-sE` I think it will be better to leave that up to individual users. That way using `--python` vs not supplying it will be similar enough to not be confusing. (and imagine if someone supplied `-sE` without knowing that we add it by default! confusing!).

Patch:
diff --git a/docs/history.rst b/docs/history.rst
index a3f49f4..244d72d 100644
--- a/docs/history.rst
+++ b/docs/history.rst
@@ -36,9 +36,11 @@ injecting said dependencies at runtime. We have to credit the great work by @wic
 The primary differences between PEX and shiv are:
 
 * ``shiv`` completey avoids the use of ``pkg_resources``. If it is included by a transitive
-  dependency, the performance implications are mitigated by limiting the length of ``sys.path`` and
-  always including the `-s <https://docs.python.org/3/using/cmdline.html#cmdoption-s>`_ and
-  `-E <https://docs.python.org/3/using/cmdline.html#cmdoption-e>`_ Python interpreter flags.
+  dependency, the performance implications are mitigated by limiting the length of ``sys.path``.
+  Internally, at LinkedIn, we always include the
+  `-s <https://docs.python.org/3/using/cmdline.html#cmdoption-s>`_ and
+  `-E <https://docs.python.org/3/using/cmdline.html#cmdoption-e>`_ Python interpreter flags by
+  specifying ``--python "/path/to/python -sE"``, which ensures a clean environment.
 * Instead of shipping our binary with downloaded wheels inside, we package an entire site-packages
   directory, as installed by ``pip``. We then bootstrap that directory post-extraction via the
   stdlib's ``site.addsitedir`` function. That way, everything works out of the box: namespace
diff --git a/src/shiv/builder.py b/src/shiv/builder.py
index 640a560..2d2b1f2 100644
--- a/src/shiv/builder.py
+++ b/src/shiv/builder.py
@@ -14,6 +14,8 @@ import zipapp
 from pathlib import Path
 from typing import Any, IO, Generator, Union
 
+from .constants import BINPRM_ERROR
+
 # Typical maximum length for a shebang line
 BINPRM_BUF_SIZE = 128
 
@@ -25,24 +27,17 @@ import {module}
 """
 
 
-def write_file_prefix(f: IO[Any], interpreter_path: Path) -> None:
+def write_file_prefix(f: IO[Any], interpreter: str) -> None:
     """Write a shebang line.
 
-    .. note::
-
-        Shiv explicitly uses `-sE` as start up flags to prevent contamination of sys.path.
-
     :param f: An open file handle.
-    :param interpreter_path: A path to a python interpreter.
+    :param interpreter: A path to a python interpreter.
     """
+    # if the provided path is too long for a shebang we should error out
+    if len(interpreter) > BINPRM_BUF_SIZE:
+        sys.exit(BINPRM_ERROR)
 
-    # fall back to /usr/bin/env if the interp path is too long
-    if len(interpreter_path.as_posix()) > BINPRM_BUF_SIZE:
-        shebang = f"/usr/bin/env {interpreter_path.name}"
-    else:
-        shebang = interpreter_path.as_posix()
-
-    f.write(b"#!" + shebang.encode(sys.getfilesystemencoding()) + b" -sE\n")
+    f.write(b"#!" + interpreter.encode(sys.getfilesystemencoding()) + b"\n")
 
 
 @contextlib.contextmanager
@@ -56,7 +51,11 @@ def maybe_open(archive: Union[str, Path], mode: str) -> Generator[IO[Any], None,
 
 
 def create_archive(
-    source: Path, target: Path, interpreter: Path, main: str, compressed: bool = True
+    source: Path,
+    target: Path,
+    interpreter: str,
+    main: str,
+    compressed: bool = True
 ) -> None:
     """Create an application archive from SOURCE.
 
@@ -95,5 +94,4 @@ def create_archive(
             z.writestr("__main__.py", main_py.encode("utf-8"))
 
     # make executable
-    if interpreter and not hasattr(target, "write"):
-        target.chmod(target.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
+    target.chmod(target.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
diff --git a/src/shiv/cli.py b/src/shiv/cli.py
index d5ea025..5c88546 100644
--- a/src/shiv/cli.py
+++ b/src/shiv/cli.py
@@ -20,7 +20,6 @@ from .constants import (
     NO_PIP_ARGS,
     NO_OUTFILE,
     NO_ENTRY_POINT,
-    INVALID_PYTHON,
 )
 
 __version__ = '0.0.25'
@@ -44,24 +43,6 @@ def find_entry_point(site_packages: Path, console_script: str) -> str:
     return config_parser["console_scripts"][console_script]
 
 
-def validate_interpreter(interpreter_path: Optional[str] = None) -> Path:
-    """Ensure that the interpreter is a real path, not a symlink.
-
-    If no interpreter is given, default to `sys.exectuable`
-
-    :param interpreter_path: A path to a Python interpreter.
-    """
-    real_path = Path(sys.executable) if interpreter_path is None else Path(
-        interpreter_path
-    )
-
-    if real_path.exists():
-        return real_path
-
-    else:
-        sys.exit(INVALID_PYTHON.format(path=real_path))
-
-
 def copy_bootstrap(bootstrap_target: Path) -> None:
     """Copy bootstrap code from shiv into the pyz.
 
@@ -124,16 +105,12 @@ def main(
                     )
                 )
 
-    # validate supplied python (if any)
-    interpreter = validate_interpreter(python)
-
     with TemporaryDirectory() as working_path:
         site_packages = Path(working_path, "site-packages")
         site_packages.mkdir(parents=True, exist_ok=True)
 
         # install deps into staged site-packages
         pip.install(
-            python or sys.executable,
             ["--target", site_packages.as_posix()] + list(pip_args),
         )
 
@@ -163,7 +140,7 @@ def main(
         builder.create_archive(
             Path(working_path),
             target=Path(output_file),
-            interpreter=interpreter,
+            interpreter=python or sys.executable,
             main="_bootstrap:bootstrap",
             compressed=compressed,
         )
diff --git a/src/shiv/constants.py b/src/shiv/constants.py
index 33d742b..58e0bc5 100644
--- a/src/shiv/constants.py
+++ b/src/shiv/constants.py
@@ -5,9 +5,9 @@ from typing import Tuple, Dict
 DISALLOWED_PIP_ARGS = "\nYou supplied a disallowed pip argument! '{arg}'\n\n{reason}\n"
 NO_PIP_ARGS = "\nYou must supply PIP ARGS!\n"
 NO_OUTFILE = "\nYou must provide an output file option! (--output-file/-o)\n"
-INVALID_PYTHON = "\nInvalid python interpreter! {path} does not exist!\n"
 NO_ENTRY_POINT = "\nNo entry point '{entry_point}' found in the console_scripts!\n"
 PIP_INSTALL_ERROR = "\nPip install failed!\n"
+BINPRM_ERROR = "\nShebang is too long, it would exceed BINPRM_BUF_SIZE! Consider /usr/bin/env"
 
 # pip
 PIP_INSTALL_ERROR = "\nPip install failed!\n"
diff --git a/src/shiv/pip.py b/src/shiv/pip.py
index 92481f0..8ab2a4c 100644
--- a/src/shiv/pip.py
+++ b/src/shiv/pip.py
@@ -42,14 +42,14 @@ def clean_pip_env() -> Generator[None, None, None]:
             pydistutils.unlink()
 
 
-def install(interpreter_path: str, args: List[str]) -> None:
+def install(args: List[str]) -> None:
     """`pip install` as a function.
 
     Accepts a list of pip arguments.
 
     .. code-block:: py
 
-        >>> install('/usr/local/bin/python3', ['numpy', '--target', 'site-packages'])
+        >>> install(['numpy', '--target', 'site-packages'])
         Collecting numpy
         Downloading numpy-1.13.3-cp35-cp35m-manylinux1_x86_64.whl (16.9MB)
             100% || 16.9MB 53kB/s
@@ -60,7 +60,7 @@ def install(interpreter_path: str, args: List[str]) -> None:
     with clean_pip_env():
 
         process = subprocess.Popen(
-            [interpreter_path, "-m", "pip", "--disable-pip-version-check", "install"] + args,
+            [sys.executable, "-m", "pip", "--disable-pip-version-check", "install"] + args,
             stdout=subprocess.PIPE,
             stderr=subprocess.STDOUT,
         )


Test patch:
diff --git a/test/test_bootstrap.py b/test/test_bootstrap.py
index 1da3885..5734f0c 100644
--- a/test/test_bootstrap.py
+++ b/test/test_bootstrap.py
@@ -8,6 +8,8 @@ from code import interact
 from uuid import uuid4
 from zipfile import ZipFile
 
+import pytest
+
 from unittest import mock
 
 from shiv.bootstrap import import_string, current_zipfile, cache_path
@@ -33,6 +35,14 @@ class TestBootstrap:
         from os.path import join
         assert func == join
 
+        # test something already imported
+        import shiv
+        assert import_string('shiv') == shiv == sys.modules['shiv']
+
+        # test bogus imports raise properly
+        with pytest.raises(ImportError):
+            import_string('this is bogus!')
+
     def test_is_zipfile(self, zip_location):
         assert not current_zipfile()
 
diff --git a/test/test_builder.py b/test/test_builder.py
index fd3e369..8b2ec04 100644
--- a/test/test_builder.py
+++ b/test/test_builder.py
@@ -8,36 +8,54 @@ from zipapp import ZipAppError
 
 import pytest
 
-from shiv.cli import validate_interpreter
 from shiv.builder import write_file_prefix, create_archive
 
 
 UGOX = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
 
 
-class TestBuilder:
-    def test_file_prefix(self):
-        with tempfile.TemporaryFile() as fd:
-            python = validate_interpreter(Path(sys.executable))
-            write_file_prefix(fd, python)
-            fd.seek(0)
-            written = fd.read()
+def tmp_write_prefix(interpreter):
+    with tempfile.TemporaryFile() as fd:
+        write_file_prefix(fd, interpreter)
+        fd.seek(0)
+        written = fd.read()
+
+    return written
+
 
-        assert written == b'#!' + python.as_posix().encode(sys.getdefaultencoding()) + b' -sE\n'
+class TestBuilder:
+    @pytest.mark.parametrize(
+        'interpreter,expected', [
+            ('/usr/bin/python', b'#!/usr/bin/python\n'),
+            ('/usr/bin/env python', b'#!/usr/bin/env python\n'),
+            ('/some/other/path/python -sE', b'#!/some/other/path/python -sE\n'),
+        ]
+    )
+    def test_file_prefix(self, interpreter, expected):
+        assert tmp_write_prefix(interpreter) == expected
+
+    def test_binprm_error(self):
+        with pytest.raises(SystemExit):
+            tmp_write_prefix(f"/{'c' * 200}/python")
 
     def test_create_archive(self, sp):
         with tempfile.TemporaryDirectory() as tmpdir:
             target = Path(tmpdir, 'test.zip')
-            create_archive(sp, target, validate_interpreter(None), 'code:interact')
+
+            # create an archive
+            create_archive(sp, target, sys.executable, 'code:interact')
+
+            # create one again (to ensure we overwrite)
+            create_archive(sp, target, sys.executable, 'code:interact')
 
             assert zipfile.is_zipfile(str(target))
 
             with pytest.raises(ZipAppError):
-                create_archive(sp, target, validate_interpreter(None), 'alsjdbas,,,')
+                create_archive(sp, target, sys.executable, 'alsjdbas,,,')
 
     def test_archive_permissions(self, sp):
         with tempfile.TemporaryDirectory() as tmpdir:
             target = Path(tmpdir, 'test.zip')
-            create_archive(sp, target, validate_interpreter(None), 'code:interact')
+            create_archive(sp, target, sys.executable, 'code:interact')
 
             assert target.stat().st_mode & UGOX == UGOX
diff --git a/test/test_cli.py b/test/test_cli.py
index a38017c..5fef26a 100644
--- a/test/test_cli.py
+++ b/test/test_cli.py
@@ -1,5 +1,4 @@
 import subprocess
-import sys
 import tempfile
 
 from pathlib import Path
@@ -8,7 +7,7 @@ import pytest
 
 from click.testing import CliRunner
 
-from shiv.cli import main, validate_interpreter
+from shiv.cli import main
 from shiv.constants import DISALLOWED_PIP_ARGS, NO_PIP_ARGS, NO_OUTFILE, BLACKLISTED_ARGS
 
 
@@ -69,13 +68,3 @@ class TestCLI:
             # now run the produced zipapp
             with subprocess.Popen([output_file], stdout=subprocess.PIPE) as proc:
                 assert proc.stdout.read().decode() == 'hello world\n'
-
-    def test_interpreter(self):
-        assert validate_interpreter(None) == validate_interpreter() == Path(sys.executable)
-
-        with pytest.raises(SystemExit):
-            validate_interpreter(Path('/usr/local/bogus_python'))
-
-    @pytest.mark.skipif(len(sys.executable) > 128, reason='only run this test is the shebang is not too long')
-    def test_real_interpreter(self):
-        assert validate_interpreter(Path(sys.executable)) == Path(sys.executable)