Fix file scripts not copied to bin on poetry install#10736
Open
Mr-Neutr0n wants to merge 4 commits into
Open
Conversation
When pyproject.toml defines scripts with type = "file" (e.g.,
my-command = { reference = "my-script.sh", type = "file" }), poetry
install now copies them into the virtualenv's bin/ directory, matching
the behavior of poetry build which already included them in wheels.
The EditableBuilder._add_scripts() method previously only handled
console_scripts entry points. This adds a second pass that iterates
over [tool.poetry.scripts], finds entries with type = "file", and
copies the referenced files to the scripts directory with proper
executable permissions.
Fixes python-poetry#10664
Reviewer's GuideExtends the editable builder to install [tool.poetry.scripts] entries defined as file-type scripts into the virtualenv’s bin directory, and adds tests/fixtures to verify both file and console scripts are correctly installed in editable builds. Sequence diagram for installing file-type scripts during editable buildssequenceDiagram
actor Developer
participant PoetryCLI
participant EditableBuilder
participant PoetryConfig
participant FileSystem
participant VirtualenvBin
Developer->>PoetryCLI: poetry install
PoetryCLI->>EditableBuilder: build_editable()
EditableBuilder->>EditableBuilder: _add_scripts()
Note over EditableBuilder: Existing behavior: add console scripts
EditableBuilder->>PoetryConfig: get console_scripts
PoetryConfig-->>EditableBuilder: console script entry points
EditableBuilder->>VirtualenvBin: write console script wrappers
Note over EditableBuilder: New behavior: handle file-type scripts
EditableBuilder->>PoetryConfig: local_config.get(scripts)
PoetryConfig-->>EditableBuilder: scripts mapping
loop for each script entry
EditableBuilder->>EditableBuilder: check specification type
alt type == file
EditableBuilder->>FileSystem: resolve source_path
alt source_path does not exist
EditableBuilder->>EditableBuilder: write_error_line(file missing)
else source_path is not a file
EditableBuilder->>EditableBuilder: write_error_line(not a file)
else valid file script
EditableBuilder->>VirtualenvBin: copy2(source_path, target)
EditableBuilder->>VirtualenvBin: chmod(target, 0o755)
end
else other types
EditableBuilder->>EditableBuilder: ignore in file pass
end
end
EditableBuilder-->>PoetryCLI: list of added scripts
PoetryCLI-->>Developer: editable installation complete
Class diagram for EditableBuilder handling of file-type scriptsclassDiagram
class EditableBuilder {
- Poetry _poetry
- Path _path
- IO _io
+ list~Path~ _add_scripts()
+ void _add_dist_info(list~Path~ added_files)
}
class Poetry {
+ dict local_config
}
class IO {
+ void write_error_line(str message)
}
class Path {
+ bool exists()
+ bool is_file()
+ Path joinpath(str name)
}
class shutil {
+ void copy2(Path source_path, Path target)
}
class ScriptsDirectory {
+ Path scripts_path
+ void chmod(int mode)
}
EditableBuilder --> Poetry : uses
EditableBuilder --> IO : uses
EditableBuilder --> Path : uses
EditableBuilder ..> shutil : uses
EditableBuilder --> ScriptsDirectory : copies_file_scripts_to
%% New logic inside _add_scripts
%% 1. Iterate self._poetry.local_config.get("scripts", {})
%% 2. For dict specifications with type == file
%% 3. Build source_path = self._path / reference
%% 4. Validate exists() and is_file()
%% 5. Copy via shutil.copy2 to scripts_path.joinpath(name)
%% 6. chmod(0o755) and append to added list
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new file-script handling in
_add_scripts()duplicates much ofWheelBuilder._copy_file_scripts; consider extracting a shared helper so editable and wheel builds use the same code path and stay in sync on future changes. - You call
shutil.copy2()and then unconditionallychmod(0o755)on the target; consider either relying on the copied mode or aligning the permission logic explicitly with the wheel builder to avoid subtle differences in behavior between install modes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new file-script handling in `_add_scripts()` duplicates much of `WheelBuilder._copy_file_scripts`; consider extracting a shared helper so editable and wheel builds use the same code path and stay in sync on future changes.
- You call `shutil.copy2()` and then unconditionally `chmod(0o755)` on the target; consider either relying on the copied mode or aligning the permission logic explicitly with the wheel builder to avoid subtle differences in behavior between install modes.
## Individual Comments
### Comment 1
<location> `src/poetry/masonry/builders/editable.py:219-220` </location>
<code_context>
+ for name, specification in self._poetry.local_config.get(
+ "scripts", {}
+ ).items():
+ if isinstance(specification, dict) and specification.get("type") == "file":
+ source = specification["reference"]
+ source_path = self._path / source
+
</code_context>
<issue_to_address>
**issue:** Accessing `specification["reference"]` directly can raise a `KeyError` for malformed configs.
Because this directly indexes `specification["reference"]`, a script entry like `type = "file"` without `reference` will raise `KeyError` and stop processing all scripts. Consider using `specification.get("reference")` and surfacing a validation-style error (similar to the missing-file case) to keep behavior consistent and more robust against malformed configs.
</issue_to_address>
### Comment 2
<location> `tests/masonry/builders/test_editable_builder.py:433-442` </location>
<code_context>
+def test_builder_installs_file_scripts(
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for failure modes of file scripts (missing or non-file references).
The new logic logs errors and skips scripts when `reference` does not exist or is not a file, but those branches aren’t tested. Please add tests for a missing path and for a directory `reference`, asserting that the script is not created in the venv `bin` directory and that the expected error is written to `io` (similar to `test_builder_catches_bad_scripts_*`).
Suggested implementation:
```python
def test_builder_installs_file_scripts(
file_scripts_poetry: Poetry,
tmp_path: Path,
) -> None:
env_manager = EnvManager(file_scripts_poetry)
venv_path = tmp_path / "venv"
env_manager.build_venv(venv_path)
tmp_venv = VirtualEnv(venv_path)
builder = EditableBuilder(file_scripts_poetry, tmp_venv, NullIO())
builder.build()
def test_builder_skips_missing_file_script(
fixture_dir: FixtureDirGetter,
tmp_path: Path,
) -> None:
poetry = Factory().create_poetry(fixture_dir("file_scripts_missing_reference_project"))
env_manager = EnvManager(poetry)
venv_path = tmp_path / "venv"
env_manager.build_venv(venv_path)
tmp_venv = VirtualEnv(venv_path)
io = BufferedIO()
builder = EditableBuilder(poetry, tmp_venv, io)
builder.build()
bin_dir = tmp_venv.path / ("Scripts" if os.name == "nt" else "bin")
# script for the missing reference must not be created
# (use the actual script name configured in the test fixture)
missing_script = bin_dir / "missing_file_script"
assert not missing_script.exists()
output = io.fetch_output()
# ensure the error about the missing file script reference is logged
# (keep the substring aligned with the actual error message)
assert "file script" in output
assert "does not exist" in output
def test_builder_skips_directory_file_script(
fixture_dir: FixtureDirGetter,
tmp_path: Path,
) -> None:
poetry = Factory().create_poetry(fixture_dir("file_scripts_directory_reference_project"))
env_manager = EnvManager(poetry)
venv_path = tmp_path / "venv"
env_manager.build_venv(venv_path)
tmp_venv = VirtualEnv(venv_path)
io = BufferedIO()
builder = EditableBuilder(poetry, tmp_venv, io)
builder.build()
bin_dir = tmp_venv.path / ("Scripts" if os.name == "nt" else "bin")
# script for the directory reference must not be created
# (use the actual script name configured in the test fixture)
directory_script = bin_dir / "directory_file_script"
assert not directory_script.exists()
output = io.fetch_output()
# ensure the error about the directory file script reference is logged
# (keep the substring aligned with the actual error message)
assert "file script" in output
assert "is not a file" in output
```
1. These tests assume a `BufferedIO` (or similar) class is already used in this test module (e.g. in `test_builder_catches_bad_scripts_*`). If the name differs, replace `BufferedIO` with the existing IO test helper and adjust how output is retrieved (`fetch_output()` vs other method).
2. Create two new fixture projects under the existing `tests/fixtures` hierarchy (or wherever `fixture_dir` resolves):
- `file_scripts_missing_reference_project`:
- A `pyproject.toml` that defines at least one valid file script and one file script whose `reference` points to a non-existent file. The failing script’s console entry name should be `missing_file_script` (or update the test to the chosen name).
- `file_scripts_directory_reference_project`:
- A `pyproject.toml` that defines at least one valid file script and one file script whose `reference` points to a directory (e.g. a folder in the project). The failing script’s console entry name should be `directory_file_script` (or update the test accordingly).
3. Update the asserted substrings (`"file script"`, `"does not exist"`, `"is not a file"`) to match the exact error messages emitted by the new logic that handles invalid file script references, following the pattern used in `test_builder_catches_bad_scripts_*`.
4. If your test suite has a helper to compute the virtualenv bin path instead of duplicating the `("Scripts" if os.name == "nt" else "bin")` logic, replace that line with the shared helper for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Use specification.get('reference') instead of specification['reference']
to avoid KeyError on malformed configs. Add a validation error message
when the reference field is missing.
Add tests covering three failure modes:
- File script with non-existent reference path
- File script referencing a directory instead of a file
- File script missing the reference field entirely
Two test bugs were causing the 11 failing pytest jobs on CI:
1. test_builder_skips_directory_file_script: the fixture pointed
'bin/some-directory' at a non-existent path, so the code took the
'does not exist' branch instead of the 'is not a file' branch the
test was asserting. Created the missing empty directory in the
fixture so the 'is not a file' branch is actually exercised.
2. test_builder_skips_file_script_missing_reference_field: the test
expected the 'missing reference' error to surface from
EditableBuilder, but the extra-scripts schema in poetry-core
requires both 'reference' and 'type' as mandatory fields. A
{ type = "file" } entry with no reference is rejected at config
validation time (Factory().create_poetry raises), so the
'if not source: continue' branch in EditableBuilder is dead code.
Rewrote the test to assert the schema-level rejection (which is
the actual user-facing behavior for malformed configs).
Local pytest run: 13/13 tests in test_editable_builder.py pass,
all 13 tests in tests/masonry/ pass.
Empty directories aren't tracked by git, but the test_builder_skips_directory_file_script fixture needs bin/some-directory to exist for the 'is not a file' branch to be exercised (without it, the test falls through to 'does not exist'). Adding .gitkeep so the path is present in the test fixture.
Author
|
@tlopex — the CI failures were test bugs, not code bugs. Fixed in Bug 1: Bug 2: Happy to also remove the Local pytest run: 13/13 tests in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #10664
When
pyproject.tomldefines scripts withtype = "file":poetry installdoes not copy the referenced file into the virtualenv'sbin/directory. Onlypoetry buildhandles these correctly (viaWheelBuilder._copy_file_scripts). Runningpoetry run my-commandafter install produces a warning about the script not being installed.Changes
src/poetry/masonry/builders/editable.py: After processing console script entry points in_add_scripts(), added a second pass that iterates over[tool.poetry.scripts]entries, identifies those withtype = "file", validates that the referenced file exists, and copies it to the virtualenv's script directory with executable permissions. This mirrors whatWheelBuilder._copy_file_scriptsalready does for built wheels.Test fixture + test: Added
tests/fixtures/file_scripts_project/with a project that has both a file script and a console entry point, and a test that verifies both are installed correctly during an editable build.Test plan
test_builder_installs_file_scriptsverifies file scripts are copied to the venv bin directory with correct content and permissionstype = "file"script, runpoetry install, confirm the script appears in the venv'sbin/Summary by Sourcery
Ensure editable installs copy file-based scripts into the virtualenv alongside console entry points.
Bug Fixes:
Tests: