Repository: pdm-project/pdm
Pull request #2766 (2024-04-03 12:05:04)

fix: refuse to run recursive composite scripts
## Pull Request Checklist

- [x] A news fragment is added in `news/` describing what is new.
- [x] Test cases added for changed code.

## Describe what you have changed in this PR.

If user defined a recursive composite script like this:

```toml
[tool.pdm.scripts]
first = { "composite" = ["first"] }
```

Then pdm will get into a resurive loop, resulting in a `RecursionError`:

```sh
> pdm run first
[RecursionError]: maximum recursion depth exceeded
WARNING: Add '-v' to see the detailed traceback
```

This PR adds a check for composite script, if one is already seen, then raise an error.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pdm-project/pdm/pull/2766?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 85.15%. Comparing base [(`f1f50a0`)](https://app.codecov.io/gh/pdm-project/pdm/commit/f1f50a0b745813a5d22d25537a7e1a851cdb04f7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project) to head [(`13a611a`)](https://app.codecov.io/gh/pdm-project/pdm/pull/2766?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2766      +/-   ##
==========================================
+ Coverage   85.10%   85.15%   +0.04%     
==========================================
  Files         106      106              
  Lines       10570    10576       +6     
  Branches     2292     2295       +3     
==========================================
+ Hits         8996     9006      +10     
+ Misses       1101     1099       -2     
+ Partials      473      471       -2     
```

| [Flag](https://app.codecov.io/gh/pdm-project/pdm/pull/2766/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/pdm-project/pdm/pull/2766/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project) | `84.93% <100.00%> (+0.04%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pdm-project/pdm/pull/2766?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pdm-project).


Patch:
diff --git a/news/2762.bugfix.md b/news/2762.bugfix.md
new file mode 100644
index 00000000..659c0fb8
--- /dev/null
+++ b/news/2762.bugfix.md
@@ -0,0 +1,1 @@
+Fix a race condition where pth files take effect when multiple packages are installed in parallel.
diff --git a/news/2766.bugfix.md b/news/2766.bugfix.md
new file mode 100644
index 00000000..449dd048
--- /dev/null
+++ b/news/2766.bugfix.md
@@ -0,0 +1,1 @@
+Refuse to run recursive composite scripts.
diff --git a/src/pdm/cli/commands/run.py b/src/pdm/cli/commands/run.py
index 506072eb..4a368125 100644
--- a/src/pdm/cli/commands/run.py
+++ b/src/pdm/cli/commands/run.py
@@ -230,7 +230,9 @@ class TaskRunner:
         signal.signal(signal.SIGINT, handle_int)
         return process.returncode
 
-    def run_task(self, task: Task, args: Sequence[str] = (), opts: TaskOptions | None = None) -> int:
+    def run_task(
+        self, task: Task, args: Sequence[str] = (), opts: TaskOptions | None = None, seen: set[str] | None = None
+    ) -> int:
         kind, _, value, options = task
         shell = False
         if kind == "cmd":
@@ -281,7 +283,7 @@ class TaskRunner:
                 split = shlex.split(script)
                 cmd = split[0]
                 subargs = split[1:] + ([] if should_interpolate else args)
-                code = self.run(cmd, subargs, options, chdir=True)
+                code = self.run(cmd, subargs, options, chdir=True, seen=seen)
                 if code != 0:
                     if not keep_going:
                         return code
@@ -289,18 +291,32 @@ class TaskRunner:
             return composite_code
         return self._run_process(args, chdir=True, shell=shell, **exec_opts(self.global_options, options, opts))
 
-    def run(self, command: str, args: list[str], opts: TaskOptions | None = None, chdir: bool = False) -> int:
+    def run(
+        self,
+        command: str,
+        args: list[str],
+        opts: TaskOptions | None = None,
+        chdir: bool = False,
+        seen: set[str] | None = None,
+    ) -> int:
         if command in self.hooks.skip:
             return 0
+        if seen is None:
+            seen = set()
         task = self.get_task(command)
         if task is not None:
+            if task.kind == "composite":
+                if command in seen:
+                    raise PdmUsageError(f"Script {command} is recursive.")
+                seen.add(command)
+
             self.hooks.try_emit("pre_script", script=command, args=args)
             pre_task = self.get_task(f"pre_{command}")
             if pre_task is not None and self.hooks.should_run(pre_task.name):
                 code = self.run_task(pre_task, opts=opts)
                 if code != 0:
                     return code
-            code = self.run_task(task, args, opts=opts)
+            code = self.run_task(task, args, opts=opts, seen=seen)
             if code != 0:
                 return code
             post_task = self.get_task(f"post_{command}")
diff --git a/src/pdm/installers/installers.py b/src/pdm/installers/installers.py
index a2143d41..341adabc 100644
--- a/src/pdm/installers/installers.py
+++ b/src/pdm/installers/installers.py
@@ -9,7 +9,7 @@ from typing import TYPE_CHECKING, Iterator
 
 from installer import install as _install
 from installer._core import _process_WHEEL_file
-from installer.destinations import SchemeDictionaryDestination
+from installer.destinations import SchemeDictionaryDestination, WheelDestination
 from installer.exceptions import InvalidWheelSource
 from installer.records import RecordEntry
 from installer.sources import WheelContentElement, WheelSource
@@ -97,12 +97,14 @@ class InstallDestination(SchemeDictionaryDestination):
         self,
         *args: Any,
         link_method: LinkMethod = "copy",
+        rename_pth: bool = False,
         **kwargs: Any,
     ) -> None:
         super().__init__(*args, **kwargs)
         self.link_method = link_method
+        self.rename_pth = rename_pth
 
-    def write_to_fs(self, scheme: Scheme, path: str | Path, stream: BinaryIO, is_executable: bool) -> RecordEntry:
+    def write_to_fs(self, scheme: Scheme, path: str, stream: BinaryIO, is_executable: bool) -> RecordEntry:
         from installer.records import Hash
         from installer.utils import copyfileobj_with_hashing, make_file_executable
 
@@ -112,6 +114,10 @@ class InstallDestination(SchemeDictionaryDestination):
 
         os.makedirs(os.path.dirname(target_path), exist_ok=True)
 
+        if self.rename_pth and target_path.endswith(".pth") and "/" not in path:
+            # Postpone the creation of pth files since it may cause race condition
+            # when multiple packages are installed at the same time.
+            target_path += ".pdmtmp"
         if self.link_method == "copy" or not hasattr(stream, "name"):
             with open(target_path, "wb") as f:
                 hash_, size = copyfileobj_with_hashing(stream, f, self.hash_algorithm)
@@ -147,6 +153,7 @@ def install_package(
     environment: BaseEnvironment,
     direct_url: dict[str, Any] | None = None,
     install_links: bool = True,
+    rename_pth: bool = False,
 ) -> str:
     """Only create .pth files referring to the cached package.
     If the cache doesn't exist, create one.
@@ -175,6 +182,7 @@ def install_package(
         interpreter=interpreter,
         script_kind=script_kind,
         link_method=link_method,
+        rename_pth=rename_pth,
     )
     source = PackageWheelSource(package)
     dist_info_dir = install(source, destination=destination, additional_metadata=additional_metadata)
@@ -184,7 +192,7 @@ def install_package(
 
 
 def install(
-    source: WheelSource, destination: InstallDestination, additional_metadata: dict[str, bytes] | None = None
+    source: WheelSource, destination: WheelDestination, additional_metadata: dict[str, bytes] | None = None
 ) -> str:
     """A lower level installation method that is copied from installer
     but is controlled by extra parameters.
@@ -198,7 +206,7 @@ def install(
 
 def install_wheel(wheel: str, environment: BaseEnvironment, direct_url: dict[str, Any] | None = None) -> str:
     """Install a wheel into the environment, return the .dist-info path"""
-    destination = InstallDestination(
+    destination = SchemeDictionaryDestination(
         scheme_dict=environment.get_paths(_get_dist_name(wheel)),
         interpreter=str(environment.interpreter.executable),
         script_kind=environment.script_kind,
diff --git a/src/pdm/installers/manager.py b/src/pdm/installers/manager.py
index fcbe43fd..2d75aa49 100644
--- a/src/pdm/installers/manager.py
+++ b/src/pdm/installers/manager.py
@@ -19,15 +19,22 @@ class InstallManager:
     # The packages below are needed to load paths and thus should not be cached.
     NO_CACHE_PACKAGES = ("editables",)
 
-    def __init__(self, environment: BaseEnvironment, *, use_install_cache: bool = False) -> None:
+    def __init__(
+        self, environment: BaseEnvironment, *, use_install_cache: bool = False, rename_pth: bool = False
+    ) -> None:
         self.environment = environment
         self.use_install_cache = use_install_cache
+        self.rename_pth = rename_pth
 
     def install(self, candidate: Candidate) -> Distribution:
         """Install a candidate into the environment, return the distribution"""
         prepared = candidate.prepare(self.environment)
         dist_info = install_package(
-            prepared.get_cached_package(), self.environment, prepared.direct_url(), self.use_install_cache
+            prepared.get_cached_package(),
+            self.environment,
+            direct_url=prepared.direct_url(),
+            install_links=self.use_install_cache,
+            rename_pth=self.rename_pth,
         )
         return Distribution.at(dist_info)
 
diff --git a/src/pdm/installers/synchronizers.py b/src/pdm/installers/synchronizers.py
index b7ee5629..8da3cb04 100644
--- a/src/pdm/installers/synchronizers.py
+++ b/src/pdm/installers/synchronizers.py
@@ -122,7 +122,7 @@ class BaseSynchronizer:
         self.no_editable = no_editable
         self.install_self = install_self
         if use_install_cache is None:
-            use_install_cache = environment.project.config["install.cache"]
+            use_install_cache = bool(environment.project.config["install.cache"])
         self.use_install_cache = use_install_cache
         self.reinstall = reinstall
         self.only_keep = only_keep
@@ -177,7 +177,7 @@ class BaseSynchronizer:
 
     def get_manager(self) -> InstallManager:
         return self.environment.project.core.install_manager_class(
-            self.environment, use_install_cache=self.use_install_cache
+            self.environment, use_install_cache=self.use_install_cache, rename_pth=True
         )
 
     @property
@@ -378,6 +378,19 @@ class Synchronizer(BaseSynchronizer):
         else:
             self.ui.echo("All packages are synced to date, nothing to do.")
 
+    def _fix_pth_files(self) -> None:
+        """Remove the .pdmtmp suffix from the installed packages"""
+        from pathlib import Path
+
+        lib_paths = self.environment.get_paths()
+        for scheme in ["purelib", "platlib"]:
+            for path in list(Path(lib_paths[scheme]).iterdir()):
+                if path.suffix == ".pdmtmp":
+                    target_path = path.with_suffix("")
+                    if target_path.exists():
+                        target_path.unlink()
+                    path.rename(target_path)
+
     def synchronize(self) -> None:
         to_add, to_update, to_remove = self.compare_with_working_set()
         to_do = {"remove": to_remove, "update": to_update, "add": to_add}
@@ -443,21 +456,25 @@ class Synchronizer(BaseSynchronizer):
                 state.errors.clear()
                 live.console.print("Retry failed jobs")
 
-            if state.errors:
-                if self.ui.verbosity < termui.Verbosity.DETAIL:
-                    live.console.print("\n[error]ERRORS[/]:")
-                    live.console.print("".join(state.errors), end="")
-                raise InstallationError("Some package operations are not complete yet")
-
-            if self.install_self:
-                self_key = self.self_key
-                assert self_key
-                self.candidates[self_key] = self.self_candidate
-                word = "a" if self.no_editable else "an editable"
-                live.console.print(f"Installing the project as {word} package...")
-                if self_key in self.working_set:
-                    self.update_candidate(self_key, progress)
-                else:
-                    self.install_candidate(self_key, progress)
+            try:
+                if state.errors:
+                    if self.ui.verbosity < termui.Verbosity.DETAIL:
+                        live.console.print("\n[error]ERRORS[/]:")
+                        live.console.print("".join(state.errors), end="")
+                    raise InstallationError("Some package operations are not complete yet")
+
+                if self.install_self:
+                    self_key = self.self_key
+                    assert self_key
+                    self.candidates[self_key] = self.self_candidate
+                    word = "a" if self.no_editable else "an editable"
+                    live.console.print(f"Installing the project as {word} package...")
+                    if self_key in self.working_set:
+                        self.update_candidate(self_key, progress)
+                    else:
+                        self.install_candidate(self_key, progress)
 
-            live.console.print(f"\n{termui.Emoji.POPPER} All complete!")
+                live.console.print(f"\n{termui.Emoji.POPPER} All complete!")
+            finally:
+                # Now we remove the .pdmtmp suffix from the installed packages
+                self._fix_pth_files()


Test patch:
diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py
index 6e1e4c03..1326cd13 100644
--- a/tests/cli/test_run.py
+++ b/tests/cli/test_run.py
@@ -665,6 +665,26 @@ def test_composite_fail_on_first_missing_task(project, pdm, capfd, _echo):
     assert "Second CALLED" not in out
 
 
+def test_composite_fails_on_recursive_script(project, pdm):
+    project.pyproject.settings["scripts"] = {
+        "first": {"composite": ["first"]},
+        "second": {"composite": ["third"]},
+        "third": {"composite": ["second"]},
+        "forth": {"composite": ["python -V", "python -V"]},
+    }
+    project.pyproject.write()
+    result = pdm(["run", "first"], obj=project)
+    assert result.exit_code == 1
+    assert "Script first is recursive" in result.stderr
+
+    result = pdm(["run", "second"], obj=project)
+    assert result.exit_code == 1
+    assert "Script second is recursive" in result.stderr
+
+    result = pdm(["run", "forth"], obj=project)
+    assert result.exit_code == 0
+
+
 def test_composite_runs_all_hooks(project, pdm, capfd, _echo):
     project.pyproject.settings["scripts"] = {
         "test": {"composite": ["first", "second"]},