Sessions using Python2.7 cannot reuse virtual envs (-r) as Python 2.7 doesn't have attr sys.base_prefix

[crwilcox]

Describe the bug
A recent change (2021.6.6) caused reuse of virtual environments targeting older python versions (2.7, likely pre 3.3 as well?) from executing

How to reproduce

❯ nox -r -s system-2.7
nox > Running session system-2.7
nox > Command python2.7 -c import sys; print(getattr(sys, 'real_prefix', sys.base_prefix)) failed with exit code 1:
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: 'module' object has no attribute 'base_prefix'
nox > Session system-2.7 failed.

NOTE: the session will work if -r is not specified.

Expected behavior
I would expect it to reuse the existing virtual environment.

[crwilcox]

I suspect that this may need to also fall back to sys.prefix to support sessions with older interpreters?
https://github.com/theacodes/nox/pull/425/files#diff-c7026fde4100b6dc8f4450d13eab8b0da0133179ce24cca5a482ecb18c23ff0cR337

[cjolowicz]

I suspect that this may need to also fall back to sys.prefix to support sessions with older interpreters?

It would fix the crash, but the reuse would never happen:

$ /tmp/venv/bin/python -c "import sys; print(sys.prefix)"
/tmp/venv

$ python2.7 -c "import sys; print(sys.prefix)"
/usr

So the resolved interpreter and the interpreter in the environment would be considered different.

See also #441 (comment)

[cjolowicz]

We could fall back to reading base-prefix from the pyvenv.cfg file in the environment. This method requires virtualenv >= 20, but the older virtualenvs set sys.real_prefix instead (which we're already checking), so we'd be covered in all cases.

[cjolowicz]

In light of this issue and #441, we may want to consider a bugfix release with the staleness check backed out (#418, #425, #428).

@theacodes @crwilcox what do you think?

To recap, the following issues have surfaced so far:

• crash when reusing Python 2 environments (Sessions using Python2.7 cannot reuse virtual envs (-r) as Python 2.7 doesn't have attr sys.base_prefix #449 = here)
• environments never reused when virtualenv selects a different interpreter than the one resolved by Nox (Issues with virtual environment reuse and multiple Python versions installed through pyenv #441)

The second of these issues arises in multiple constellations. So far we've seen two: virtualenv favoring its own interpreter over those on PATH, and an upstream bug in virtualenv's caching with pyenv.

One way to fix these issues would be the following:

• read pyvenv.cfg instead of querying the interpreter in the existing environment, fall back to sys.real_prefix for old virtualenvs and sys.base_prefix for venv-style environments
• replace the interpreter resolution in VirtualEnv by invoking virtualenv's discovery mechanism

The second change is large and would require extensive testing. We'll also have to depend on virtualenv >= 20 instead of virtualenv >= 14.

Reverting the feature would give us some time to approach this carefully.

[theacodes]

I'm fine with that.

…

On Sat, Jun 12, 2021, 5:58 AM Claudio Jolowicz ***@***.***> wrote: In light of this issue and #441 <#441>, we may want to consider a bugfix release with the staleness check backed out (#418 <#418>, #425 <#425>, #428 <#428>). @theacodes <https://github.com/theacodes> @crwilcox <https://github.com/crwilcox> what do you think? To recap, the following issues have surfaced so far: - crash when reusing Python 2 environments (#449 <#449> = here) - environments never reused when virtualenv selects a different interpreter than the one resolved by Nox (#441 <#441>) The second of these issues arises in multiple constellations. So far we've seen two: virtualenv favoring its own interpreter over those on PATH, and an upstream bug in virtualenv's caching with pyenv. One way to fix these issues would be the following: - read pyvenv.cfg instead of querying the interpreter in the existing environment, fall back to sys.real_prefix for old virtualenvs - replace the interpreter resolution in VirtualEnv by invoking virtualenv's discovery mechanism The second change is large and would require extensive testing. We'll also have to depend on virtualenv >= 20 instead of virtualenv >= 14. Reverting the feature would give us some time to approach this carefully. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I4YPQHIGOHPSTRJQTNLTSMVV5ANCNFSM46RTT4MA> .

[cjolowicz]

@theacodes

I'm fine with that.

Cool :) So here's a thought: Can we open a branch 2021.6.x (or so) from the current release, and target the reverting PR and release PR to that? AFAICT the release automation should work fine if the tag is pushed to that branch instead of main. (Is that also true for ReadTheDocs?) The advantage would be that there's no disruption on main (revert, release, revert the revert), and the bugfix release can contain only the revert, not everything that already landed. Does that make sense?

[theacodes]

I'm actually fine with us just doing it on main. Nox doesn't move fast enough for that to be disruptive.

…

On Sat, Jun 12, 2021, 3:08 PM Claudio Jolowicz ***@***.***> wrote: @theacodes <https://github.com/theacodes> I'm fine with that. Cool :) So here's a thought: Can we open a branch 2021.6.x (or so) from the current release, and target the reverting PR and release PR to that? AFAICT the release automation should work fine if the tag is pushed to that branch instead of main. (Is that also true for ReadTheDocs?) The advantage would be that there's no disruption on main (revert, release, revert the revert), and the bugfix release can contain only the one change, not everything that already landed. Does that make sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I46CZMHIXGRQDX6AMZDTSOWD3ANCNFSM46RTT4MA> .

[cjolowicz]

It turned out that reverting the associated PRs was non-trivial, so I put it behind a feature flag instead, see #451

[crwilcox]

thanks @cjolowicz!