From 2651ddc7b0788932df9da7416ccd1618d76c11fe Mon Sep 17 00:00:00 2001 From: phaer Date: Fri, 26 Jan 2024 18:24:26 +0100 Subject: [PATCH 1/4] python/catch_conflicts: scan $out, not sys.path This changes the non-legacy version of pythonCatchConflictsHook to recursively scan the output of the target derivation as well as its propagatedBuildInputs for duplicate dependencies. Previously, we did scan sys.path but did prove problematic as it produced false positives i.e. when build-time dependencies of hooks - such as setuptools in pythonCatchConflictsHook itself - where mistakenly flagged as duplicates; even though the are not included in the outputs of the target dervation. As all python runtime-dependencies are currently passed via propagatedBuildInputs in nixpkgs, scanning that plus site-packages seems sufficient to catch all conflicts that matter at runtime and less likely to produce false positives. The legacyHook in catch_conflicts_py2.py needs to be migrated as well, if it's still needed. --- .../python/catch_conflicts/catch_conflicts.py | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index d5c99e64751c..9339c62a6399 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -2,21 +2,35 @@ from importlib.metadata import PathDistribution from pathlib import Path import collections import sys +import os do_abort = False packages = collections.defaultdict(list) +out_path = Path(os.getenv("out")) +version = sys.version_info +site_packages_path = f'lib/python{version[0]}.{version[1]}/site-packages' -for path in sys.path: - for dist_info in Path(path).glob("*.dist-info"): - dist = PathDistribution(dist_info) +def find_packages(store_path, site_packages_path): + site_packages = (store_path / site_packages_path) + propagated_build_inputs = (store_path / "nix-support/propagated-build-inputs") + if site_packages.exists(): + for dist_info in site_packages.glob("*.dist-info"): + dist = PathDistribution(dist_info) + packages[dist._normalized_name].append( + f"{dist._normalized_name} {dist.version} ({dist._path})" + ) - packages[dist._normalized_name].append( - f"{dist._normalized_name} {dist.version} ({dist._path})" - ) + if propagated_build_inputs.exists(): + with open(propagated_build_inputs, "r") as f: + build_inputs = f.read().strip().split(" ") + for build_input in build_inputs: + find_packages(Path(build_input), site_packages_path) +find_packages(out_path, site_packages_path) + for name, duplicates in packages.items(): if len(duplicates) > 1: do_abort = True From 0cbd114d41b619aa611ab158528f551b14b8cd9c Mon Sep 17 00:00:00 2001 From: DavHau Date: Sat, 10 Feb 2024 18:27:32 +0700 Subject: [PATCH 2/4] pythonCatchConflictsHook: improve and add tests --- .../python/catch_conflicts/catch_conflicts.py | 70 ++++++--- .../interpreters/python/hooks/default.nix | 4 + .../python-catch-conflicts-hook-tests.nix | 137 ++++++++++++++++++ 3 files changed, 192 insertions(+), 19 deletions(-) create mode 100644 pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index 9339c62a6399..319b92bee3bc 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -5,39 +5,71 @@ import sys import os -do_abort = False -packages = collections.defaultdict(list) -out_path = Path(os.getenv("out")) -version = sys.version_info -site_packages_path = f'lib/python{version[0]}.{version[1]}/site-packages' +do_abort: bool = False +packages: dict[str, dict[str, list[dict[str, list[str]]]]] = collections.defaultdict(list) +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' -def find_packages(store_path, site_packages_path): - site_packages = (store_path / site_packages_path) - propagated_build_inputs = (store_path / "nix-support/propagated-build-inputs") +# pretty print a package +def describe_package(dist: PathDistribution) -> str: + return f"{dist._normalized_name} {dist.version} ({dist._path})" + + +# pretty print a list of parents (dependency chain) +def describe_parents(parents: list[str]) -> str: + if not parents: + return "" + return \ + f" dependency chain:\n " \ + + str(f"\n ...depending on: ".join(parents)) + + +# 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( + version=version, + parents=parents, + )) + + +# transitively discover python dependencies and store them in 'packages' +def find_packages(store_path: Path, site_packages_path: str, parents: list[str]) -> None: + site_packages: Path = (store_path / site_packages_path) + propagated_build_inputs: Path = (store_path / "nix-support/propagated-build-inputs") + + # add the current package to the list if site_packages.exists(): for dist_info in site_packages.glob("*.dist-info"): - dist = PathDistribution(dist_info) - packages[dist._normalized_name].append( - f"{dist._normalized_name} {dist.version} ({dist._path})" - ) + dist: PathDistribution = PathDistribution(dist_info) + add_entry(dist._normalized_name, dist.version, store_path, parents) + # recursively add dependencies if propagated_build_inputs.exists(): with open(propagated_build_inputs, "r") as f: - build_inputs = f.read().strip().split(" ") + build_inputs: list[str] = f.read().strip().split(" ") for build_input in build_inputs: - find_packages(Path(build_input), site_packages_path) + find_packages(Path(build_input), site_packages_path, parents + [build_input]) -find_packages(out_path, site_packages_path) +find_packages(out_path, site_packages_path, [f"this derivation: {out_path}"]) -for name, duplicates in packages.items(): - if len(duplicates) > 1: +# print all duplicates +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 duplicate in duplicates: - print(f"\t{duplicate}") + for store_path, candidates in store_paths.items(): + for candidate in candidates: + print(f" {name} {candidate['version']} ({store_path})") + print(describe_parents(candidate['parents'])) +# fail if duplicates were found if do_abort: print("") print( diff --git a/pkgs/development/interpreters/python/hooks/default.nix b/pkgs/development/interpreters/python/hooks/default.nix index 0557c62eeff4..e6d093a10fb7 100644 --- a/pkgs/development/interpreters/python/hooks/default.nix +++ b/pkgs/development/interpreters/python/hooks/default.nix @@ -118,6 +118,10 @@ in { } // lib.optionalAttrs useLegacyHook { inherit setuptools; }; + passthru.tests = import ./python-catch-conflicts-hook-tests.nix { + inherit pythonOnBuildForHost runCommand; + inherit (pkgs) coreutils gnugrep writeShellScript; + }; } ./python-catch-conflicts-hook.sh) {}; pythonImportsCheckHook = callPackage ({ makePythonHook }: 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 new file mode 100644 index 000000000000..f3d9235799e0 --- /dev/null +++ b/pkgs/development/interpreters/python/hooks/python-catch-conflicts-hook-tests.nix @@ -0,0 +1,137 @@ +{ pythonOnBuildForHost, runCommand, writeShellScript, coreutils, gnugrep }: let + + pythonPkgs = pythonOnBuildForHost.pkgs; + + ### UTILITIES + + # customize a package so that its store paths differs + customize = pkg: pkg.overrideAttrs { some_modification = true; }; + + # generates minimal pyproject.toml + pyprojectToml = pname: builtins.toFile "pyproject.toml" '' + [project] + name = "${pname}" + version = "1.0.0" + ''; + + # generates source for a python project + projectSource = pname: runCommand "my-project-source" {} '' + mkdir -p $out/src + cp ${pyprojectToml pname} $out/pyproject.toml + touch $out/src/__init__.py + ''; + + # helper to reduce boilerplate + generatePythonPackage = args: pythonPkgs.buildPythonPackage ( + { + version = "1.0.0"; + src = runCommand "my-project-source" {} '' + mkdir -p $out/src + cp ${pyprojectToml args.pname} $out/pyproject.toml + touch $out/src/__init__.py + ''; + pyproject = true; + catchConflicts = true; + buildInputs = [ pythonPkgs.setuptools ]; + } + // args + ); + + # in order to test for a failing build, wrap it in a shell script + expectFailure = build: errorMsg: build.overrideDerivation (old: { + builder = writeShellScript "test-for-failure" '' + export PATH=${coreutils}/bin:${gnugrep}/bin:$PATH + ${old.builder} "$@" > ./log 2>&1 + status=$? + cat ./log + if [ $status -eq 0 ] || ! grep -q "${errorMsg}" ./log; then + echo "The build should have failed with '${errorMsg}', but it didn't" + exit 1 + else + echo "The build failed as expected with: ${errorMsg}" + mkdir -p $out + fi + ''; + }); +in { + + ### TEST CASES + + # Test case which must not trigger any conflicts. + # This derivation has runtime dependencies on custom versions of multiple build tools. + # This scenario is relevant for lang2nix tools which do not override the nixpkgs fix-point. + # see https://github.com/NixOS/nixpkgs/issues/283695 + ignores-build-time-deps = + generatePythonPackage { + pname = "ignores-build-time-deps"; + buildInputs = [ + pythonPkgs.build + pythonPkgs.packaging + pythonPkgs.setuptools + pythonPkgs.wheel + ]; + propagatedBuildInputs = [ + # Add customized versions of build tools as runtime deps + (customize pythonPkgs.packaging) + (customize pythonPkgs.setuptools) + (customize pythonPkgs.wheel) + ]; + }; + + # Simplest test case that should trigger a conflict + catches-simple-conflict = let + # this build must fail due to conflicts + package = pythonPkgs.buildPythonPackage rec { + pname = "catches-simple-conflict"; + version = "0.0.0"; + src = projectSource pname; + pyproject = true; + catchConflicts = true; + buildInputs = [ + pythonPkgs.setuptools + ]; + # depend on two different versions of packaging + # (an actual runtime dependency conflict) + propagatedBuildInputs = [ + pythonPkgs.packaging + (customize pythonPkgs.packaging) + ]; + }; + in + expectFailure package "Found duplicated packages in closure for dependency 'packaging'"; + + + /* + More complex test case with a transitive conflict + + Test sets up this dependency tree: + + toplevel + ├── dep1 + │ └── leaf + └── dep2 + └── leaf (customized version -> conflicting) + */ + catches-transitive-conflict = let + # package depending on both dependency1 and dependency2 + toplevel = generatePythonPackage { + pname = "catches-transitive-conflict"; + propagatedBuildInputs = [ dep1 dep2 ]; + }; + # dep1 package depending on leaf + dep1 = generatePythonPackage { + pname = "dependency1"; + propagatedBuildInputs = [ leaf ]; + }; + # dep2 package depending on conflicting version of leaf + dep2 = generatePythonPackage { + pname = "dependency2"; + propagatedBuildInputs = [ (customize leaf) ]; + }; + # some leaf package + leaf = generatePythonPackage { + pname = "leaf"; + }; + in + expectFailure toplevel "Found duplicated packages in closure for dependency 'leaf'"; +} From ffa815958e61aa978c715eb6e279c290617a6615 Mon Sep 17 00:00:00 2001 From: DavHau Date: Sun, 11 Feb 2024 21:10:07 +0700 Subject: [PATCH 3/4] pythonCatchConflictsHook: make compatible to all python 3 versions --- .../python/catch_conflicts/catch_conflicts.py | 23 +++++++++++-------- .../interpreters/python/hooks/default.nix | 2 +- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py index 319b92bee3bc..d4219192790b 100644 --- a/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py +++ b/pkgs/development/interpreters/python/catch_conflicts/catch_conflicts.py @@ -3,22 +3,25 @@ from pathlib import Path import collections import sys import os - - +from typing import Dict, List, Tuple do_abort: bool = False -packages: dict[str, dict[str, list[dict[str, list[str]]]]] = collections.defaultdict(list) +packages: Dict[str, Dict[str, List[Dict[str, List[str]]]]] = collections.defaultdict(list) out_path: Path = Path(os.getenv("out")) -version: tuple[int, int] = sys.version_info +version: Tuple[int, int] = sys.version_info site_packages_path: str = f'lib/python{version[0]}.{version[1]}/site-packages' +def get_name(dist: PathDistribution) -> str: + return dist.metadata['name'].lower().replace('-', '_') + + # pretty print a package def describe_package(dist: PathDistribution) -> str: - return f"{dist._normalized_name} {dist.version} ({dist._path})" + return f"{get_name(dist)} {dist.version} ({dist._path})" # pretty print a list of parents (dependency chain) -def describe_parents(parents: list[str]) -> str: +def describe_parents(parents: List[str]) -> str: if not parents: return "" return \ @@ -27,7 +30,7 @@ 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: +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]: @@ -39,7 +42,7 @@ def add_entry(name: str, version: str, store_path: str, parents: list[str]) -> N # transitively discover python dependencies and store them in 'packages' -def find_packages(store_path: Path, site_packages_path: str, parents: list[str]) -> None: +def find_packages(store_path: Path, site_packages_path: str, parents: List[str]) -> None: site_packages: Path = (store_path / site_packages_path) propagated_build_inputs: Path = (store_path / "nix-support/propagated-build-inputs") @@ -47,12 +50,12 @@ def find_packages(store_path: Path, site_packages_path: str, parents: list[str]) if site_packages.exists(): for dist_info in site_packages.glob("*.dist-info"): dist: PathDistribution = PathDistribution(dist_info) - add_entry(dist._normalized_name, dist.version, store_path, parents) + add_entry(get_name(dist), dist.version, store_path, parents) # 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().strip().split(" ") for build_input in build_inputs: find_packages(Path(build_input), site_packages_path, parents + [build_input]) diff --git a/pkgs/development/interpreters/python/hooks/default.nix b/pkgs/development/interpreters/python/hooks/default.nix index e6d093a10fb7..39a958b69ab3 100644 --- a/pkgs/development/interpreters/python/hooks/default.nix +++ b/pkgs/development/interpreters/python/hooks/default.nix @@ -108,7 +108,7 @@ in { makePythonHook { name = "python-catch-conflicts-hook"; substitutions = let - useLegacyHook = lib.versionOlder python.pythonVersion "3.10"; + useLegacyHook = lib.versionOlder python.pythonVersion "3"; in { inherit pythonInterpreter pythonSitePackages; catchConflicts = if useLegacyHook then From a299915fff009a44db79e093db1a3f68e4fea5c7 Mon Sep 17 00:00:00 2001 From: DavHau Date: Mon, 12 Feb 2024 19:30:46 +0700 Subject: [PATCH 4/4] pythonCatchConflictsHook: improve docs --- doc/languages-frameworks/python.section.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/languages-frameworks/python.section.md b/doc/languages-frameworks/python.section.md index 0849aacdf166..7e4f34e76875 100644 --- a/doc/languages-frameworks/python.section.md +++ b/doc/languages-frameworks/python.section.md @@ -469,7 +469,7 @@ are used in [`buildPythonPackage`](#buildpythonpackage-function). be added as `nativeBuildInput`. - `pipInstallHook` to install wheels. - `pytestCheckHook` to run tests with `pytest`. See [example usage](#using-pytestcheckhook). -- `pythonCatchConflictsHook` to check whether a Python package is not already existing. +- `pythonCatchConflictsHook` to fail if the package depends on two different versions of the same dependency. - `pythonImportsCheckHook` to check whether importing the listed modules works. - `pythonRelaxDepsHook` will relax Python dependencies restrictions for the package. See [example usage](#using-pythonrelaxdepshook).