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

ansible-test - prefer venv over virtualenv on Python 3 #73000

Merged
merged 1 commit into from Dec 17, 2020

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Dec 16, 2020

SUMMARY

Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since some systems have a non-functional venv installation (such as Debian).

Supersedes #72778
Fixes: #72738

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

changelogs/fragments/ansible-test-venv-virtualenv-fallback.yml
test/lib/ansible_test/_data/injector/virtualenv-isolated.sh
test/lib/ansible_test/_data/injector/virtualenv.sh
test/lib/ansible_test/_data/setup/remote.sh

@ansibot ansibot added affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. python3 support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Dec 16, 2020
@samdoran samdoran changed the title ansible-test - preferer venv over virtualenv on Python 3 ansible-test - prefer venv over virtualenv on Python 3 Dec 16, 2020
Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed
anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since
some systems have a non-functional venv installation (such as Debian).
@samdoran
Copy link
Contributor Author

@felixfontein @dmsimard Can you try out this change and see if it resolves the problems you were having with virtualenv.sh?

@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Dec 16, 2020
@webknjaz
Copy link
Member

@samdoran what is the rationale for this change? Third-party virtualenv is often better for py3 too: it's faster starting v20.

@mattclay
Copy link
Member

@webknjaz ansible-test favors venv over virtualenv on Python 3 already. This change just updates the virtualenv scripts to follow the same behavior. Using venv avoids additional requirements needed by virtualenv>=20 which complicates management of the test environment.

@webknjaz
Copy link
Member

Oh, I thought it'd be nice to make use of the new plugin APIs virtualenv now provides one day. Like having custom seeders.

@felixfontein
Copy link
Contributor

@samdoran seems to work, at least I was able to use your branch and remove the installation of virtualenv in community.hashi_vault and CI still passes: ansible-collections/community.hashi_vault#30

@samdoran
Copy link
Contributor Author

@webknjaz For Python 3.9, we need to move to a new version of virtualenv (>20) since <20 was failing tests in Python 3.9. virtualenv >20 brings in distlib, filelock, six, and appdirs as deps whereas virtualenv <20 had no deps. Additional deps bring in more potential instability (we'd have to pin more deps and update them as things break).

All that is why we would rather just use venv if it is available and get away from installing virtualenv during remote instance creation.

@samdoran samdoran merged commit 850a77f into ansible:devel Dec 17, 2020
@samdoran samdoran deleted the ci/prefer-venv-over-virtualenv branch December 17, 2020 15:51
@felixfontein
Copy link
Contributor

@samdoran any chance of this being backported to stable-2.9 and stable-2.10? (We'd need that to get rid of the hack :) )

@samdoran
Copy link
Contributor Author

I don't think so since this is a change in behavior.

@mattclay
Copy link
Member

mattclay commented Jan 8, 2021

@felixfontein @samdoran Perhaps we could do one of the following to improve the transition while maintaining backwards compatibility?

  1. Require setting an environment variable to opt-in to the new behavior. This would only be needed for the backports.
  2. Provide another version of the scripts (such as a _v2 suffixed) that would provide the new behavior. This would be needed in all supported branches.

I think the first option is the most appealing, since it wouldn't require a separate script and the additional logic would only be needed for the backports.

@mattclay
Copy link
Member

mattclay commented Jan 8, 2021

The more I think about this, the more I'm wondering if there really is much of a backwards compatibility concern here. Yes, we're using a different tool to create the virtual environment, but it should provide the same behavior in the end.

If a virtual environment was created before, it should still be. If it was failing to create a virtual environment before, then the test would already be failing -- making it pass shouldn't be a problem.

@felixfontein
Copy link
Contributor

As long as it suffices to set the env variable in the test scripts (and not when calling ansible-test), that's fine for me. But your new argument sounds reasonable to me as well.

mattclay pushed a commit to mattclay/ansible that referenced this pull request Jan 8, 2021
Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed
anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since
some systems have a non-functional venv installation (such as Debian).

(cherry picked from commit 850a77f)
mattclay pushed a commit to mattclay/ansible that referenced this pull request Jan 8, 2021
Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed
anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since
some systems have a non-functional venv installation (such as Debian).

(cherry picked from commit 850a77f)
(cherry picked from commit a48b3d2)
@mattclay
Copy link
Member

mattclay commented Jan 8, 2021

@felixfontein I've opened backports for stable-2.10 and stable-2.9:

After discussing with @relrod we decided to go with opt-in behavior, just to be safe. To opt-in to the new behavior in those branches, tests will need to set the ANSIBLE_TEST_PREFER_VENV environment variable. So something like this would work:

export ANSIBLE_TEST_PREFER_VENV=1
source virtualenv.sh

Whereas before the tests simply used:

source virtualenv.sh

This should allow a single test configuration to work across 2.9, 2.10 and devel once the backports are merged.

relrod pushed a commit that referenced this pull request Jan 8, 2021
* ansible-test - prefer venv over virtualenv on Python 3 (#73000)

Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed
anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since
some systems have a non-functional venv installation (such as Debian).

(cherry picked from commit 850a77f)

* Make the new ansible-test venv behavior opt-in

Co-authored-by: Sam Doooran <sdoran@redhat.com>
relrod pushed a commit that referenced this pull request Jan 8, 2021
* ansible-test - prefer venv over virtualenv on Python 3 (#73000)

Also pin virtualenv to 16.7.10 for older Mac OS X systems. This was the version being installed
anway with the previous constraint (<20).

On systems with Python 3, now prefer venv over virtualenv. Test to see if venv is functional since
some systems have a non-functional venv installation (such as Debian).

(cherry picked from commit 850a77f)
(cherry picked from commit a48b3d2)

* Make the new ansible-test venv behavior opt-in

Co-authored-by: Sam Doooran <sdoran@redhat.com>
felixfontein added a commit to felixfontein/community.hashi_vault that referenced this pull request Jan 9, 2021
@felixfontein
Copy link
Contributor

@mattclay great, thanks! I've tried it out in ansible-collections/community.hashi_vault#45, works perfectly!

briantist pushed a commit to ansible-collections/community.hashi_vault that referenced this pull request Jan 9, 2021
@mattclay
Copy link
Member

@felixfontein Looks good. I recommend including a comment when setting the env var that it is only needed while supporting 2.9 and 2.10, so that it will be more obvious in the future when it can be removed.

@ansible ansible locked and limited conversation to collaborators Jan 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. python3 support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ansible-test default container does not have virtualenv installed
5 participants