@henryiii sorry, won't be able to, until at least Tuesday. I'm on vacay without a laptop.

Okay, enjoy! I think there are enough tests to be pretty safe if everything passes.

Didn't that environment variable also control whether we check the interpreter version before reuse? IIRC that logic was subtly broken, which is why we put it behind the environment variable in the first place, until someone could come fix it.

For context: #449 (comment)

If this fixes the issue for good, 🎉 :)

Yes, that’s the os.path.samefile fix above. The two ways of computing the interpreter path don’t textually compare, but they are logically the same path. And it’s tested (that it is not recreated if unchanged) now too.

Yes, that’s the os.path.samefile fix above.

I'm not sure I understand how that addresses the issues mentioned in the comment linked above? Specifically, these two:

• Issues with virtual environment reuse and multiple Python versions installed through pyenv #441
• Cache invalidation broken with pyenv? pypa/virtualenv#2130

These issues result in environments never being reused because Nox and virtualenv disagree on which interpreter should be selected. Nox does a PATH lookup, while virtualenv favors the interpreter it was installed on, and sometimes uses stale entries from its cache because it doesn't understand pyenv's shims.

These issues result in environments never being reused because Nox and virtualenv disagree on which interpreter should be selected

I’ll check, but what I was seeing was that the simple comparison was always failing due to one being resolved differently than the other, but they were the same file. So it was always recreating for me, and the fix above fixed it. I have pyenv but not using it here, so might have missed that case.

We do require virtualenv 20+ now IIRC, so we could follow the suggestion there and use its discovery system, I think?

(I can also just keep the interp check behind a flag)

(I can also just keep the interp check behind a flag)

Yeah, this way we can land the environment type check right away and profit :)

I'm all for tackling the interpreter check as well, but I feel it needs a bit of thought.

Unrelated, but we have a logging issue in the tests. On a test failure (possibly only in the correct spot) we get tons of logging file closed errors.

@cjolowicz added your suggestion, feel free to change the "changes requested". :)

Didn't realize I could do it - okay, going from the LGTM, going to merge and work on the final followup!