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

pipenv: 2018.11.26 -> 2020.5.28 #89164

Merged
merged 1 commit into from Jun 1, 2020
Merged

pipenv: 2018.11.26 -> 2020.5.28 #89164

merged 1 commit into from Jun 1, 2020

Conversation

@kalekseev
Copy link
Contributor

kalekseev commented May 29, 2020

Motivation for this change

https://github.com/pypa/pipenv/releases/tag/v2020.5.28

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.
@das-g
Copy link
Member

das-g commented May 30, 2020

This derivation depends on invoke and parver:

nativeBuildInputs = [ invoke parver ];

Upstream's Pipfile no longer explicitly depends on these:
pypa/pipenv@v2018.11.26...v2020.5.28diff-1e61a31bf9b94805f869dc4137ec1885L18-L19

However, they still seem to be indirect dependencies, as they're still listed in upstream's Pipfile.lock:

Should (and can) we remove the explicit dependencies from the derivation?

@das-g
Copy link
Member

das-g commented May 30, 2020

Result of nixpkgs-review pr 89164 1

1 package built:
- pipenv
@kalekseev kalekseev force-pushed the kalekseev:update/pipenv branch from f93148b to a9643d1 May 30, 2020
@kalekseev
Copy link
Contributor Author

kalekseev commented May 30, 2020

Upstream's Pipfile no longer explicitly depends on these:
pypa/pipenv@v2018.11.26...v2020.5.28diff-1e61a31bf9b94805f869dc4137ec1885L18-L19

However, they still seem to be indirect dependencies, as they're still listed in upstream's Pipfile.lock:

Pipenv team just changed how they included dev/test deps into Pipfile, instead of repeating setup.py deps in Pipfile they do pipenv = {path = ".", editable = true, extras = ["tests", "dev"]} in https://github.com/pypa/pipenv/blob/v2020.5.28/Pipfile#L2, as a result dev and test deps https://github.com/pypa/pipenv/blob/v2020.5.28/setup.py#L34-L46 included into Pipfile.lock.

Should (and can) we remove the explicit dependencies from the derivation?

The new version can be build without them, while the old version build fails, so I assume it's safe to remove them.

@kalekseev
Copy link
Contributor Author

kalekseev commented May 30, 2020

@das-g
das-g approved these changes May 30, 2020
@kalekseev kalekseev force-pushed the kalekseev:update/pipenv branch from a9643d1 to bf8f67c May 30, 2020
@kalekseev
Copy link
Contributor Author

kalekseev commented May 30, 2020

Added patch that fixes resolver path in subprocess call.
UPDATE: found even more problems with subprocess calls (eg. pipenv install psycopg2 calls setup.py with interpreter without setuptools), so right now using interpreter with all runtime dependencies in core.py seems like the right thing todo (although my tests worked with [virtualenv setuptools pip])

@kalekseev kalekseev force-pushed the kalekseev:update/pipenv branch 2 times, most recently from ab4f141 to 947bbb4 May 30, 2020
@Mic92
Copy link
Contributor

Mic92 commented May 31, 2020

Are the issues resolved now?

pkgs/development/tools/pipenv/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/pipenv/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/pipenv/default.nix Outdated Show resolved Hide resolved
substituteInPlace pipenv/core.py \
--replace "vistir.compat.Path(sys.executable).absolute().as_posix()" "vistir.compat.Path('${pythonEnv.interpreter}').absolute().as_posix()"
--replace "sys.executable" "'${pythonEnv.interpreter}'"

This comment has been minimized.

Copy link
@das-g

das-g May 31, 2020

Member

👍 for simplifying the substitution while you had to make it wider anyway.

@kalekseev
Copy link
Contributor Author

kalekseev commented May 31, 2020

@Mic92
Copy link
Contributor

Mic92 commented May 31, 2020

cc @das-g

@das-g
Copy link
Member

das-g commented May 31, 2020

For 947bbb4 on NixOS unstable:

Result of nixpkgs-review pr 89164 1

1 package built:
- pipenv

Tested with some pure-python packages:

(Non-pure packages might require a FHS environment with certain libs available.)

pipenv install ipython
output
Creating a virtualenv for this project…
Pipfile: /home/das-g/tmp/pipenv-foo/Pipfile
Using /nix/store/q732h09azy7lf0j30bnnhdl15p4rxpdy-python3-3.7.7/bin/python3.7m (3.7.7) to create virtualenv…
⠴ Creating virtual environment...created virtual environment CPython3.7.7.final.0-64 in 348ms
  creator CPython3Posix(dest=/home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM, clear=False, global=False)
  seeder FromAppData(download=False, pip=latest, setuptools=latest, wheel=latest, via=copy, app_data_dir=/home/das-g/.local/share/virtualenv/seed-app-data/v1.0.1)
  activators BashActivator,CShellActivator,FishActivator,PowerShellActivator,PythonActivator,XonshActivator

✔ Successfully created virtual environment! 
Virtualenv location: /home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM
Creating a Pipfile for this project…
⠋ Installing...Installing ipython…
Adding ipython to Pipfile's [packages]…
✔ Installation Succeeded 
Pipfile.lock not found, creating…
Locking [dev-packages] dependencies…
Locking [packages] dependencies…
Building requirements...
Resolving dependencies...
✔ Success! 
Updated Pipfile.lock (c8fa08)!
Installing dependencies from Pipfile.lock (c8fa08)…
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 1/1 — 00:00:00
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
pipenv run ipython
output
Python 3.7.7 (default, Mar 10 2020, 06:34:06) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.15.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]:

IPython session:

In [1]: ls                                                                      
Pipfile  Pipfile.lock

In [2]:  # Pressed Ctrl-d
Do you really want to exit ([y]/n)? 
pipenv install pytest
output
Installing pytest…
Adding pytest to Pipfile's [packages]…
✔ Installation Succeeded 
Pipfile.lock (c8fa08) out of date, updating to (ae86da)…
Locking [dev-packages] dependencies…
Locking [packages] dependencies…
Building requirements...
Resolving dependencies...
✔ Success! 
Updated Pipfile.lock (ae86da)!
Installing dependencies from Pipfile.lock (ae86da)…
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 3/3 — 00:00:00
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
pipenv shell
output
Launching subshell in virtual environment…
. /home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM/bin/activate
(pipenv-foo) $>

pipenv shell session:

(pipenv-foo) $> pytest
============================= test session starts ==============================
platform linux -- Python 3.7.7, pytest-5.4.2, py-1.8.1, pluggy-0.13.1
rootdir: /home/das-g/tmp/pipenv-foo
collected 0 items                                                              

============================ no tests ran in 0.00s =============================
(pipenv-foo) $> exit
pipenv --rm

output:

Removing virtualenv (/home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM)…
pipenv install
output
Creating a virtualenv for this project…
Pipfile: /home/das-g/tmp/pipenv-foo/Pipfile
Using /nix/store/q732h09azy7lf0j30bnnhdl15p4rxpdy-python3-3.7.7/bin/python3.7m (3.7.7) to create virtualenv…
⠼ Creating virtual environment...created virtual environment CPython3.7.7.final.0-64 in 225ms
  creator CPython3Posix(dest=/home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM, clear=False, global=False)
  seeder FromAppData(download=False, pip=latest, setuptools=latest, wheel=latest, via=copy, app_data_dir=/home/das-g/.local/share/virtualenv/seed-app-data/v1.0.1)
  activators BashActivator,CShellActivator,FishActivator,PowerShellActivator,PythonActivator,XonshActivator

✔ Successfully created virtual environment! 
Virtualenv location: /home/das-g/.local/share/virtualenvs/pipenv-foo-7RzlY9FM
Installing dependencies from Pipfile.lock (ae86da)…
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 23/23 — 00:00:03
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.

pipenv run pip freeze
output
appdirs @ file:///build/appdirs-1.4.3/dist/appdirs-1.4.3-py2.py3-none-any.whl
attrs==19.3.0
backcall==0.1.0
certifi @ file:///build/certifi-2020.4.5.1/dist/certifi-2020.4.5.1-py2.py3-none-any.whl
decorator==4.4.2
distlib @ file:///build/distlib-0.3.0/dist/distlib-0.3.0-py3-none-any.whl
filelock @ file:///build/filelock-3.0.12/dist/filelock-3.0.12-py3-none-any.whl
importlib-metadata @ file:///build/importlib_metadata-1.5.0/dist/importlib_metadata-1.5.0-py2.py3-none-any.whl
ipython==7.15.0
ipython-genutils==0.2.0
jedi==0.17.0
more-itertools @ file:///build/more-itertools-8.0.2/dist/more_itertools-8.0.2-py3-none-any.whl
packaging==20.4
parso==0.7.0
pexpect==4.8.0
pickleshare==0.7.5
pipenv @ file:///build/pipenv-2020.5.28/dist/pipenv-2020.5.28-py2.py3-none-any.whl
pluggy==0.13.1
prompt-toolkit==3.0.5
ptyprocess==0.6.0
py==1.8.1
Pygments==2.6.1
pyparsing==2.4.7
pytest==5.4.2
six @ file:///build/six-1.14.0/dist/six-1.14.0-py2.py3-none-any.whl
traitlets==4.3.3
virtualenv @ file:///build/virtualenv-20.0.21/dist/virtualenv-20.0.21-py2.py3-none-any.whl
virtualenv-clone @ file:///build/virtualenv-clone-0.5.4/dist/virtualenv_clone-0.5.4-py3-none-any.whl
wcwidth==0.1.9
zipp @ file:///build/zipp-0.6.0/dist/zipp-0.6.0-py2.py3-none-any.whl
cat Pipfile
output
[[source]]
name = "pypi"
url = "https://pypi.org/simple"
verify_ssl = true

[dev-packages]

[packages]
ipython = "*"
pytest = "*"

[requires]
python_version = "3.7"

👍 I'd say that looks good.

@kalekseev kalekseev force-pushed the kalekseev:update/pipenv branch from 947bbb4 to 730dead May 31, 2020
@kalekseev
Copy link
Contributor Author

kalekseev commented May 31, 2020

@das-g thanks for suggestions, I was in a hurry today morning so pushed fixed version without checking comment's wording. I have a question, I pass runtimeDeps into clojure instead of using clojure's argument ps https://github.com/NixOS/nixpkgs/pull/89164/files#diff-745b71166205f5309e101521f5e9148bR17, so that can be simplified to python3.withPackages(ps: runtimeDeps); but more importantly I'm not sure if this correct according to nix or we must use packages from function argument.

@das-g
das-g approved these changes May 31, 2020
Copy link
Member

das-g left a comment

LGTM 👍

:shipit: Ship it!

@das-g
Copy link
Member

das-g commented May 31, 2020

I have a question, I pass runtimeDeps into clojure instead of using clojure's argument ps https://github.com/NixOS/nixpkgs/pull/89164/files#diff-745b71166205f5309e101521f5e9148bR17, so that can be simplified to python3.withPackages(ps: runtimeDeps); but more importantly I'm not sure if this correct according to nix or we must use packages from function argument.

I think you mean "closure". Clojure's a programming language.

I guess this works because the packages that python3.withPackages will feed to the function it gets passed are the same ones as python3.pkgs anyway (or aren't they? I'm unsure), so the whole thing will evaluate to the same value in a different way. Whether it's good style, I don't know. It does seem kinda off, now that you point it out.

@kalekseev
Copy link
Contributor Author

kalekseev commented May 31, 2020

I think you mean "closure". Clojure's a programming language.

yes, thanks

@das-g
Copy link
Member

das-g commented May 31, 2020

One option would be to set the variable to the function instead of the list: das-g/nixpkgs@82b639e

@kalekseev
Copy link
Contributor Author

kalekseev commented May 31, 2020

It looks cleaner for me one thing I would change is to use verb runtimeDeps -> buildRuntimeDeps.
What do you think should I update pr with that change?

@das-g
Copy link
Member

das-g commented May 31, 2020

It looks cleaner for me one thing I would change is to use verb runtimeDeps -> buildRuntimeDeps.

What would that changed name mean? That the function builds the runtime dependencies? (It doesn't. It just selects some entries from a set and returns them as a list.) Or that it returns buildtime and runtime dependencies? (Is that the case?)

@kalekseev
Copy link
Contributor Author

kalekseev commented May 31, 2020

The problem is my limited experience with nix so I'm just trying to mimic things and names that I already saw somewhere in the code, so I think you are right buildRuntimeDeps is not good name for that. Let me know if I should push your original patch and thanks for your help.

@das-g
Copy link
Member

das-g commented May 31, 2020

The problem is my limited experience with nix so I'm just trying to mimic things and names that I already saw somewhere in the code, so I think you are right buildRuntimeDeps is not good name for that.

Yeah. I myself thought about renaming the variable now that it holds a function rather than a list, but I'm not aware of any naming convention in Nix to indicate that fact. Some functions seem to be named after what they do, others after what they return.

Let me know if I should push your original patch and thanks for your help.

If you feel that it's an improvement, sure. I'm probably not that much more experienced in Nix than you. Feel free to use my commit das-g@82b639e as-is or to fold it into yours, whichever you prefer.

If we need expert judgment, maybe @FRidh or @jonringer (code owners of Python related things in NixPkgs) can chime in?

# Python-related code and docs
/maintainers/scripts/update-python-libraries @FRidh
/pkgs/top-level/python-packages.nix @FRidh @jonringer
/pkgs/development/interpreters/python @FRidh
/pkgs/development/python-modules @FRidh @jonringer
/doc/languages-frameworks/python.section.md @FRidh

@FRidh FRidh merged commit 5e8e887 into NixOS:master Jun 1, 2020
16 checks passed
16 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="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="730dead"; rev="730deaddcff3df1d1ea9cd4e562970afda00b693"; } ./pkgs/t
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
pipenv, pipenv.passthru.tests on aarch64-linux Success
Details
pipenv, pipenv.passthru.tests on x86_64-linux Success
Details
@das-g
Copy link
Member

das-g commented Jun 1, 2020

FRidh merged commit 5e8e887 into NixOS:master 3 hours ago

Eh, that works, too. 🤷 I'll make a separate PR of das-g/nixpkgs@82b639e then.

@das-g das-g mentioned this pull request Jun 1, 2020
5 of 10 tasks complete
@das-g
Copy link
Member

das-g commented Jun 1, 2020

Eh, that works, too. 🤷 I'll make a separate PR of das-g/nixpkgs@82b639e then.

Done: #89293

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

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