-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Use robust shebang code for all shebang modification #13390
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
6da1037
b5917be
cb9d513
1f3452f
a460e1f
fa92def
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Python scripts added as :file:`data` files (e.g. via ``setuptools``\ ’ ``scripts=`` parameter) | ||
now get their shebang modified like regular scripts, and no longer break for venv paths with spaces. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,14 +90,13 @@ def fix_script(path: str) -> bool: | |
assert os.path.isfile(path) | ||
|
||
with open(path, "rb") as script: | ||
firstline = script.readline() | ||
if not firstline.startswith(b"#!python"): | ||
prelude = script.readline() | ||
if (m := re.match(rb"^#!python[^\s]*(\s.*)?$", prelude)) is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't conform to the spec, which only allows rewriting lines starting with precisely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you misread: that’s exactly what that regex matches. The only reason I switched from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I did misread. But I'd still argue that the regex is harder to read. Why not simply split It's worth noting that there are some edge cases in the spec - The current code doesn't get this right, either, but if we're going to claim the new code "is more robust" or "will no longer break", we should at least try to follow the spec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
and strip the trailing newline from the second part. I find the regex to be more readable than that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, we disagree. (BTW, without documentation on |
||
return False | ||
exename = sys.executable.encode(sys.getfilesystemencoding()) | ||
firstline = b"#!" + exename + os.linesep.encode("ascii") | ||
prelude = ScriptMaker(None, None)._get_shebang("utf-8", m.group(1) or b"") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'd suggest that we need a test here that ensures that |
||
rest = script.read() | ||
with open(path, "wb") as script: | ||
script.write(firstline) | ||
script.write(prelude) | ||
script.write(rest) | ||
return True | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,26 @@ | ||
from __future__ import annotations | ||
|
||
import base64 | ||
import csv | ||
import hashlib | ||
import os | ||
import shutil | ||
import sysconfig | ||
from pathlib import Path | ||
from typing import Any | ||
from typing import TYPE_CHECKING, Any | ||
|
||
import pytest | ||
|
||
from tests.lib import PipTestEnvironment, TestData, create_basic_wheel_for_package | ||
from tests.lib import create_basic_wheel_for_package | ||
from tests.lib.wheel import WheelBuilder, make_wheel | ||
|
||
from ..lib.venv import VirtualEnvironment | ||
|
||
if TYPE_CHECKING: | ||
from collections.abc import Callable | ||
|
||
from tests.lib import PipTestEnvironment, ScriptFactory, TestData | ||
|
||
|
||
# assert_installed expects a package subdirectory, so give it to them | ||
def make_wheel_with_file(name: str, version: str, **kwargs: Any) -> WheelBuilder: | ||
|
@@ -366,13 +375,22 @@ def test_wheel_record_lines_have_hash_for_data_files( | |
] | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"ws_dirname", ["work space", "workspace"], ids=["spaces", "no_spaces"] | ||
) | ||
def test_wheel_record_lines_have_updated_hash_for_scripts( | ||
script: PipTestEnvironment, | ||
tmpdir: Path, | ||
virtualenv_factory: Callable[[Path], VirtualEnvironment], | ||
script_factory: ScriptFactory, | ||
ws_dirname: str, | ||
) -> None: | ||
""" | ||
pip rewrites "#!python" shebang lines in scripts when it installs them; | ||
make sure it updates the RECORD file correspondingly. | ||
""" | ||
(tmpdir / ws_dirname).mkdir(exist_ok=True, parents=True) | ||
virtualenv = virtualenv_factory(tmpdir / ws_dirname / "venv") | ||
script = script_factory(tmpdir / ws_dirname, virtualenv) | ||
package = make_wheel( | ||
"simple", | ||
"0.1.0", | ||
|
@@ -388,7 +406,12 @@ def test_wheel_record_lines_have_updated_hash_for_scripts( | |
|
||
script_path = script.bin_path / "dostuff" | ||
script_contents = script_path.read_bytes() | ||
assert not script_contents.startswith(b"#!python\n") | ||
expected_prefix = ( | ||
b"#!/bin/sh\n'''exec'" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strong -1 on having the test rely on the precise mechanism used in the generated shebang.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how should I do it? without knowledge of the mechanism, checks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know. It's a hard problem. Distlib has a bunch of tests around shebangs, so if we were using documented and supported distlib APIs, I'd say we could rely on them (and the existing check, that just confirms we did rewrite, would be enough). But because we're using the internal The fact that tests are failing on Windows demonstrates that this test isn't sufficient - the shebang you're checking for isn't what gets used on Windows (because Windows shells don't support it). |
||
if " " in ws_dirname | ||
else f"#!{script.bin_path}{os.path.sep}python".encode() | ||
) | ||
assert script_contents.startswith(expected_prefix) | ||
|
||
script_digest = hashlib.sha256(script_contents).digest() | ||
script_digest_b64 = ( | ||
|
Uh oh!
There was an error while loading. Please reload this page.