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

Jump to bottom

Python: add sitecustomize.py, listen to NIX_PYTHONPATH #64634

Merged
merged 2 commits into from Jul 13, 2019
Merged

Python: add sitecustomize.py, listen to NIX_PYTHONPATH #64634

merged 2 commits into from Jul 13, 2019

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Jul 11, 2019

Motivation for this change

Split from #25985.

Add a sitecustomize.py file that listens to NIX_PYTHONPATH and unsets it afterwards. This gives the same feature as both PYTHONHOME and PYTHONPATH, however, without preventing the user from using those environment variables, and without leaking the variable.

This variable is now used in python.buildEnv, thereby solving issues with a leaking PYTHONHOME which could break nested Python programs/environments.

In the future we may choose to use this as well in buildPythonPackage.

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.

@FRidh FRidh mentioned this pull request Jul 11, 2019
Closed
7 tasks
@risicle
Copy link
Contributor

risicle commented Jul 11, 2019

This is a really neat idea - I've been doing something similar in a much hackier way to get virtualenvs to work with less hassle - simply using sitecustomize.py to re-sort all of the paths beginning with /nix/ to the "end" of sys.path.

Moving towards preferring PYTHONPATH provided packages over nix-provided ones... is there a worry this will threaten the "integrity" (for want of a better word) of nix-provided python applications?

@FRidh
Copy link
Member Author

FRidh commented Jul 12, 2019

I don't follow you. This does not use PYTHONPATH. We may eventual use this also in other wrappers but I don't see any negative changes there.

abbradar
abbradar approved these changes Jul 12, 2019
View reviewed changes
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, this commit introduces NIX_PYTHON_SCRIPT_NAME which
can be used for setting sys.argv[0].

Not in the commit, a rebasing mistake?

I needed to read on the previous discussions since I have completely forgotten what's the problem we are trying to solve :D

It's better than using PYTHONHOME. I like the idea of unsetting the variable; if we apply it to NIX_PYTHON_SCRIPT_NAME won't this solve its leakiness? I don't see any other problems with it, maybe I missed anything in #25985?

We still don't solve crazy cases like (5) from #25985 (comment) but that's a clear improvement.

@abbradar
Copy link
Member

abbradar commented Jul 12, 2019 •
edited

And if we move to using them both in buildPythonPackage we finally can get rid of this abomination!. The world would definitely become a better place. So I'm all for going all in, again unless I miss anything.

@FRidh
Python: add sitecustomize.py, listen to NIX_PYTHONPATH
a432735 
This commit adds a Nix-specific module that recursively adds paths that
are on `NIX_PYTHONPATH` to `sys.path`. In order to process possible
`.pth` files `site.addsitedir` is used.

The paths listed in `PYTHONPATH` are added to `sys.path` afterwards, but
they will be added before the entries we add here and thus take
precedence.

The reason for adding support for this environment variable is that we
can set it in a wrapper without breaking support for `PYTHONPATH`.
@FRidh
python.buildEnv: use NIX_PYTHONPATH
99a8893
@FRidh
Copy link
Member Author

FRidh commented Jul 12, 2019

  1. should simply not be done.

I've updated the commit message. Yes, we could use NIX_PYTHON_SCRIPT_NAME but I won't to test a bit more with exec -a, whether it really really does not work.

@risicle
Copy link
Contributor

risicle commented Jul 12, 2019 •
edited

@FRidh my point is just that users may have custom entries in their runtime environment's PYTHONPATH and if these now get prioritized in front of NIX_PYTHONPATH entries, $ some-python-based-cli-tool may have one of its dependencies instead resolved to the runtime environment's and break.

It's possible I've misunderstood how this is being handled with makeWrapper though.

@FRidh
Copy link
Member Author

FRidh commented Jul 13, 2019

Ah, now I understand. No, this won't change.

@FRidh FRidh merged commit ec7f569 into NixOS:staging Jul 13, 2019
@FRidh FRidh deleted the sitecustomize branch July 13, 2019 07:37
@FRidh FRidh mentioned this pull request Jul 13, 2019
Closed
@FRidh FRidh mentioned this pull request Nov 12, 2019
Open
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/terminal-emulator-leaks-environment-variables-to-shell/33673/11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbradar abbradar abbradar approved these changes

@cstrahan cstrahan Awaiting requested review from cstrahan

@dotlambda dotlambda Awaiting requested review from dotlambda

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@FRidh @risicle @abbradar @nixos-discourse