From 423b0dd86f99fdf82915880d62c0ee33eade8337 Mon Sep 17 00:00:00 2001 From: Matt Rixman Date: Sun, 20 Aug 2023 14:58:33 -0600 Subject: [PATCH] pre-commit: fixing import bug for built-in hooks Pre-commit's "system" language is a sort of escape hach from the encapsulation which pre-commit typically provides. It just runs commands without thinking too hard about setting up or tearing down environments. But it is used for two things: user-provided hooks which reference things in calling environment, and pre-commit-provided hooks which live in the pre-commit package (I'm calling these "meta" hooks). The challenge becomes: which PATH and PYTHONPATH variables do we expose to pre-commit? If it's the calling environment, the meta hooks break, and if it's the packaged environment, the external-facing system hooks break. In order to make both of these work at the same time, this commit separates 'system' into two languages: 'system', and 'meta'. System hooks get access to the calling environment, and meta hooks get access to the packaged environment. --- pkgs/tools/misc/pre-commit/default.nix | 22 ++++- ...-hooks-use-calling-environment-paths.patch | 94 +++++++++++++++++++ pkgs/tools/misc/pre-commit/tests.nix | 37 ++++++++ 3 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 pkgs/tools/misc/pre-commit/system-hooks-use-calling-environment-paths.patch create mode 100644 pkgs/tools/misc/pre-commit/tests.nix diff --git a/pkgs/tools/misc/pre-commit/default.nix b/pkgs/tools/misc/pre-commit/default.nix index 0afa9180a47d5..45b01e4e65276 100644 --- a/pkgs/tools/misc/pre-commit/default.nix +++ b/pkgs/tools/misc/pre-commit/default.nix @@ -32,6 +32,7 @@ buildPythonApplication rec { patches = [ ./languages-use-the-hardcoded-path-to-python-binaries.patch + ./system-hooks-use-calling-environment-paths.patch ./hook-tmpl.patch ./pygrep-pythonpath.patch ]; @@ -104,12 +105,25 @@ buildPythonApplication rec { deactivate ''; - # Propagating dependencies leaks them through $PYTHONPATH which causes issues - # when used in nix-shell. + postFixup = '' + # Propagating inputs causes them to show up in $PYTHONPATH in a nix shell, + # this is problematic because they might conflict with the user's dependencies, + # so we remove them. rm $out/nix-support/propagated-build-inputs + + # the wrapper script modifies PATH and PYTHONPATH to point into the nix store + # this prevents 'system' type hooks from referencing the calling environment + # save untainted copies of that environment for such cases + sed -i '2i\export "OUTER_PYTHONPATH=$PYTHONPATH"' $out/bin/pre-commit + sed -i '2i\export "OUTER_PATH=$PATH"' $out/bin/pre-commit ''; + # Pre-commit also has built-in hooks which it needs to import from the nix store + # we hid them from user devshells by removing propated-build-inputs, but now we + # need to restore them for pre-commit's use (but only inside of the wrapper). + makeWrapperArgs = ''--set PYTHONPATH $PYTHONPATH''; + disabledTests = [ # ERROR: The install method you used for conda--probably either `pip install conda` # or `easy_install conda`--is not compatible with using conda as an application. @@ -178,8 +192,8 @@ buildPythonApplication rec { "pre_commit" ]; - passthru.tests.version = testers.testVersion { - package = pre-commit; + passthru.tests = callPackage ./tests.nix { + inherit git pre-commit; }; meta = with lib; { diff --git a/pkgs/tools/misc/pre-commit/system-hooks-use-calling-environment-paths.patch b/pkgs/tools/misc/pre-commit/system-hooks-use-calling-environment-paths.patch new file mode 100644 index 0000000000000..fe336810ee48f --- /dev/null +++ b/pkgs/tools/misc/pre-commit/system-hooks-use-calling-environment-paths.patch @@ -0,0 +1,94 @@ +diff --git a/pre_commit/all_languages.py b/pre_commit/all_languages.py +index 476bad9..742172a 100644 +--- a/pre_commit/all_languages.py ++++ b/pre_commit/all_languages.py +@@ -21,6 +21,7 @@ from pre_commit.languages import rust + from pre_commit.languages import script + from pre_commit.languages import swift + from pre_commit.languages import system ++from pre_commit.languages import meta + + + languages: dict[str, Language] = { +@@ -44,6 +45,7 @@ languages: dict[str, Language] = { + 'script': script, + 'swift': swift, + 'system': system, ++ 'meta': meta, + # TODO: fully deprecate `python_venv` + 'python_venv': python, + } +diff --git a/pre_commit/clientlib.py b/pre_commit/clientlib.py +index d0651ca..008307b 100644 +--- a/pre_commit/clientlib.py ++++ b/pre_commit/clientlib.py +@@ -265,8 +265,8 @@ META_HOOK_DICT = cfgv.Map( + 'Hook', 'id', + cfgv.Required('id', cfgv.check_string), + cfgv.Required('id', cfgv.check_one_of(tuple(k for k, _ in _meta))), +- # language must be system +- cfgv.Optional('language', cfgv.check_one_of({'system'}), 'system'), ++ # language must be meta ++ cfgv.Optional('language', cfgv.check_one_of({'meta'}), 'meta'), + # entry cannot be overridden + NotAllowed('entry', cfgv.check_any), + *( +diff --git a/pre_commit/lang_base.py b/pre_commit/lang_base.py +index 4a993ea..eba8f3e 100644 +--- a/pre_commit/lang_base.py ++++ b/pre_commit/lang_base.py +@@ -130,6 +130,27 @@ def no_install( + def no_env(prefix: Prefix, version: str) -> Generator[None, None, None]: + yield + ++@contextlib.contextmanager ++def outer_env(prefix: Prefix, version: str) -> Generator[None, None, None]: ++ ++ ++ # access the non-nix-modified environment ++ # which was captured pre-modifications by the wrapper script ++ # which was configured by the postFixup hook here: ++ # nixpkgs/pkgs/tools/misc/pre-commit/default.nix ++ backups = {} ++ for var in ["PATH", "PYTHONPATH"]: ++ val = os.environ.get(var) ++ outer_val = os.environ.get("OUTER_" + var) ++ if outer_val: ++ backups[var] = val ++ os.environ[var] = outer_val ++ ++ yield ++ ++ # restore the nix-modified environment ++ for k, v in backups.items(): ++ os.environ[k] = v + + def target_concurrency() -> int: + if 'PRE_COMMIT_NO_CONCURRENCY' in os.environ: +diff --git a/pre_commit/languages/meta.py b/pre_commit/languages/meta.py +new file mode 100644 +index 0000000..f6ad688 +--- /dev/null ++++ b/pre_commit/languages/meta.py +@@ -0,0 +1,10 @@ ++from __future__ import annotations ++ ++from pre_commit import lang_base ++ ++ENVIRONMENT_DIR = None ++get_default_version = lang_base.basic_get_default_version ++health_check = lang_base.basic_health_check ++install_environment = lang_base.no_install ++in_env = lang_base.no_env ++run_hook = lang_base.basic_run_hook +diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py +index f6ad688..d8237e7 100644 +--- a/pre_commit/languages/system.py ++++ b/pre_commit/languages/system.py +@@ -6,5 +6,5 @@ ENVIRONMENT_DIR = None + get_default_version = lang_base.basic_get_default_version + health_check = lang_base.basic_health_check + install_environment = lang_base.no_install +-in_env = lang_base.no_env ++in_env = lang_base.outer_env + run_hook = lang_base.basic_run_hook diff --git a/pkgs/tools/misc/pre-commit/tests.nix b/pkgs/tools/misc/pre-commit/tests.nix new file mode 100644 index 0000000000000..9aa83155c87d6 --- /dev/null +++ b/pkgs/tools/misc/pre-commit/tests.nix @@ -0,0 +1,37 @@ +{ git +, pre-commit +, runCommand +, testers +}: { + check-meta-hooks = runCommand "check-meta-hooks" { + nativeBuildInputs = [ git pre-commit ]; + } '' + cd "$(mktemp --directory)" + export HOME="$PWD" + cat << 'EOF' > .pre-commit-config.yaml + repos: + - repo: local + hooks: + - id: echo + name: echo + entry: echo + files: \.yaml$ + language: system + - repo: meta + hooks: + - id: check-hooks-apply + - id: check-useless-excludes + - id: identity + EOF + git config --global user.email "you@example.com" + git config --global user.name "Your Name" + git init --initial-branch=main + git add . + pre-commit run --all-files + touch $out + ''; + + version = testers.testVersion { + package = pre-commit; + }; +}