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

Fix incorrect sys.argv[0] path when calling project scripts #6737

Merged

Conversation

wagnerluis1982
Copy link
Contributor

Pull Request Check List

Resolves: #965
Closes: #1001

  • Added tests for changed code.
  • Updated documentation for changed code.

This PR supersedes PR #1001. I am reusing the code with the PR author's blessing, @chdsbd (see #1001 (comment)). The work I did here is the following:

  1. I rebased the original code against the latest master and resolved the conflicts.
  2. I handled the request for change by @finswimmer (see fix: RunCommand calling scripts with incorrect executable path #1001 (review))
  3. I rewrote the test to use a real pyproject.toml and added a second test case

Description copied from PR #1001:

Calling sys.argv should run the same program as the currently running program. To make calling Poetry scripts through RunCommand match this behavior, we must set sys.argv[0] to be the full path of the executable.

This change makes the behavior of calling a script through poetry run the same as calling a script directly from the .venv/bin.

@wagnerluis1982
Copy link
Contributor Author

Test on Windows is tricky 😅

src/poetry/console/commands/run.py Outdated Show resolved Hide resolved
src/poetry/console/commands/run.py Outdated Show resolved Hide resolved
@wagnerluis1982 wagnerluis1982 force-pushed the fix-incorrect-executable-path branch 4 times, most recently from 44d627a to 8725493 Compare October 13, 2022 15:23
@wagnerluis1982 wagnerluis1982 force-pushed the fix-incorrect-executable-path branch 3 times, most recently from 86b7745 to 438eb2e Compare October 31, 2022 09:58
@wagnerluis1982 wagnerluis1982 force-pushed the fix-incorrect-executable-path branch 3 times, most recently from 24e047e to 4114546 Compare November 4, 2022 06:54
chdsbd and others added 10 commits November 13, 2022 20:25
Fixes python-poetryGH-965

Calling `sys.argv` should run the same program as the currently
running program. To make calling Poetry scripts through RunCommand
match this behavior, we must set `sys.argv[0]` to be the full path of
the executable.

This change makes the behavior of calling a script through `poetry run`
the same as calling a script directly from the .venv/bin.
There are probably cleaner ways to test the CLI, so let me know.
@wagnerluis1982
Copy link
Contributor Author

@radoering I didn't proper answered you regarding the script_dirs:

On this PR, I am not concerned about locations. I rely locations to Env._bin method (that I turned public and renamed to Env.get_bin_path).

But checking the underlying implementation of the turned-public method, it makes use of Env._bin_dir not Env.script_dirs.

@radoering
Copy link
Member

I still think script_dirs is more correct. I suppose it may be possible that _bin_dir is read-only and scripts are installed in another directory. By the way, Env._bin/Env.get_bin_path does not work for Windows because scripts are not installed as ".exe" but ".cmd". I adapted the test so that a real script is installed by poetry to avoid making false assumptions.

#6971 assumes the script is always installed to the bin/Scripts folder, which will cause an issue if you add an entry to [tool.poetry.scripts] after you run poetry install.

This is very valuable input. With that information I'm quite confident we can just iterate over script_dirs and if we don't find the script it should be fine to keep argv0 as is.

Please take a look at my changes and let me know if you have any concerns.

@wagnerluis1982
Copy link
Contributor Author

Hi @radoering, thank you for the update.

Regarding script_dirs I am still unsure about it. From what I can see in the code script_dirs[0] is always the target folder for installed scripts.

It seems to me you're trying to solve the generic case, i.e. run any command, but run_script method is used only for scripts in the [tool.poetry.scripts].

My reasoning is the following: in the RunCommand.handle there is a condition (lines 27, 28) which uses run_script when the script exists in the [tool.poetry.scripts], otherwise calls self.env.execute without any preprocessing.

def handle(self) -> int:
args = self.argument("args")
script = args[0]
scripts = self.poetry.local_config.get("scripts")
if scripts and script in scripts:
return self.run_script(scripts[script], args)
try:
return self.env.execute(*args)
except FileNotFoundError:
self.line_error(f"<error>Command not found: <c1>{script}</c1></error>")
return 1

@radoering
Copy link
Member

The root project is built/installed here:

And here you can see that script_dirs is searched for a writable directory to install the scripts:

def _add_scripts(self) -> list[Path]:
added = []
entry_points = self.convert_entry_points()
for scripts_path in self._env.script_dirs:
if is_dir_writable(path=scripts_path, create=True):
break
else:
self._io.write_error_line(
" - Failed to find a suitable script installation directory for"
f" {self._poetry.file.parent}"
)
return []

@wagnerluis1982
Copy link
Contributor Author

Makes sense. I'm convinced!

PS: I don't quite understand how script_path.exists() is working on Windows, given you're running without extension in the tests 😱

@radoering
Copy link
Member

It's a bit weird. 😄 There are two files on Windows. One without extension and one with ".cmd". And even when executing without an extension the ".cmd" seems to be used. Strictly speaking, we had to check for the existence of the ".cmd", but since we are creating both files, it's probably not that relevant. I assume we could also add the ".cmd" for argv0. Might be cleaner. 🤷

with script_file.open("w", encoding="utf-8") as f:
f.write(
decode(
SCRIPT_TEMPLATE.format(
python=self._env.python,
module=module,
callable_holder=callable_holder,
callable_=callable_,
)
)
)
script_file.chmod(0o755)
added.append(script_file)
if WINDOWS:
cmd_script = script_file.with_suffix(".cmd")
cmd = WINDOWS_CMD_TEMPLATE.format(python=self._env.python, script=name)
self._debug(
f" - Adding the <c2>{cmd_script.name}</c2> script wrapper to"
f" <b>{scripts_path}</b>"
)
with cmd_script.open("w", encoding="utf-8") as f:
f.write(decode(cmd))
added.append(cmd_script)

src/poetry/console/commands/run.py Outdated Show resolved Hide resolved
tests/console/commands/test_run.py Show resolved Hide resolved
tests/console/commands/test_run.py Show resolved Hide resolved
@wagnerluis1982
Copy link
Contributor Author

I pushed a commit to include .cmd on Windows (tests are passing).

Also, I added a few minor contextual comments to the code.

@wagnerluis1982 wagnerluis1982 mentioned this pull request Jan 22, 2023
@radoering radoering dismissed neersighted’s stale review January 23, 2023 05:32

Everything fixed or no longer relevant.

@radoering radoering merged commit 33242c2 into python-poetry:master Jan 23, 2023
@wagnerluis1982 wagnerluis1982 deleted the fix-incorrect-executable-path branch January 23, 2023 07:20
@wagnerluis1982
Copy link
Contributor Author

wagnerluis1982 commented Feb 17, 2023 via email

@davidism
Copy link

davidism commented Feb 17, 2023

It should probably be ["-c"] instead of the name of the Poetry script if it's not installed. That mirrors Python's sys.argv behavior for what Poetry is actually doing internally. Or Poetry could set it to [imported_module.__file__] regardless of if the script is installed or not. No tool inspecting sys.argv would know what to do with the name on its own.

sys.orig_argv can be used (and shouldn't be modified by Poetry) to get the original command line that would produce the same execution.

@wagnerluis1982
Copy link
Contributor Author

@davidism I think is sort of pointless to discuss this here, in a closed PR. Better to focus the discussion on the open issue #7528

mwalbeck pushed a commit to mwalbeck/docker-python-poetry that referenced this pull request Feb 28, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [poetry](https://python-poetry.org/) ([source](https://github.com/python-poetry/poetry), [changelog](https://python-poetry.org/history/)) | minor | `1.3.2` -> `1.4.0` |

---

### Release Notes

<details>
<summary>python-poetry/poetry</summary>

### [`v1.4.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#&#8203;140---2023-02-27)

[Compare Source](python-poetry/poetry@1.3.2...1.4.0)

##### Added

-   **Add a modern installer (`installer.modern-installation`) for faster installation of packages and independence from pip** ([#&#8203;6205](python-poetry/poetry#6205)).
-   Add support for `Private ::` trove classifiers ([#&#8203;7271](python-poetry/poetry#7271)).
-   Add the version of poetry in the `@generated` comment at the beginning of the lock file ([#&#8203;7339](python-poetry/poetry#7339)).
-   Add support for `virtualenvs.prefer-active-python` when running `poetry new` and `poetry init` ([#&#8203;7100](python-poetry/poetry#7100)).

##### Changed

-   **Deprecate the old installer, i.e. setting `experimental.new-installer` to `false`** ([#&#8203;7358](python-poetry/poetry#7358)).
-   Remove unused `platform` field from cached package info and bump the cache version ([#&#8203;7304](python-poetry/poetry#7304)).
-   Extra dependencies of the root project are now sorted in the lock file ([#&#8203;7375](python-poetry/poetry#7375)).
-   Remove upper boundary for `importlib-metadata` dependency ([#&#8203;7434](python-poetry/poetry#7434)).
-   Validate path dependencies during use instead of during construction ([#&#8203;6844](python-poetry/poetry#6844)).
-   Remove the deprecated `repository` modules ([#&#8203;7468](python-poetry/poetry#7468)).

##### Fixed

-   Fix an issue where an unconditional dependency of an extra was not installed in specific environments ([#&#8203;7175](python-poetry/poetry#7175)).
-   Fix an issue where a pre-release of a dependency was chosen even if a stable release fulfilled the constraint ([#&#8203;7225](python-poetry/poetry#7225), [#&#8203;7236](python-poetry/poetry#7236)).
-   Fix an issue where HTTP redirects were not handled correctly during publishing ([#&#8203;7160](python-poetry/poetry#7160)).
-   Fix an issue where `poetry check` did not handle the `-C, --directory` option correctly ([#&#8203;7241](python-poetry/poetry#7241)).
-   Fix an issue where the subdirectory information of a git dependency was not written to the lock file ([#&#8203;7367](python-poetry/poetry#7367)).
-   Fix an issue where the wrong Python version was selected when creating an virtual environment ([#&#8203;7221](python-poetry/poetry#7221)).
-   Fix an issue where packages that should be kept were uninstalled when calling `poetry install --sync` ([#&#8203;7389](python-poetry/poetry#7389)).
-   Fix an issue where an incorrect value was set for `sys.argv[0]` when running installed scripts ([#&#8203;6737](python-poetry/poetry#6737)).
-   Fix an issue where hashes in `direct_url.json` files were not written according to the specification ([#&#8203;7475](python-poetry/poetry#7475)).
-   Fix an issue where poetry commands failed due to special characters in the path of the project or virtual environment ([#&#8203;7471](python-poetry/poetry#7471)).
-   Fix an issue where poetry crashed with a `JSONDecodeError` when running a Python script that produced certain warnings ([#&#8203;6665](python-poetry/poetry#6665)).

##### Docs

-   Add advice on how to maintain a poetry plugin ([#&#8203;6977](python-poetry/poetry#6977)).
-   Update tox examples to comply with the latest tox release ([#&#8203;7341](python-poetry/poetry#7341)).
-   Mention that the `poetry export` can export `constraints.txt` files ([#&#8203;7383](python-poetry/poetry#7383)).
-   Add clarifications for moving configuration files ([#&#8203;6864](python-poetry/poetry#6864)).
-   Mention the different types of exact version specifications ([#&#8203;7503](python-poetry/poetry#7503)).

##### poetry-core ([`1.5.1`](https://github.com/python-poetry/poetry-core/releases/tag/1.5.1))

-   Improve marker handling ([#&#8203;528](python-poetry/poetry-core#528),
    [#&#8203;534](python-poetry/poetry-core#534),
    [#&#8203;530](python-poetry/poetry-core#530),
    [#&#8203;546](python-poetry/poetry-core#546),
    [#&#8203;547](python-poetry/poetry-core#547)).
-   Validate whether dependencies referenced in `extras` are defined in the main dependency group ([#&#8203;542](python-poetry/poetry-core#542)).
-   Poetry no longer generates a `setup.py` file in sdists by default ([#&#8203;318](python-poetry/poetry-core#318)).
-   Fix an issue where trailing newlines were allowed in `tool.poetry.description` ([#&#8203;505](python-poetry/poetry-core#505)).
-   Fix an issue where the name of the data folder in wheels was not normalized ([#&#8203;532](python-poetry/poetry-core#532)).
-   Fix an issue where the order of entries in the RECORD file was not deterministic ([#&#8203;545](python-poetry/poetry-core#545)).
-   Fix an issue where zero padding was not correctly handled in version comparisons ([#&#8203;540](python-poetry/poetry-core#540)).
-   Fix an issue where sdist builds did not support multiple READMEs ([#&#8203;486](python-poetry/poetry-core#486)).

##### poetry-plugin-export ([`^1.3.0`](https://github.com/python-poetry/poetry-plugin-export/releases/tag/1.3.0))

-   Fix an issue where the export failed if there was a circular dependency on the root package ([#&#8203;118](python-poetry/poetry-plugin-export#118)).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTIuNSIsInVwZGF0ZWRJblZlciI6IjM0LjE1Mi41In0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/655
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RunCommand incorrectly sets sys.argv for Poetry scripts
6 participants