From 21833407b4364b7266a1b234d06182f655ec6cb7 Mon Sep 17 00:00:00 2001 From: Ben Wolsieffer Date: Sat, 20 Apr 2024 15:25:07 -0400 Subject: [PATCH 1/4] pythonCatchConflictsHook: add test for multiple dependency chains Add a test where a conflicting package can be found at the end of multiple dependency chains. This is far too simple an example to demonstrate the ill effects of exponential time complexity, but does serve to demonstrate how the error output changes when each path is only visited once. --- .../python-catch-conflicts-hook-tests.nix | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix b/pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix index cba1034e0963..3890df40cb74 100644 --- a/pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix +++ b/pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix @@ -143,4 +143,46 @@ in { }; in expectFailure toplevel "Found duplicated packages in closure for dependency 'leaf'"; + + /* + Transitive conflict with multiple dependency chains leading to the + conflicting package. + + Test sets up this dependency tree: + + toplevel + ├── dep1 + │ └── leaf + ├── dep2 + │ └── leaf + └── dep3 + └── leaf (customized version -> conflicting) + */ + catches-conflict-multiple-chains = let + # package depending on dependency1, dependency2 and dependency3 + toplevel = generatePythonPackage { + pname = "catches-conflict-multiple-chains"; + propagatedBuildInputs = [ dep1 dep2 dep3 ]; + }; + # dep1 package depending on leaf + dep1 = generatePythonPackage { + pname = "dependency1"; + propagatedBuildInputs = [ leaf ]; + }; + # dep2 package depending on leaf + dep2 = generatePythonPackage { + pname = "dependency2"; + propagatedBuildInputs = [ leaf ]; + }; + # dep3 package depending on conflicting version of leaf + dep3 = generatePythonPackage { + pname = "dependency3"; + propagatedBuildInputs = [ (customize leaf) ]; + }; + # some leaf package + leaf = generatePythonPackage { + pname = "leaf"; + }; + in + expectFailure toplevel "Found duplicated packages in closure for dependency 'leaf'"; } From a25e43e6d7089d4655f945c9874bd6756fbb5c90 Mon Sep 17 00:00:00 2001 From: Ben Wolsieffer Date: Sat, 20 Apr 2024 14:47:00 -0400 Subject: [PATCH 2/4] pythonCatchConflictsHook: prevent exponential worst-case The hook performs a depth first search on the graph defined by propagatedBuildInputs. This traverses all paths through the graph, except for any cycles. In the worst case with a highly connected graph, this search can take exponential time. In practice, this means that in cases with long dependency chains and multiple packages depending on the same package, the hook can take several hours to run. Avoid this problem by keeping track of already visited paths and only visiting each path once. This makes the search complete in linear time. The visible effect of this change is that, if a conflict is found, only one dependency chain that leads to the conflicting package is printed, rather than all the possible dependency chains. --- .../python/catch_conflicts/catch_conflicts.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index ad679d9f9f99..648fec015903 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -3,9 +3,10 @@ from pathlib import Path import collections import sys import os -from typing import Dict, List, Tuple +from typing import Dict, List, Set, Tuple do_abort: bool = False packages: Dict[str, Dict[str, List[Dict[str, List[str]]]]] = collections.defaultdict(list) +found_paths: Set[Path] = set() out_path: Path = Path(os.getenv("out")) version: Tuple[int, int] = sys.version_info site_packages_path: str = f'lib/python{version[0]}.{version[1]}/site-packages' @@ -46,6 +47,12 @@ def find_packages(store_path: Path, site_packages_path: str, parents: List[str]) site_packages: Path = (store_path / site_packages_path) propagated_build_inputs: Path = (store_path / "nix-support/propagated-build-inputs") + # only visit each path once, to avoid exponential complexity with highly + # connected dependency graphs + if store_path in found_paths: + return + found_paths.add(store_path) + # add the current package to the list if site_packages.exists(): for dist_info in site_packages.glob("*.dist-info"): From 7f14d675a7e1e2bdef3c9c1b6f83c42474715e51 Mon Sep 17 00:00:00 2001 From: Ben Wolsieffer Date: Sat, 20 Apr 2024 15:16:45 -0400 Subject: [PATCH 3/4] pythonCatchConflictsHook: cleanup due to visiting each path once Now that we only visit each path once, a few things can be simplified. We no longer have to keep a list of different dependency chains leading to a package, since only one chain will ever be found. Also, the already visited check also takes care of cycles, so the other cycle check can be removed. --- .../python/catch_conflicts/catch_conflicts.py | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index 648fec015903..cb2bd56c71d5 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -5,7 +5,7 @@ import sys import os from typing import Dict, List, Set, Tuple do_abort: bool = False -packages: Dict[str, Dict[str, List[Dict[str, List[str]]]]] = collections.defaultdict(list) +packages: Dict[str, Dict[str, Dict[str, List[str]]]] = collections.defaultdict(dict) found_paths: Set[Path] = set() out_path: Path = Path(os.getenv("out")) version: Tuple[int, int] = sys.version_info @@ -32,14 +32,10 @@ def describe_parents(parents: List[str]) -> str: # inserts an entry into 'packages' def add_entry(name: str, version: str, store_path: str, parents: List[str]) -> None: - if name not in packages: - packages[name] = {} - if store_path not in packages[name]: - packages[name][store_path] = [] - packages[name][store_path].append(dict( + packages[name][store_path] = dict( version=version, parents=parents, - )) + ) # transitively discover python dependencies and store them in 'packages' @@ -64,8 +60,7 @@ def find_packages(store_path: Path, site_packages_path: str, parents: List[str]) with open(propagated_build_inputs, "r") as f: build_inputs: List[str] = f.read().strip().split(" ") for build_input in build_inputs: - if build_input not in parents: - find_packages(Path(build_input), site_packages_path, parents + [build_input]) + find_packages(Path(build_input), site_packages_path, parents + [build_input]) find_packages(out_path, site_packages_path, [f"this derivation: {out_path}"]) @@ -75,10 +70,9 @@ for name, store_paths in packages.items(): if len(store_paths) > 1: do_abort = True print("Found duplicated packages in closure for dependency '{}': ".format(name)) - for store_path, candidates in store_paths.items(): - for candidate in candidates: - print(f" {name} {candidate['version']} ({store_path})") - print(describe_parents(candidate['parents'])) + for store_path, candidate in store_paths.items(): + print(f" {name} {candidate['version']} ({store_path})") + print(describe_parents(candidate['parents'])) # fail if duplicates were found if do_abort: From f9de72f24776538e7e2243f54ab46f3e3a921ab5 Mon Sep 17 00:00:00 2001 From: Ben Wolsieffer Date: Sat, 20 Apr 2024 15:28:11 -0400 Subject: [PATCH 4/4] pythonCatchConflictsHook: split propagated-build-inputs on runs of whitespace Currently, nix-support/propagated-build-inputs is parsed by splitting on a single space. This means that if this file contains multiple spaces separating two paths, the build_inputs list will end up containing an empty string. Instead, call split() with no arguments, which splits on runs of whitespace and also ignores whitespace at the beginning and end of the string, eliminating the need for strip(). --- .../interpreters/python/catch_conflicts/catch_conflicts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index cb2bd56c71d5..4713cfb7026e 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -58,7 +58,7 @@ def find_packages(store_path: Path, site_packages_path: str, parents: List[str]) # recursively add dependencies if propagated_build_inputs.exists(): with open(propagated_build_inputs, "r") as f: - build_inputs: List[str] = f.read().strip().split(" ") + build_inputs: List[str] = f.read().split() for build_input in build_inputs: find_packages(Path(build_input), site_packages_path, parents + [build_input])