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

python3Packages.pre-commit: 1.18.1 -> 1.18.2 #67509

Merged
merged 1 commit into from Aug 27, 2019

Conversation

@borisbabic
Copy link
Contributor

commented Aug 26, 2019

Motivation for this change

Version update

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@risicle

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

nix-review builds for me, macos 10.13. Can't say much more than that though - no tests.

Copy link
Contributor

left a comment

nix-review passes on NixOS
diff LGTM
executables seem to work
leaf package

https://github.com/NixOS/nixpkgs/pull/67509
2 package were build:
pre-commit python27Packages.pre-commit

@FRidh when i run the pre-commit package in an environment with python27Packages.pre-commit (such as nix-review shell), i seem to be inheriting some of those packages even with a wrapped program. Should we just disallow receiving pythonpaths from environment as part of the wrapping process?

$ ./results/pre-commit/bin/pre-commit --help
Traceback (most recent call last):
  File "/nix/store/921z4azpr87cnjc1mip8sk7vghkkqh83-pre-commit-1.18.2/bin/.pre-commit-wrapped", line 7, in <module>
    from pre_commit.main import main
  File "/nix/store/m4pigfn9lphmcdqa9nfjz159ivj91f1c-pre-commit-1.18.2/lib/python2.7/site-packages/pre_commit/main.py", line 11, in <module>
    from pre_commit import git
  File "/nix/store/m4pigfn9lphmcdqa9nfjz159ivj91f1c-pre-commit-1.18.2/lib/python2.7/site-packages/pre_commit/git.py", line 7, in <module>
    from pre_commit.util import cmd_output
  File "/nix/store/m4pigfn9lphmcdqa9nfjz159ivj91f1c-pre-commit-1.18.2/lib/python2.7/site-packages/pre_commit/util.py", line 18, in <module>
    from importlib.resources import open_binary
  File "/nix/store/yglc8kgzmdaskyngv90y1mw96fl00pjy-python3-3.7.4/lib/python3.7/importlib/resources.py", line 11, in <module>
    from typing import Iterable, Iterator, Optional, Set, Union   # noqa: F401
  File "/nix/store/44gm1b72bh2ry0pb0n5yi2rncj8lgx04-python2.7-typing-3.6.6/lib/python2.7/site-packages/typing.py", line 624, in <module>
    AnyStr = TypeVar('AnyStr', bytes, unicode)
NameError: name 'unicode' is not defined

from pkgs/applications/version-management/git-and-tools/default.nix:

  pre-commit = pkgs.python3Packages.toPythonApplication pkgs.python3Packages.pre-commit;
@FRidh

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@jonringer nix-review should not use nix-shell but nix run instead. That way the Python hook is not ran. Ignoring PYTHONPATH in our wrappers can be done but should we. The env vars PYTHONPATH, PYTHONHOME, PYTHONNOUSERSITE` mess with the hardness of our builds, yet they are Python features.

@FRidh FRidh merged commit 1296b10 into NixOS:master Aug 27, 2019
13 checks passed
13 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="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
@jonringer

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Yea, that's what i realized was happening, by default nix-review drops you into a nix-shell and my PYTHONPATH was inheriting ALL of the python packages.

I still think it would be nice to have a "isolated" python wrapping process which will insulate a given python application from receiving PYTHONPATH (such as unsetting it before the wrapper calls into the python application).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.