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

buildPythonPackage: accept developmentPrefix and send pip output to stderr #22015

Closed
wants to merge 1 commit into from

Conversation

timbertson
Copy link
Contributor

Motivation for this change

This changes 2 small parts about buildPythonPackage. I'm happy to split them into two PRs if you prefer, but I figured I'd combine them since they require a rebuild of all python packages.

  1. send pip install output to stderr. The nix integration in direnv (https://direnv.net/, from @zimbatm) relies on nix-shell --run CMD producing only the output from CMD on stdout (it dumps the environment from inside nix-shell and recreates it in your current shell). The python shellHook logs the following pip boilerplate to stdout, which breaks this functionality:
Obtaining file:///home/tim/dev/python/test
Installing collected packages: test
  Running setup.py develop for test
Successfully installed test

This is clearly output that belongs on stderr, so I've added a >&2 redirect in the shell hook.

  1. allow passing developmentPrefix to buildPythonPackage. This is used only in the shellHook, and allows a specific path to be used for development installation, rather than creating a tempfile each time. This keeps the $PATH reproducible, which is important if you store it somewhere to be reused at a later time (since nix-shell can take a while on complex inputs, I cache the results used by direnv to set my environment). The default behaviour is unchanged.
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@timbertson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

@@ -13,6 +13,7 @@
, preShellHook ? ""
# Execute after shell hook
, postShellHook ? ""
, developmentPrefix ? ""
Copy link
Member

@zimbatm zimbatm Jan 23, 2017

Choose a reason for hiding this comment

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

By reading this variable name I'm not able to infer what it's doing.

Shouldn't the dependencies be packaged in their own derivation instead of running pip install in the shellHook?

Copy link
Member

Choose a reason for hiding this comment

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

What about shellHookPipDir?

@FRidh
Copy link
Member

FRidh commented Jan 23, 2017

@timbertson would you argue this feature is important enough to warrant a change in buildPythonPackage in nixpkgs that we basically will have to support, instead of passing a custom shellHook yourself?

@timbertson
Copy link
Contributor Author

@FRI it's a fair point. I'm not super familiar with the buildPythonPackage shellhook, so it's hard to answer. To be honest, it would be better if I didn't have to pass in anything special for this to work, because otherwise I'd have to modify each derivation I want to use. This optional param makes that easier, but still requires modification.

I've extracted just the stderr redirect change to #22086, since I doubt anyone objects to that change. I'll spend some more time with this shellHook and see if overriding it locally (or just working around the error) is too much trouble.

@timbertson timbertson closed this Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants