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.

Sign up for GitHub

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

Allow skipping the use of the PYTHONNOUSERSITE variable #51641

Merged
merged 2 commits into from May 13, 2019
Merged

Allow skipping the use of the PYTHONNOUSERSITE variable #51641

merged 2 commits into from May 13, 2019

Conversation

thomasjm
Copy link
Contributor

@thomasjm thomasjm commented Dec 6, 2018

Motivation for this change

By default, Nix-installed Python binaries use the PYTHONNOUSERSITE variable (see here) to prevent Python from looking for packages in the user's site-packages directory.

I wanted to be able to install a Python installation that is slightly impure, so that you can run pip install --user some_package and then use the resulting package. This is convenient when you want to set up an environment where the user either can't install new Python packages via Nix, or would prefer to use pip directly (for example, if a package version isn't available on Nix).

So, this PR introduces a flag skipNoUserSite that can be used to disable the PYTHONNOUSERSITE setting when needed. The change happens in two places:

  1. pkgs/development/interpreters/python/mk-python-derivation.nix and pkgs/development/interpreters/python/wrap.sh: the change here allows you to disable PYTHONNOUSERSITE for individual Python packages, i.e. those built by buildPythonPackage.
  2. pkgs/development/interpreters/python/wrapper.nix: the change here allows you to disable PYTHONNOUSERSITE for Python environments, i.e. those created by python.withPackages. It causes every wrapped binary inside the environment to be user site-packages aware.

This flag is purely opt-in, so hopefully it's okay to add a little impurity for this use case :)

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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@thomasjm thomasjm requested a review from FRidh as a code owner December 6, 2018 23:45
@thomasjm
Copy link
Contributor Author

Friendly ping on this @FRidh

FRidh
FRidh requested changes Feb 15, 2019
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

I think it's fine to allow this. Some small changes needed and then target staging.

pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved
pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved
pkgs/development/interpreters/python/wrapper.nix Outdated Show resolved Hide resolved
@FRidh FRidh added this to the 19.03 milestone Feb 15, 2019
@thomasjm
Copy link
Contributor Author

I think it's good to go now? Is there anything I need to do to target staging?

@FRidh
Copy link
Member

FRidh commented Feb 15, 2019

Squash commits so we have two commits again, and then rebase onto staging.

@thomasjm thomasjm force-pushed the nousersite-master branch 2 times, most recently from 8ac937e to 03b8fb8 Compare February 15, 2019 09:07
@thomasjm
Copy link
Contributor Author

Whoops are you sure I should have rebased onto staging? Should I change the target of this PR to staging or rebase onto master? (Sorry everyone who was just added for the spam)

@FRidh FRidh changed the base branch from master to python-unstable February 15, 2019 09:09
@thomasjm
Copy link
Contributor Author

@FRidh anything else I need to do on this one or is it waiting on more review? Thanks!

@FRidh FRidh modified the milestones: 19.03, 19.09 Feb 23, 2019
@FRidh FRidh mentioned this pull request Mar 23, 2019
Closed
10 tasks
@thomasjm
Copy link
Contributor Author

Friendly ping on this @FRidh , is it ready to land? Github is acting like there are changes requested but I think it's confused...

@FRidh
Copy link
Member

FRidh commented Apr 19, 2019 via email

@thomasjm
Copy link
Contributor Author

@FRidh could we push this one over the finish line? Thanks!

@FRidh
Copy link
Member

FRidh commented May 12, 2019

Commit titles are incorrect, it's PYTHONNOUSERSITE.

@thomasjm
Copy link
Contributor Author

Fixed

@FRidh FRidh merged commit d801204 into NixOS:staging May 13, 2019
@FRidh
Copy link
Member

FRidh commented May 13, 2019

In a followup commit, please explain the options in the Nixpkgs manual.

@thomasjm
Copy link
Contributor Author

You mean a followup PR? I can't add a commit here since this PR was merged.

@thomasjm thomasjm mentioned this pull request May 14, 2019
Merged
@thomasjm
Copy link
Contributor Author

Created #61502

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

@FRidh FRidh FRidh requested changes

@lovek323 lovek323 Awaiting requested review from lovek323

@pSub pSub Awaiting requested review from pSub

Assignees

@FRidh FRidh

Labels
2.status: work-in-progress 6.topic: python 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
19.09
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thomasjm @FRidh @GrahamcOfBorg