Repository: wntrblm/nox
Pull request #781 (2024-02-23 17:48:13)

fix: rebuild env if making an incompatible change
This is a followup to #762. Switching environments is likely to be a bit more common, so let's handle it correctly. This has a key path comparison fix that I think was stopping this from being the default before, and instead available via a secret environment variable used in tests.

This is also now "smart" about switching between compatible backends. Switching from venv to virtualenv or back won't trigger a rebuild.

@slafs, could you check this?

henryiii: Okay, enjoy! I think there are enough tests to be pretty safe if everything passes.
cjolowicz: 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.
henryiii: Yes, that’s the samefile fix above.
cjolowicz: > 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:

- #441
- https://github.com/pypa/virtualenv/issues/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.
henryiii: > 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?
cjolowicz: > (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.
henryiii: 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.
henryiii: @cjolowicz added your suggestion, feel free to change the "changes requested". :)

Patch:
diff --git a/nox/virtualenv.py b/nox/virtualenv.py
index d11db74..1895f84 100644
--- a/nox/virtualenv.py
+++ b/nox/virtualenv.py
@@ -35,7 +35,6 @@ _BLACKLISTED_ENV_VARS = frozenset(
     ["PIP_RESPECT_VIRTUALENV", "PIP_REQUIRE_VIRTUALENV", "__PYVENV_LAUNCHER__"]
 )
 _SYSTEM = platform.system()
-_ENABLE_STALENESS_CHECK = "NOX_ENABLE_STALENESS_CHECK" in os.environ
 
 
 class InterpreterNotFound(OSError):
@@ -214,9 +213,12 @@ class CondaEnv(ProcessEnv):
 
     def _clean_location(self) -> bool:
         """Deletes existing conda environment"""
+        is_conda = os.path.isdir(os.path.join(self.location, "conda-meta"))
         if os.path.exists(self.location):
-            if self.reuse_existing:
+            if self.reuse_existing and is_conda:
                 return False
+            if not is_conda:
+                shutil.rmtree(self.location)
             else:
                 cmd = [
                     self.conda_cmd,
@@ -227,9 +229,9 @@ class CondaEnv(ProcessEnv):
                     "--all",
                 ]
                 nox.command.run(cmd, silent=True, log=False)
-                # Make sure that location is clean
-                with contextlib.suppress(FileNotFoundError):
-                    shutil.rmtree(self.location)
+            # Make sure that location is clean
+            with contextlib.suppress(FileNotFoundError):
+                shutil.rmtree(self.location)
 
         return True
 
@@ -330,45 +332,78 @@ class VirtualEnv(ProcessEnv):
         self.reuse_existing = reuse_existing
         self.venv_backend = venv_backend
         self.venv_params = venv_params or []
+        if venv_backend not in {"virtualenv", "venv", "uv"}:
+            msg = f"venv_backend {venv_backend} not recognized"
+            raise ValueError(msg)
         super().__init__(env={"VIRTUAL_ENV": self.location})
 
     def _clean_location(self) -> bool:
         """Deletes any existing virtual environment"""
         if os.path.exists(self.location):
-            if self.reuse_existing and not _ENABLE_STALENESS_CHECK:
-                return False
             if (
                 self.reuse_existing
                 and self._check_reused_environment_type()
                 and self._check_reused_environment_interpreter()
             ):
                 return False
-            else:
-                shutil.rmtree(self.location)
-
+            shutil.rmtree(self.location)
         return True
 
+    def _read_pyvenv_cfg(self) -> dict[str, str] | None:
+        """Read a pyvenv.cfg file into dict, returns None if missing."""
+        path = os.path.join(self.location, "pyvenv.cfg")
+        with contextlib.suppress(FileNotFoundError), open(path) as fp:
+            parts = (x.partition("=") for x in fp if "=" in x)
+            return {k.strip(): v.strip() for k, _, v in parts}
+        return None
+
     def _check_reused_environment_type(self) -> bool:
-        """Check if reused environment type is the same."""
-        try:
-            with open(os.path.join(self.location, "pyvenv.cfg")) as fp:
-                parts = (x.partition("=") for x in fp if "=" in x)
-                config = {k.strip(): v.strip() for k, _, v in parts}
-            if "uv" in config or "gourgeist" in config:
-                old_env = "uv"
-            elif "virtualenv" in config:
-                old_env = "virtualenv"
-            else:
-                old_env = "venv"
-        except FileNotFoundError:  # pragma: no cover
-            # virtualenv < 20.0 does not create pyvenv.cfg
+        """Check if reused environment type is the same or equivalent."""
+
+        config = self._read_pyvenv_cfg()
+        # virtualenv < 20.0 does not create pyvenv.cfg
+        if config is None:
             old_env = "virtualenv"
+        elif "uv" in config or "gourgeist" in config:
+            old_env = "uv"
+        elif "virtualenv" in config:
+            old_env = "virtualenv"
+        else:
+            old_env = "venv"
+
+        # Can't detect mamba separately, but shouldn't matter
+        if os.path.isdir(os.path.join(self.location, "conda-meta")):
+            return False
 
-        return old_env == self.venv_backend
+        # Matching is always true
+        if old_env == self.venv_backend:
+            return True
+
+        # venv family with pip installed
+        if {old_env, self.venv_backend} <= {"virtualenv", "venv"}:
+            return True
+
+        # Switching to "uv" is safe, but not the other direction (no pip)
+        if old_env in {"virtualenv", "venv"} and self.venv_backend == "uv":
+            return True
+
+        return False
 
     def _check_reused_environment_interpreter(self) -> bool:
-        """Check if reused environment interpreter is the same."""
-        original = self._read_base_prefix_from_pyvenv_cfg()
+        """
+        Check if reused environment interpreter is the same. Currently only checks if
+        NOX_ENABLE_STALENESS_CHECK is set in the environment. See
+
+        * https://github.com/wntrblm/nox/issues/449#issuecomment-860030890
+        * https://github.com/wntrblm/nox/issues/441
+        * https://github.com/pypa/virtualenv/issues/2130
+        """
+        if not os.environ.get("NOX_ENABLE_STALENESS_CHECK", ""):
+            return True
+
+        config = self._read_pyvenv_cfg() or {}
+        original = config.get("base-prefix", None)
+
         program = (
             "import sys; sys.stdout.write(getattr(sys, 'real_prefix', sys.base_prefix))"
         )
@@ -384,18 +419,11 @@ class VirtualEnv(ProcessEnv):
             ["python", "-c", program], silent=True, log=False, paths=self.bin_paths
         )
 
-        return original == created
-
-    def _read_base_prefix_from_pyvenv_cfg(self) -> str | None:
-        """Return the base-prefix entry from pyvenv.cfg, if present."""
-        path = os.path.join(self.location, "pyvenv.cfg")
-        if os.path.isfile(path):
-            with open(path) as io:
-                for line in io:
-                    key, _, value = line.partition("=")
-                    if key.strip() == "base-prefix":
-                        return value.strip()
-        return None
+        return (
+            os.path.exists(original)
+            and os.path.exists(created)
+            and os.path.samefile(original, created)
+        )
 
     @property
     def _resolved_interpreter(self) -> str:


Test patch:
diff --git a/tests/test_virtualenv.py b/tests/test_virtualenv.py
index d3973b2..708de0d 100644
--- a/tests/test_virtualenv.py
+++ b/tests/test_virtualenv.py
@@ -39,6 +39,9 @@ HAS_UV = shutil.which("uv") is not None
 RAISE_ERROR = "RAISE_ERROR"
 VIRTUALENV_VERSION = metadata.version("virtualenv")
 
+has_uv = pytest.mark.skipif(not HAS_UV, reason="Missing uv command.")
+has_conda = pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+
 
 class TextProcessResult(NamedTuple):
     stdout: str
@@ -47,9 +50,16 @@ class TextProcessResult(NamedTuple):
 
 @pytest.fixture
 def make_one(tmpdir):
-    def factory(*args, **kwargs):
+    def factory(*args, venv_backend: str = "virtualenv", **kwargs):
         location = tmpdir.join("venv")
-        venv = nox.virtualenv.VirtualEnv(location.strpath, *args, **kwargs)
+        if venv_backend in {"mamba", "conda"}:
+            venv = nox.virtualenv.CondaEnv(
+                location.strpath, *args, conda_cmd=venv_backend, **kwargs
+            )
+        else:
+            venv = nox.virtualenv.VirtualEnv(
+                location.strpath, *args, venv_backend=venv_backend, **kwargs
+            )
         return (venv, location)
 
     return factory
@@ -123,6 +133,11 @@ def test_process_env_create():
         penv.create()
 
 
+def test_invalid_venv_create(make_one):
+    with pytest.raises(ValueError):
+        make_one(venv_backend="invalid")
+
+
 def test_condaenv_constructor_defaults(make_conda):
     venv, _ = make_conda()
     assert venv.location
@@ -137,7 +152,7 @@ def test_condaenv_constructor_explicit(make_conda):
     assert venv.reuse_existing is True
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_condaenv_create(make_conda):
     venv, dir_ = make_conda()
     venv.create()
@@ -166,7 +181,7 @@ def test_condaenv_create(make_conda):
     assert venv._reused
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_condaenv_create_with_params(make_conda):
     venv, dir_ = make_conda(venv_params=["--verbose"])
     venv.create()
@@ -178,7 +193,7 @@ def test_condaenv_create_with_params(make_conda):
         assert dir_.join("bin", "pip").check()
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_condaenv_create_interpreter(make_conda):
     venv, dir_ = make_conda(interpreter="3.7")
     venv.create()
@@ -192,7 +207,7 @@ def test_condaenv_create_interpreter(make_conda):
         assert dir_.join("bin", "python3.7").check()
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_conda_env_create_verbose(make_conda):
     venv, dir_ = make_conda()
     with mock.patch("nox.virtualenv.nox.command.run") as mock_run:
@@ -222,13 +237,13 @@ def test_condaenv_bin_windows(make_conda):
     ] == venv.bin_paths
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_condaenv_(make_conda):
     venv, dir_ = make_conda()
     assert not venv.is_offline()
 
 
-@pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
+@has_conda
 def test_condaenv_detection(make_conda):
     venv, dir_ = make_conda()
     venv.create()
@@ -245,7 +260,7 @@ def test_condaenv_detection(make_conda):
     assert path_regex.search(output).group("env_dir") == dir_.strpath
 
 
-@pytest.mark.skipif(not HAS_UV, reason="Missing uv command.")
+@has_uv
 def test_uv_creation(make_one):
     venv, _ = make_one(venv_backend="uv")
     assert venv.location
@@ -387,16 +402,10 @@ def test_create_reuse_environment(make_one):
     assert reused
 
 
-@pytest.fixture
-def _enable_staleness_check(monkeypatch):
-    monkeypatch.setattr("nox.virtualenv._ENABLE_STALENESS_CHECK", True)
-
-
-enable_staleness_check = pytest.mark.usefixtures("_enable_staleness_check")
-
-
-@enable_staleness_check
 def test_create_reuse_environment_with_different_interpreter(make_one, monkeypatch):
+    # Making the reuse requirement more strict
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
+
     venv, location = make_one(reuse_existing=True)
     venv.create()
 
@@ -412,37 +421,95 @@ def test_create_reuse_environment_with_different_interpreter(make_one, monkeypat
     assert not location.join("marker").check()
 
 
-@enable_staleness_check
+@has_uv
 def test_create_reuse_stale_venv_environment(make_one):
     venv, location = make_one(reuse_existing=True)
     venv.create()
 
-    # Drop a venv-style pyvenv.cfg into the environment.
+    # Drop a uv-style pyvenv.cfg into the environment.
     pyvenv_cfg = """\
     home = /usr/bin
     include-system-site-packages = false
     version = 3.9.6
+    uv = 0.1.9
     """
     location.join("pyvenv.cfg").write(dedent(pyvenv_cfg))
 
     reused = not venv.create()
 
-    # The environment is not reused because it does not look like a
-    # virtualenv-style environment.
     assert not reused
 
 
-@enable_staleness_check
-def test_create_reuse_stale_virtualenv_environment(make_one):
+def test_not_stale_virtualenv_environment(make_one, monkeypatch):
+    # Making the reuse requirement more strict
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
+
+    venv, location = make_one(reuse_existing=True, venv_backend="virtualenv")
+    venv.create()
+
+    venv, location = make_one(reuse_existing=True, venv_backend="virtualenv")
+    reused = not venv.create()
+
+    assert reused
+
+
+@has_conda
+def test_stale_virtualenv_to_conda_environment(make_one):
+    venv, location = make_one(reuse_existing=True, venv_backend="virtualenv")
+    venv.create()
+
+    venv, location = make_one(reuse_existing=True, venv_backend="conda")
+    reused = not venv.create()
+
+    # The environment is not reused because it is now conda style
+    # environment.
+    assert not reused
+
+
+@has_conda
+def test_reuse_conda_environment(make_one):
+    venv, _ = make_one(reuse_existing=True, venv_backend="conda")
+    venv.create()
+
+    venv, _ = make_one(reuse_existing=True, venv_backend="conda")
+    reused = not venv.create()
+
+    assert reused
+
+
+@pytest.mark.parametrize(
+    ("frm", "to", "result"),
+    [
+        ("virtualenv", "venv", True),
+        ("venv", "virtualenv", True),
+        ("virtualenv", "uv", True),
+        pytest.param("uv", "virtualenv", False, marks=has_uv),
+        pytest.param("conda", "virtualenv", False, marks=has_conda),
+    ],
+)
+def test_stale_environment(make_one, frm, to, result, monkeypatch):
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
+    venv, _ = make_one(reuse_existing=True, venv_backend=frm)
+    venv.create()
+
+    venv, _ = make_one(reuse_existing=True, venv_backend=to)
+    reused = venv._check_reused_environment_type()
+
+    assert reused == result
+
+
+@has_uv
+def test_create_reuse_stale_virtualenv_environment(make_one, monkeypatch):
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
     venv, location = make_one(reuse_existing=True, venv_backend="venv")
     venv.create()
 
-    # Drop a virtualenv-style pyvenv.cfg into the environment.
+    # Drop a uv-style pyvenv.cfg into the environment.
     pyvenv_cfg = """\
     home = /usr
     implementation = CPython
     version_info = 3.9.6.final.0
-    virtualenv = 20.4.6
+    uv = 0.1.9
     include-system-site-packages = false
     base-prefix = /usr
     base-exec-prefix = /usr
@@ -457,8 +524,25 @@ def test_create_reuse_stale_virtualenv_environment(make_one):
     assert not reused
 
 
-@enable_staleness_check
-def test_create_reuse_venv_environment(make_one):
+@has_uv
+def test_create_reuse_uv_environment(make_one):
+    venv, location = make_one(reuse_existing=True, venv_backend="uv")
+    venv.create()
+
+    # Place a spurious occurrence of "uv" in the pyvenv.cfg.
+    pyvenv_cfg = location.join("pyvenv.cfg")
+    pyvenv_cfg.write(pyvenv_cfg.read() + "bogus = uv\n")
+
+    reused = not venv.create()
+
+    # The environment is reused because it looks like a uv environment
+    assert reused
+
+
+def test_create_reuse_venv_environment(make_one, monkeypatch):
+    # Making the reuse requirement more strict
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
+
     venv, location = make_one(reuse_existing=True, venv_backend="venv")
     venv.create()
 
@@ -472,7 +556,6 @@ def test_create_reuse_venv_environment(make_one):
     assert reused
 
 
-@enable_staleness_check
 @pytest.mark.skipif(IS_WINDOWS, reason="Avoid 'No pyvenv.cfg file' error on Windows.")
 def test_create_reuse_oldstyle_virtualenv_environment(make_one):
     venv, location = make_one(reuse_existing=True)
@@ -491,9 +574,9 @@ def test_create_reuse_oldstyle_virtualenv_environment(make_one):
     assert reused
 
 
-@enable_staleness_check
 @pytest.mark.skipif(IS_WINDOWS, reason="Avoid 'No pyvenv.cfg file' error on Windows.")
-def test_inner_functions_reusing_venv(make_one):
+def test_inner_functions_reusing_venv(make_one, monkeypatch):
+    monkeypatch.setenv("NOX_ENABLE_STALENESS_CHECK", "1")
     venv, location = make_one(reuse_existing=True)
     venv.create()
 
@@ -506,7 +589,7 @@ def test_inner_functions_reusing_venv(make_one):
     """
     location.join("pyvenv.cfg").write(dedent(pyvenv_cfg))
 
-    base_prefix = venv._read_base_prefix_from_pyvenv_cfg()
+    base_prefix = venv._read_pyvenv_cfg()["base-prefix"]
     assert base_prefix == "foo"
 
     reused_interpreter = venv._check_reused_environment_interpreter()
@@ -514,7 +597,6 @@ def test_inner_functions_reusing_venv(make_one):
     assert not reused_interpreter
 
 
-@enable_staleness_check
 @pytest.mark.skipif(
     version.parse(VIRTUALENV_VERSION) >= version.parse("20.22.0"),
     reason="Python 2.7 unsupported for virtualenv>=20.22.0",