Skip to content

Activate virtualenv in hook template #1522

Closed
Closed
Activate virtualenv in hook template#1522
Labels
question

Description

@cjolowicz
cjolowicz
opened on Jun 20, 2020

Would it be possible for pre-commit's hook template to modify PATH before invoking pre-commit, when pre-commit is running in a virtualenv? This would allow pre-commit to run Python hooks installed into the virtualenv.

Currently, if pre-commit is installed in a virtualenv, git hooks invoke pre-commit without activating the virtualenv. More specifically, while the Python interpreter running pre-commit uses the virtualenv, the PATH and VIRTUAL_ENV environment variables are not modified to reflect the virtualenv. It is thus the responsibility of the user to activate the virtualenv in the shell invoking git.

As explained below (see Rationale), I believe allowing pre-commit to run hooks in the same virtualenv can offer significant advantages to some projects. This could be achieved by inserting three lines into the hook template (see Implementation Notes).

If you are interested in this feature, I would be happy to provide a PR with tests and documentation. Do you think this is a sensible idea? If so, should this be done always, or hidden behind a feature flag such as an option to the pre-commit install command? I would love to hear your thoughts!

Thanks for your work creating and maintaining so many awesome tools!

Rationale

For certain Python projects, it is beneficial to use dedicated tools to manage the environments and dependencies of Python-language pre-commit hooks, rather than pre-commit itself. Here are some of the advantages this can offer:

  • Project dependencies can be declared in one place, and managed by one tool.
  • Automated dependency upgrades (e.g. Dependabot) for hooks become possible.
  • Additional dependencies (e.g. Flake8 extensions) are visible to the dependency management tool. (pre-commit autoupdate is centered around the concept of Git repositories; it is -- and should be -- agnostic of language-level concepts such as PyPI versions, does not perform dependency resolution, and does not inspect additional dependencies.)
  • Subdependencies (dependencies of dependencies) can be locked automatically, making checks more repeatable and deterministic.

pre-commit already supports this use case via repository-local system hooks. However, this requires that the virtualenv be activated in the calling shell. In some situations, explicitly activating the environment is not practical. Consider these examples:

Example 1: pre-commit and Nox

When running pre-commit in a Nox session, Python hooks can be installed into the environment associated with the Nox session, alongside pre-commit itself. This works fine when pre-commit is invoked by Nox: Nox activates the virtual environment for the pre-commit session, so the hooks are located on PATH. However, when pre-commit is invoked from git hooks, the virtualenv is not active, so pre-commit fails to locate the hooks.

Example 2: pre-commit and Poetry

Another example is the case of adding pre-commit and the Python hooks as dependencies to a Poetry-managed project. When invoking pre-commit with Poetry (poetry run pre-commit), the environment is activated by Poetry. But when pre-commit is invoked from git hooks, the hooks are no longer on PATH.

Example 3: pre-commit, Nox, and Poetry

For a more concrete example of an attempt to integrate pre-commit with both Nox and Poetry, see this tree, specifically the noxfile.py and .pre-commit-config.yaml files in the {{cookiecutter.project_name}} directory. The associated PR is cjolowicz/cookiecutter-hypermodern-python#397.

Implementation Notes

Virtualenv activation could be achieved by inserting three lines into the hook template:

diff --git a/pre_commit/resources/hook-tmpl b/pre_commit/resources/hook-tmpl
index 299144e..19fc7ea 100755
--- a/pre_commit/resources/hook-tmpl
+++ b/pre_commit/resources/hook-tmpl
@@ -26,6 +26,9 @@ ARGS.extend(sys.argv[1:])

 DNE = '`pre-commit` not found.  Did you forget to activate your virtualenv?'
 if os.access(INSTALL_PYTHON, os.X_OK):
+    bindir = os.path.dirname(INSTALL_PYTHON)
+    if os.path.isfile(os.path.join(os.path.dirname(bindir), 'pyvenv.cfg')):
+        os.environ['PATH'] = os.pathsep.join((bindir, os.environ.get('PATH', '')))
     CMD = [INSTALL_PYTHON, '-mpre_commit']
 elif which('pre-commit'):
     CMD = ['pre-commit']

The hook template checks if the Python interpreter used when installing pre-commit (INSTALL_PYTHON) is located in a virtualenv, by checking for the file pyvenv.cfg in the directory above the executable. If this is the case, the directory containing the Python interpreter is prepended to PATH. In a typical virtualenv, this is bin on UNIX-like systems, and Scripts on Windows. To keep the script short, the process environment is modified in place, rather than copied and passed to os.execvpe or subprocess.Popen.

Activity

  1. asottile commented on Jun 20, 2020

    @asottile
    asottile
    on Jun 20, 2020
    Member

    this is a duplicate of several issues, I believe this one is the most recent one

    the problems outlined in there are roughly:

    • language: system is at your own support
    • unmanaged environments are just that, unamanged
    • though pre-commit is written in python, it is not a python-specific tool
    • this would create a difference between pre-commit run and git commit which is not ok
  2. asottile
    closed this as completedon Jun 20, 2020
  3. asottile
    added
    question
    on Jun 20, 2020

  4. cjolowicz commented on Jun 29, 2020

    @cjolowicz
    cjolowicz
    on Jun 29, 2020
    Author

    Thanks for your reply!

    this is a duplicate of several issues, I believe this one is the most recent one
    #1465

    This says pre-commit is "locating the wrong Python executable", so it does not look like a duplicate to me. My change does not affect the Python executable located by pre-commit. It places the Python executable located by pre-commit on PATH if it is part of a virtual environment.

    IIUC, your PR #166 that originally added the INSTALL_PYTHON mechanism (then called ENV_PYTHON) had the exact same motivation as my issue:

    This enables the following workflow:

    $ sh -c '. venv/bin/activate && pre-commit install'
    $ git commit -m ...
    which previously would have needed to have been:

    $ sh -c '. venv/bin/activate && pre-commit install'
    $ . venv/bin/activate
    $ git commit -m ...

    Please let me address your points.

    language: system is at your own support
    unmanaged environments are just that, unamanged

    Sure, no objections. This attempts to remove a pitfall from managing environments outside of pre-commit.

    though pre-commit is written in python, it is not a python-specific tool

    I understand that. This change does not add a Python-only feature. It does leverage the fact that pre-commit is a Python tool, and can co-exist with Python system hooks in an environment.

    this would create a difference between pre-commit run and git commit which is not ok

    A change in behavior only exists if the following conditions all hold:

    • pre-commit is installed in a virtual environment
    • the virtual environment is not activated in the user's shell
    • the user invokes pre-commit run with an explicit path, like this:
      • venv/bin/pre-commit run
      • venv/bin/python -m pre-commit run

    In this scenario pre-commit run would not locate hooks in the virtual environment, while git commit (with my change) would.

    I struggle to imagine a user invoking pre-commit in a virtualenv and relying on that virtualenv not being on PATH. But then again, https://xkcd.com/1172/ ;-)

    The three-line version also potentially reorders PATH. This can be avoided using something like this:

    def ensurepath(env, bindir):
    path = env.get("PATH", "").split(os.pathsep)
    if bindir not in path:
        path.insert(0, bindir)
        return dict(env, PATH=os.pathsep.join(path))

    In that case, the implementation should probably be moved to hook-impl.

    Finally, let me anticipate one other potential objection to the change:

    Why not prepend the hook entry with something like poetry run / pipenv run?

    It does not work for tox/nox. These tools would require us to hardcode the path into the relevant environment, which is not ideal. Also, a solution that is agnostic of the specific tool and only requires an activated virtualenv seems preferable.

  5. asottile commented on Jun 29, 2020

    @asottile
    asottile
    on Jun 29, 2020
    Member

    please read through the linked issues in the issue I linked you, I promise this is a duplicate. you're asking for special behaviour only for language: system hooks which is not going to happen

    In this scenario pre-commit run would not locate hooks in the virtual environment, while git commit (with my change) would.

    yes, that's exactly the difference in behaviour that I do not want

    I struggle to imagine a user invoking pre-commit in a virtualenv and relying on that virtualenv not being on PATH. But then again, https://xkcd.com/1172/ ;-)

    ok well you're not thinking too hard about this then :) -- the most common install layout for pre-commit on macos is to install pre-commit via brew. pre-commit installed via brew puts a virtualenv into /usr/local/.... activating this virtualenv would deactivate the user's virtualenv.

    my setup is the same actually, though I install into ~/opt/venv and symlink into ~/bin.

    pre-commit explicitly does not get involved with PATH manipulation because it encourages non-reproducible environment setups.

    It does not work for tox/nox.

    sure it does, I've seen quite a few people use tox -e foo as a hook

    please trust that I've already put a lot of thought into this

  6. revmischa commented on Nov 5, 2020

    @revmischa
    revmischa
    on Nov 5, 2020

    Just chiming in here to say that I use poetry and because I use mypy plugins for django, my pre-commit config looks like:

    repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.3.0 # https://github.com/pre-commit/pre-commit-hooks/releases
    hooks:
      - id: debug-statements
      - id: check-merge-conflict
  - repo: https://gitlab.com/pycqa/flake8
    rev: 3.8.4  # https://gitlab.com/pycqa/flake8/-/tags
    hooks:
    -   id: flake8
        language_version: python3.9
  # type checking
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.790
    hooks:
      - id: mypy
        additional_dependencies:  # have to add every django module here
          - django-stubs
          - djangorestframework-stubs
          - python-decouple
          - dj-database-url
          - django-filter
          - djangorestframework
          - drf-spectacular
          - pycountry
        language_version: python3.9
  # formatting
  - repo: https://github.com/psf/black
    rev: 20.8b1 # Replace by any tag/version: https://github.com/psf/black/tags
    hooks:
      - id: black
        language_version: python3.9

    Having to specify all the additional_dependencies is not ideal. Ideally pre-commit would detect my project has a poetry venv and could run the hooks in that.

  7. asottile commented on Nov 5, 2020

    @asottile
    asottile
    on Nov 5, 2020
    Member

    I'm locking this since ya'll can't read, I'm not going to reiterate this

  8. pre-commit
    locked as resolved and limited conversation to collaborators on Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    question

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions