Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pre-commit: 1.21.0 -> 2.4.0 #88456

Merged
merged 1 commit into from Jun 2, 2020
Merged

pre-commit: 1.21.0 -> 2.4.0 #88456

merged 1 commit into from Jun 2, 2020

Conversation

@kalekseev
Copy link
Contributor

@kalekseev kalekseev commented May 20, 2020

Motivation for this change

pre-commit can't install environment for python based hooks because it use module version call: python -mvirtualenv

Things done

Created a patch that replace python -mvirtualenv with ${virtualenv}/bin/virtualenv, used existing hook-tmpl patch as a guidance.

Problem reproduction

Create .pre-commit-config.yaml

repos:
  -   repo: https://gitlab.com/pycqa/flake8
      rev: 3.8.1
      hooks:
      -   id: flake8

Run pre-commit

[INFO] Initializing environment for https://gitlab.com/pycqa/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7/bin/python3.7', '-mvirtualenv', '/home/u/.cache/pre-commit/repoxx7im869/py_env-python3.7', '-p', '/nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7/bin/python3.7')
return code: 1
expected return code: 0
stdout: (none)
stderr:
    /nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7/bin/python3.7: No module named virtualenv

Check the log at /home/u/.cache/pre-commit/pre-commit.log
@FRidh
Copy link
Member

@FRidh FRidh commented May 21, 2020

@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 21, 2020

Or make it use stdlib venv instead?

Well, check that comment from pre-commit author pre-commit/pre-commit#1114 (comment), basically pre-commit can be configured to use venv but not having virtualenv is considered: outside the supported setup, thus I made it use virtualenv to match setups outside nix.

@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 21, 2020

Hi @asottile, could you give us quick advice on this question: can we switch to -mvenv by default, does that have any downsides if we are targeting python37+, are you planning to drop virtualenv dependency in the future?

@asottile
Copy link

@asottile asottile commented May 21, 2020

I am going the opposite way of dropping -mvirtualenv: the -mvenv support was removed in pre-commit 2.4.0 and will not be supported moving forward (language: python_venv is now an alias to language: python). -mvenv has quite a lot of downsides, notably: portability, cross language version support, environment setup speed, and install speed. -mvirtualenv creates venv-compatible modules as of virtualenv 20.x so venv itself should be unnecessary.

@asottile
Copy link

@asottile asottile commented May 21, 2020

this patch also appears to be against an old version of pre-commit 🤷

@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 21, 2020

@asottile thanks!

this patch also appears to be against an old version of pre-commit

yes, I believe we are blocked on v1 for now, at a minimum we need virtualenv bumped first to v20, also I'm not sure how nixpkgs supports different version in nixpkgs.python27Packages and in python3 so I just fixed current version of pre-commit.

@asottile
Copy link

@asottile asottile commented May 21, 2020

ah you're still supporting python 2? I'm so sorry

-mvenv also doesn't exist in python2

@andersk
Copy link
Contributor

@andersk andersk commented May 26, 2020

at a minimum we need virtualenv bumped first to v20

That’s happened now (#88613).

also I'm not sure how nixpkgs supports different version in nixpkgs.python27Packages and in python3

I don’t think there’s any reason we need to continue supporting python27Packages.pre-commit.

@kalekseev kalekseev force-pushed the kalekseev:fix/pre-commit branch from 5b39bcc to ce625b7 May 26, 2020
@@ -2406,7 +2406,7 @@ in {

pkginfo = callPackage ../development/python-modules/pkginfo { };

pre-commit = callPackage ../development/python-modules/pre-commit { };
pre-commit = disabledIf isPy27 (callPackage ../development/python-modules/pre-commit { });

This comment has been minimized.

@kalekseev

kalekseev May 26, 2020
Author Contributor

Not sure if this right thing to do, the top comment in this file states that python cli utils don't belong to that file, so I think we should remove this line.

This comment has been minimized.

@FRidh

FRidh May 26, 2020
Member

Well spotted, but it's OK here. The idea is that importable modules are in this file. Apparently pre-commit is also used that way, so that's why it is here as well.

@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 26, 2020

at a minimum we need virtualenv bumped first to v20

That’s happened now (#88613).

also I'm not sure how nixpkgs supports different version in nixpkgs.python27Packages and in python3

I don’t think there’s any reason we need to continue supporting python27Packages.pre-commit.

Thanks, I updated PR and bumped pre-commit to 2.4.0 version, but during testing found that nodeenv package is broken:

❯ nodeenv
Traceback (most recent call last):
  File "/nix/store/li46lf445jpcj451c5rq6pd5hhj4frg3-python3.7-nodeenv-1.3.3/bin/.nodeenv-wrapped", line 6, in <module>
    from nodeenv import main
  File "/nix/store/li46lf445jpcj451c5rq6pd5hhj4frg3-python3.7-nodeenv-1.3.3/lib/python3.7/site-packages/nodeenv.py", line 43, in <module>
    from pkg_resources import parse_version
ModuleNotFoundError: No module named 'pkg_resources'
@kalekseev kalekseev changed the title pre-commit: hardcode path to virtualenv's binary pre-commit: 1.21.0 -> 2.4.0 May 26, 2020
@FRidh
Copy link
Member

@FRidh FRidh commented May 26, 2020

Thanks, I updated PR and bumped pre-commit to 2.4.0 version, but during testing found that nodeenv package is broken:

nodeenv needs setuptools in propagatedBuildInputs.

@ofborg ofborg bot added the 8.has: clean-up label May 26, 2020
@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 26, 2020

nodeenv needs setuptools in propagatedBuildInputs.

can we fix that in this pr or must create a separate?

@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented May 27, 2020

I've created pr with nodeenv fix #89010, now changes in this pr work for all my use cases (mostly python, node hooks)

@kalekseev kalekseev force-pushed the kalekseev:fix/pre-commit branch from ce625b7 to 4cbca43 May 27, 2020
Copy link
Contributor

@jonringer jonringer left a comment

diff LGTM

https://github.com/NixOS/nixpkgs/pull/88456
2 packages built:
pre-commit python38Packages.pre-commit
@kalekseev
Copy link
Contributor Author

@kalekseev kalekseev commented Jun 2, 2020

@jonringer @FRidh anything else I can do to make this merged?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jun 2, 2020

no, looks like i forgot to merge

@jonringer jonringer merged commit 49ca8ce into NixOS:master Jun 2, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4cbca43"; rev="4cbca43e63a5f8ae93265b1faeb8f0ca68c308ca"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
pre-commit, pre-commit.passthru.tests on aarch64-linux Success
Details
pre-commit, pre-commit.passthru.tests on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.