Skip to content

Fix bug when writing out scripts if the python interpreter has a space #276

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix bug when writing out scripts if the python interpreter has a space #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 36 additions & 35 deletions src/installer/scripts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,41 @@
"""


def _is_executable_simple(executable: bytes) -> bool:
if b" " in executable:
return False
shebang_length = len(executable) + 3 # Prefix #! and newline after.
# According to distlib, Darwin can handle up to 512 characters. But I want
# to avoid platform sniffing to make this as platform-agnostic as possible.
# The "complex" script isn't that bad anyway.
return shebang_length <= 127


def _build_shebang(executable: str, forlauncher: bool) -> bytes:
"""Build a shebang line.

The non-launcher cases are taken directly from distlib's implementation,
which tries its best to account for command length, spaces in path, etc.

https://bitbucket.org/pypa/distlib/src/58cd5c6/distlib/scripts.py#lines-124
"""
executable_bytes = executable.encode("utf-8")
if forlauncher: # The launcher can just use the command as-is.
return b"#!" + executable_bytes
if _is_executable_simple(executable_bytes):
return b"#!" + executable_bytes
Comment on lines +65 to +68
Copy link
Member

Choose a reason for hiding this comment

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

I have just noticed, that for both those cases we return exactly the same value. Can you make it into a single if statement with an or?

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 have a followup commit that does this. It's not really appropriate when moving a block of code from one location to another, to also start changing the interior design of that block.


# Shebang support for an executable with a space in it is under-specified
# and platform-dependent, so we use a clever hack to generate a script to
# run in ``/bin/sh`` that should work on all reasonably modern platforms.
# Read the following message to understand how the hack works:
# https://github.com/pypa/installer/pull/4#issuecomment-623668717

quoted = shlex.quote(executable).encode("utf-8")
# I don't understand a lick what this is trying to do.
return b"#!/bin/sh\n'''exec' " + quoted + b' "$0" "$@"\n' + b"' '''"


class InvalidScript(ValueError):
"""Raised if the user provides incorrect script section or kind."""

Expand Down Expand Up @@ -102,7 +137,7 @@ def generate(self, executable: str, kind: "LauncherKind") -> tuple[str, bytes]:
"""
launcher = self._get_launcher_data(kind)
executable = self._get_alternate_executable(executable, kind)
shebang = self._build_shebang(executable, forlauncher=bool(launcher))
shebang = _build_shebang(executable, forlauncher=bool(launcher))
code = _SCRIPT_TEMPLATE.format(
module=self.module,
import_name=self.attr.split(".")[0],
Expand All @@ -118,37 +153,3 @@ def generate(self, executable: str, kind: "LauncherKind") -> tuple[str, bytes]:
name = f"{self.name}.exe"
data = launcher + shebang + b"\n" + stream.getvalue()
return name, data

@staticmethod
def _is_executable_simple(executable: bytes) -> bool:
if b" " in executable:
return False
shebang_length = len(executable) + 3 # Prefix #! and newline after.
# According to distlib, Darwin can handle up to 512 characters. But I want
# to avoid platform sniffing to make this as platform-agnostic as possible.
# The "complex" script isn't that bad anyway.
return shebang_length <= 127

def _build_shebang(self, executable: str, forlauncher: bool) -> bytes:
"""Build a shebang line.

The non-launcher cases are taken directly from distlib's implementation,
which tries its best to account for command length, spaces in path, etc.

https://bitbucket.org/pypa/distlib/src/58cd5c6/distlib/scripts.py#lines-124
"""
executable_bytes = executable.encode("utf-8")
if forlauncher: # The launcher can just use the command as-is.
return b"#!" + executable_bytes
if self._is_executable_simple(executable_bytes):
return b"#!" + executable_bytes

# Shebang support for an executable with a space in it is under-specified
# and platform-dependent, so we use a clever hack to generate a script to
# run in ``/bin/sh`` that should work on all reasonably modern platforms.
# Read the following message to understand how the hack works:
# https://github.com/pypa/installer/pull/4#issuecomment-623668717

quoted = shlex.quote(executable).encode("utf-8")
# I don't understand a lick what this is trying to do.
return b"#!/bin/sh\n'''exec' " + quoted + b' "$0" "$@"\n' + b"' '''"
4 changes: 3 additions & 1 deletion src/installer/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
cast,
)

from installer.scripts import _build_shebang
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that I like importing and using a "private" function. I think a better solution would be to extract the part of the logic that is required for this use case into another function and having the _build_shebang use that. That should also allow _build_shebang to remain a part of the class, as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The part of the logic that is required for this use case is "all of it".

Would it make you feel more comfortable if we remove the underscore and declare it a public function?


if TYPE_CHECKING:
from installer.records import RecordEntry
from installer.scripts import LauncherKind, ScriptSection
Expand Down Expand Up @@ -186,7 +188,7 @@ def fix_shebang(stream: BinaryIO, interpreter: str) -> Iterator[BinaryIO]:
if stream.read(8) == b"#!python":
new_stream = io.BytesIO()
# write our new shebang
new_stream.write(f"#!{interpreter}\n".encode())
new_stream.write(_build_shebang(interpreter, False) + b"\n")
# copy the rest of the stream
stream.seek(0)
stream.readline() # skip first line
Expand Down