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

doc: python: refreshing virtualenv section for venv #77569

Merged
merged 1 commit into from Jan 21, 2020

Conversation

@d-goldin
Copy link
Contributor

@d-goldin d-goldin commented Jan 12, 2020

Motivation for this change

Updating section about imperative use of ad-hoc virtual-environments
for use of pythons built-in venv module. Also trying to make it a bit
friendlier to beginners by adding a bit more explanation to the code
snippet and some remarks about using python 2.7 + virtualenv
and making it a bit more usable out of the box by not trying to re-create
the virtual environment each time.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.
Copy link
Contributor

@jonringer jonringer left a comment

LGTM

@FRidh
Copy link
Member

@FRidh FRidh commented Jan 13, 2020

If this is a supposedly popular way of working, why don't we provide a hook for this?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 13, 2020

what do you recommend this looking like?

  (python.pkgs.virtualenv.venvHook.withName "dev-venv")
  python.pkgs.virtualenv.venvHook # alias for .withName "venv"
@FRidh FRidh mentioned this pull request Jan 13, 2020
0 of 10 tasks complete
@FRidh
Copy link
Member

@FRidh FRidh commented Jan 13, 2020

Please have a look at #77644.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 13, 2020

I like the venvShellHook better in #77644

@d-goldin
Copy link
Contributor Author

@d-goldin d-goldin commented Jan 14, 2020

@jonringer: I think the shellhook is a decent idea to improve out of the box ease of use, but think the doc could be improved a bit, so I will adjust this PR to mention the hook, but I also think it might make sense to leave the "manual way" for educational reasons, for example if people need to adjust it to old-school virtualenv or similar. Does that make sense?

@ofborg ofborg bot removed the 6.topic: python label Jan 14, 2020
@d-goldin
Copy link
Contributor Author

@d-goldin d-goldin commented Jan 14, 2020

If this is a supposedly popular way of working, why don't we provide a hook for this?

Just to add why I wanted to minimally improve this example: While not overly idiomatic from nix perspective, I think this is a workflow that many new-comers are looking at when doing python. Likely because they have to deal with some projects / dependencies that are not packaged but also lack understanding yet of how to do it more idiomatically. I've observed questions about this a few times on IRC so decided to maybe add a few words to lower barrier to entry / for gradual transition.

@FRidh
Copy link
Member

@FRidh FRidh commented Jan 14, 2020

I think the explanation is good and helpful. Please use python3 instead of python2 though.

@d-goldin
Copy link
Contributor Author

@d-goldin d-goldin commented Jan 14, 2020

Oh, sorry, copy paste fail. Also forgot the pip install post hook. Will fix.

@d-goldin d-goldin force-pushed the d-goldin:python_doc_venv branch from 7d68ff2 to 5ded64d Jan 14, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 14, 2020

I wasn't trying to say this wasn't worthwhile, it's still an improvement to documentation which usually can always need some help.

I was just trying to express that venvShellHook is a lot less repetitious

Copy link
Contributor

@jonringer jonringer left a comment

LGTM, thank you for updating docs :)

Copy link
Contributor

@jonringer jonringer left a comment

otherwise LGTM

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved
Updating section about imperative use of ad-hoc virtual-environments for
use of pythons built-in `venv` module via venvShellHook.  Also trying to
make it a bit friendlier to beginners by adding a bit more explanation
to the code snippet and some remarks old-school virtualenv.

Adjusting for venvShellHook and adding manual example

Adding pip install and replacing python2 example with python3
@d-goldin d-goldin force-pushed the d-goldin:python_doc_venv branch from 5ded64d to d69bfcd Jan 14, 2020
@d-goldin
Copy link
Contributor Author

@d-goldin d-goldin commented Jan 20, 2020

I don't have merge rights, so would be nice if someone else could do that.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 21, 2020

sorry, forgot about this

@jonringer jonringer merged commit 25d0d2b into NixOS:master Jan 21, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
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
@d-goldin d-goldin deleted the d-goldin:python_doc_venv branch Jan 26, 2020
d-goldin added a commit to d-goldin/nixpkgs that referenced this pull request Jan 31, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to NixOS#77569
@d-goldin d-goldin mentioned this pull request Jan 31, 2020
2 of 10 tasks complete
d-goldin added a commit to d-goldin/nixpkgs that referenced this pull request Feb 2, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to NixOS#77569
jonringer added a commit that referenced this pull request Feb 2, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to #77569
jpgu-epam added a commit to jpgu-epam/nixpkgs that referenced this pull request Feb 4, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to NixOS#77569
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

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