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

installer: use venv and improve error handling #4099

Merged

Conversation

abn
Copy link
Member

@abn abn commented May 24, 2021

This change drops the dependency on virtualenv for the installer,
replacing environment creation with the built-in venv module available
in Python >= 3.2.

In addition, this change also improves error handling for subprocess
commands.

The following behavioural changes are also introduced.

  • An error log is written to a secure temporary file with stdout and
    tracebacks on error.
  • If an existing virtual environment exists prior to installation, this
    is saved, and used for recovery if installation fails.

Resolves: #4093
Resolves: #4089
Resolves: #4195
Resolves: #4463

@abn abn requested a review from a team May 24, 2021 22:08
@abn
Copy link
Member Author

abn commented May 24, 2021

cc: @sdispater

@abn abn added the Installer label May 24, 2021
@sdispater
Copy link
Member

@abn Won't that be an issue on Debian-based system which, as far as I recall, do not have the python-venv package installed?

@abn
Copy link
Member Author

abn commented Jun 4, 2021

I thought this was now a built-in on there as well. Will check. We can do a fall back. But think now there is also install-virtualenv.

@neersighted
Copy link
Member

I thought this was now a built-in on there as well. Will check. We can do a fall back. But think now there is also install-virtualenv.

Pretty sure this is a non-issue. The python3.7-venv package just contains the pyvenv-3.7 wrapper -- it looks like the venv module belongs to the libpython3.7-stdlib package: https://packages.debian.org/buster/amd64/libpython3.7-stdlib/filelist

neersighted
neersighted previously approved these changes Nov 1, 2021
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Looks good to me! I have some minor (very minor) nitpicks, but this nicely solves the issue I was having and eliminates the complexity of shelling out to virtualenv!

install-poetry.py Outdated Show resolved Hide resolved
install-poetry.py Show resolved Hide resolved
neersighted
neersighted previously approved these changes Nov 10, 2021
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I've rebased this/fixed the merge conflicts and implemented the suggestions I made before (discussed offline with @abn) -- this is ready to merge assuming the checks pass.

Since #4666 was landed in master this will be subject to full integration test coverage.

@neersighted neersighted self-assigned this Nov 11, 2021
@neersighted neersighted force-pushed the improve-installer-error-handling branch 3 times, most recently from 0130019 to 1b48f0e Compare November 11, 2021 09:38
neersighted
neersighted previously approved these changes Nov 11, 2021
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I've rebased this onto master, and squashed my contributions. We're now testing green across the board, with only one weird hack/workaround (can't call Subprocess with a Path on Windows).

For posterity, we were struggling with macOS + Python 3.10 -- it ended up being an issue with blanking the environment. Omitting that eliminates a whole class of weirdness -- we can always revisit if needed.

@neersighted
Copy link
Member

@abn Won't that be an issue on Debian-based system which, as far as I recall, do not have the python-venv package installed?

It turns out I was wrong and we do end up with an error -- however, since Ubuntu/Debian monkey-patch Python, it's quite readable and is not a blocker:

Retrieving Poetry metadata

# Welcome to Poetry!

This will download and install the latest version of Poetry,
a dependency and package manager for Python.

It will add the `poetry` command to Poetry's bin directory, located at:

/home/ubuntu/.local/bin

You can uninstall at any time by executing this script with the --uninstall option,
and these changes will be reverted.

Installing Poetry (1.1.11): Creating environment
The virtual environment was not created successfully because ensurepip is not
available.  On Debian/Ubuntu systems, you need to install the python3-venv
package using the following command.

    apt install python3.8-venv

You may need to use sudo with that command.  After installing the python3-venv
package, recreate your virtual environment.

Failing command: ['/home/ubuntu/.local/share/pypoetry/venv/bin/python3', '-Im', 'ensurepip', '--upgrade', '--default-pip']

@abn
Copy link
Member Author

abn commented Nov 11, 2021

I have a wip fix for that coming as soon as I fix merge conflicts from master.

We'll be falling back to a virtualenv zipapp.

@abn abn marked this pull request as draft November 12, 2021 01:21
@abn abn force-pushed the improve-installer-error-handling branch 2 times, most recently from 3487d4b to d7f5c90 Compare November 12, 2021 02:37
@abn abn marked this pull request as ready for review November 12, 2021 02:39
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Looks good -- the best fusion of both our changes. I'd like to fix that redundant return cls(target) and merge this!

abn and others added 2 commits November 12, 2021 03:48
This change refactors virtual environment creation and introduces a new
wrapper to handle command execution etc. If venv module is not
available, in cases like that of the ubuntu distribution where the
module is not part of the default python install, the virtualenv
package is used by fetching the appropriate zipapp for the package for
the python runtime.

Resolves: python-poetry#4089
Resolves: python-poetry#4093
This change ensures that if creation of a new environment fails, any
previously existing environment is restored.
@abn abn force-pushed the improve-installer-error-handling branch from d7f5c90 to 0419ed2 Compare November 12, 2021 02:48
@abn abn requested a review from neersighted November 12, 2021 02:49
@neersighted neersighted merged commit 8dc83b3 into python-poetry:master Nov 12, 2021
1nF0rmed pushed a commit to 1nF0rmed/poetry that referenced this pull request Nov 15, 2021
* installer: improve error handling and logging

Co-authored-by: Bjorn Neergaard <[email protected]>

* installer: default to using in-built venv module

This change refactors virtual environment creation and introduces a new
wrapper to handle command execution etc. If venv module is not
available, in cases like that of the ubuntu distribution where the
module is not part of the default python install, the virtualenv
package is used by fetching the appropriate zipapp for the package for
the python runtime.

Resolves: python-poetry#4089
Resolves: python-poetry#4093

* ci/installer: upload failure logs

* installer: preserve existing environment on failure

This change ensures that if creation of a new environment fails, any
previously existing environment is restored.

Co-authored-by: Bjorn Neergaard <[email protected]>
@abn abn deleted the improve-installer-error-handling branch November 18, 2021 10:37
edvardm pushed a commit to edvardm/poetry that referenced this pull request Nov 24, 2021
* installer: improve error handling and logging

Co-authored-by: Bjorn Neergaard <[email protected]>

* installer: default to using in-built venv module

This change refactors virtual environment creation and introduces a new
wrapper to handle command execution etc. If venv module is not
available, in cases like that of the ubuntu distribution where the
module is not part of the default python install, the virtualenv
package is used by fetching the appropriate zipapp for the package for
the python runtime.

Resolves: python-poetry#4089
Resolves: python-poetry#4093

* ci/installer: upload failure logs

* installer: preserve existing environment on failure

This change ensures that if creation of a new environment fails, any
previously existing environment is restored.

Co-authored-by: Bjorn Neergaard <[email protected]>
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants