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

python: rework how venv and poetry are handled #522

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

bobvanderlinden
Copy link
Contributor

Currently devenv manages its own virtualenv in DEVENV_STATE/venv. In most circumstances this works fine. However, since poetry in some cases wants to manage its own virtualenv it breaks down.

To reproduce the bug:

  • Initialize devenv with venv and poetry.
  • Initialize pyproject.toml
  • Change the python version in pyproject.toml to one that doesn't match the python version in devenv.
  • Run poetry install or rerun devenv

Poetry will (by default) create a new virtualenv in $HOME and use that, instead of the DEVENV_STATE/venv one from devenv.

It is not possible to instruct poetry to specifically use the virtualenv that devenv created. However, it is possible to instruct poetry to use PROJECT_ROOT/.venv and it is able to create it when it's gone.

I opted to also use PROJECT_ROOT/.venv when languages.python.venv.enable is set instead of DEVENV_STATE/venv. I found that it is a convention to use PROJECT_ROOT/.venv and tools (like poetry) are presuming this is the case.

Lastly, the change also includes a fix for an issue that happens when poetry install fails. When it failed previously the checksum of the lock file was still stored. This results in devenv not running poetry install anymore the next time devenv runs. Now devenv tells the user that installation fails and to run devenv install manually.

Whenever poetry install failed a checksum was still written. This means
the next run of devenv would skip installation.

It is better to indicate to the user that the installation has failed
and avoid writing the checksum. It results in the next run of devenv to
rerun the installation.
Whenever config.languages.python.poetry.enable is set devenv would
previously create a .venv directory using `python -m venv`. This works
fine, but poetry itself is also able to create virtualenv, which is
automatically used when poetry needs a virtualenv and it does not exist.

To keep virtualenv consistent it is best to create the virtualenv using
poetry to avoid a 'devenv'-version of virtualenv and a poetry one.

Since poetry also handles switching of python interpreter I opted to use
this as well.

This also decouples config.languages.python.venv.enable from
config.languages.python.poetry.enable.
Previously poetry was overridden with the python version that was
supplied in config.languages.python.package.

However, the version of poetry that is in nixpkgs is not always
compatible with the version of python that is supplied in devenv.

Since Poetry should always be operating independently from
project-specific python version it should be fine to keep the
pkgs.poetry package as-is.
`.devenv/state/venv` was used to create virtual python environments. For
most projects this is fine, but for poetry there are some specific
exceptions where this isn't a good idea. `.venv` in the root of the
project is a common convention and poetry has explicit support for this
convention.

The problem where `.devenv/state/venv` is not a good idea is when the
python version in devenv does not match the python version-range in
pyproject.toml. Poetry will detect this and will try to create a new
virtualenv. It by default does this in `~/.cache/poetry/virtualenvs/`,
which results in a unexcpected shared virtual environment.

poetry has an option called
[virtualenvs.in-project](https://python-poetry.org/docs/configuration/#virtualenvsin-project)
which makes sure poetry stays within the project directory. This can
already be set globally in the user configuration, but setting this for
devenv is for a future commit.
@domenkozar
Copy link
Member

One nice property that we have currently is that all state is stored inside DEVENV_STATE, could we somehow symlink .env there to comply with it?

@bobvanderlinden
Copy link
Contributor Author

One nice property that we have currently is that all state is stored inside DEVENV_STATE, could we somehow symlink .env there to comply with it?

Good suggestion 👍 That would indeed be nice.

I worry that that it doesn't work nicely with poetry. Since poetry will recreate .venv whenever it find that the python version inside .venv is not up-to-date with the current python version. It can happen that poetry will create the directory, which results in a bit of a shaky state (devenv looks at DEVENV_STATE/venv, poetry looks at .venv).

I'll check whether poetry actually recreates the .venv or whether it will automatically follow the symlink.

Currently .venv is placed in the project root. Since we want to avoid
polluting the project root and have mutable state only in $DEVENV_STATE,
it is nice to put venv there as well.

Since poetry does not support other paths other than PROJECT_ROOT/.venv,
a symlink is created that links PROJECT_ROOT/.venv to
$DEVENV_STATE/venv. This link is followed by poetry in all circumstances
that I was able to test. Even if $DEVENV_STATE/venv does not exist,
poetry will create $DEVENV_STATE/venv without overwriting the
PROJECT_ROOT/.venv symlink.
@bobvanderlinden
Copy link
Contributor Author

I must admit that I was a bit skeptic about the .venv symlink, thinking that poetry wouldn't handle it properly, but I was wrong. I have moved the virtualenv to DEVENV_STATE/venv again and made sure PROJECT_ROOT/.venv always links to DEVENV_STATE/venv. This even works when DEVENV_STATE/venv is gone and poetry wants to recreate the venv: it'll do so in DEVENV_STATE/venv without overwriting the PROJECT_ROOT/.venv symlink 👍

Thanks for the suggestion, this makes things quite a bit better 👍

@domenkozar domenkozar merged commit 85dd8ca into cachix:main Apr 11, 2023
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants