fix: install layer dependencies only (#145)

With the idea of adding to the PYTHONPATH, each venv layer only needs to
install its packages inside the venv instance, thus reducing the number
of packages that are installed in each venv instance. We still use the
full package spec in the result report for easier identification of all
the dependencies used.

BREAKING CHANGE: this change implies that package declarations that are
used for pinning dependencies will have to be added to each venv layer
that requires them.

releasenotes/notes/fix-sitepkgs-3ef98bad9dc20f70.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixed handling of environment variables, package installations and namespace
+    packages.

riot/riot.py

@@ -106,7 +106,7 @@ def bin_path(self) -> t.Optional[str]:
         return os.path.join(self.venv_path, "bin")
 
     @property
-    def site_packages_path(self) -> t.Optional[str]:
+    def site_packages_path(self) -> str:
         version = ".".join((str(_) for _ in self.version_info()[:2]))
         return os.path.join(self.venv_path, "lib", f"python{version}", "site-packages")
 
@@ -210,13 +210,18 @@ def instances(
         parent_inst: t.Optional["VenvInstance"] = None,
     ) -> t.Generator["VenvInstance", None, None]:
         # Expand out the instances for the venv.
-        for env in expand_specs(self.env):
+        for env_spec in expand_specs(self.env):
+            # Bubble up env
+            env = parent_inst.env.copy() if parent_inst else {}
+            env.update(dict(env_spec))
+
             # Bubble up pys
             pys = (
                 self.pys
                 or (parent.pys if parent else None)
                 or [parent_inst.py if parent_inst else None]
             )
+
             for py in pys:
                 for pkgs in expand_specs(self.pkgs):
                     inst = VenvInstance(
@@ -226,7 +231,7 @@ def instances(
                         or (parent_inst.command if parent_inst else None),
                         py=py,
                         env=env,
-                        pkgs=pkgs,
+                        pkgs=dict(pkgs),
                         parent=parent_inst,
                     )
                     if not self.venvs:
@@ -238,9 +243,9 @@ def instances(
 
 @dataclasses.dataclass
 class VenvInstance:
-    pkgs: t.Tuple[t.Tuple[str, str]]
+    pkgs: t.Dict[str, str]
     py: Interpreter
-    env: t.Tuple[t.Tuple[str, str]]
+    env: t.Dict[str, str]
     name: t.Optional[str] = None
     command: t.Optional[str] = None
     parent: t.Optional["VenvInstance"] = None
@@ -258,7 +263,8 @@ def venv_path(self) -> t.Optional[str]:
         venv_path = self.py.venv_path
         if self.needs_venv:
             venv_path = "_".join(
-                [venv_path] + [f"{n}{rmchars('<=>.,', v)}" for n, v in self.pkgs]
+                [venv_path]
+                + [f"{n}{rmchars('<=>.,', v)}" for n, v in self.pkgs.items()]
             )
         return venv_path
 
@@ -275,23 +281,22 @@ def needs_venv(self) -> bool:
     @property
     def pkg_str(self) -> str:
         """Return pip friendly install string from defined packages."""
-        chain: t.List[VenvInstance] = []
+        return pip_deps(self.pkgs)
+
+    @property
+    def full_pkg_str(self) -> str:
+        """Return pip friendly install string from defined packages."""
+        chain: t.List[VenvInstance] = [self]
         current: t.Optional[VenvInstance] = self
         while current is not None:
-            chain.append(current)
+            chain.insert(0, current)
             current = current.parent
 
         pkgs: t.Dict[str, str] = {}
         for inst in chain:
             pkgs.update(dict(inst.pkgs))
 
-        return " ".join(
-            [
-                f"'{get_pep_dep(lib, version)}'"
-                for lib, version in pkgs.items()
-                if version is not None
-            ]
-        )
+        return pip_deps(pkgs)
 
     @property
     def bin_path(self) -> t.Optional[str]:
@@ -326,8 +331,13 @@ def site_packages_path(self) -> t.Optional[str]:
         return os.path.join(target, "lib", f"python{version}", "site-packages")
 
     @property
-    def pythonpath(self) -> str:
-        paths = []
+    def site_packages_list(self) -> t.List[str]:
+        """Return a list of all the site-packages paths along the parenting relation.
+
+        The list starts with the empty string and is followed by the site-packages path
+        of the current instance, then the parent site-packages paths follow.
+        """
+        paths = ["", os.getcwd()]  # mimick 'python -m'
 
         current: t.Optional[VenvInstance] = self
         while current is not None:
@@ -340,10 +350,11 @@ def pythonpath(self) -> str:
             if self.py.site_packages_path is not None:
                 paths.append(self.py.site_packages_path)
 
-        # Finally add the CWD so that we can, e.g., find and run tests without 'python -m'
-        paths.append(os.getcwd())
+        return paths
 
-        return ":".join(paths)
+    @property
+    def pythonpath(self) -> str:
+        return ":".join(self.site_packages_list)
 
     def prepare(
         self, env: t.Dict[str, str], py: t.Optional[Interpreter] = None
@@ -380,7 +391,11 @@ def prepare(
                     )
                 else:
                     if self.pkg_str:
-                        logger.info("Installing venv dependencies %s.", self.pkg_str)
+                        logger.info(
+                            "Installing venv dependencies %s in prefix %s.",
+                            self.pkg_str,
+                            self.target,
+                        )
                         try:
                             Session.run_cmd_venv(
                                 venv_path,
@@ -512,8 +527,6 @@ def run(
                 else:
                     raise
 
-            logger.info("Running with %s", inst.py)
-
             venv_path = inst.venv_path
             assert venv_path is not None
 
@@ -523,9 +536,11 @@ def run(
                 )
                 continue
 
+            logger.info("Running with %s", inst.py)
+
             # Result which will be updated with the test outcome.
             result = VenvInstanceResult(
-                instance=inst, venv_name=venv_path, pkgstr=inst.pkg_str
+                instance=inst, venv_name=venv_path, pkgstr=inst.full_pkg_str
             )
 
             # Generate the environment for the instance.
@@ -556,19 +571,54 @@ def run(
                 assert command is not None
                 if cmdargs is not None:
                     command = command.format(cmdargs=(" ".join(cmdargs))).strip()
-                env_str = " ".join(f"{k}={v}" for k, v in env.items())
+                env_str = "\n".join(f"{k}={v}" for k, v in env.items())
                 logger.info(
-                    "Running command '%s' with environment '%s'.", command, env_str
+                    "Running command '%s' in venv '%s' with environment:\n%s.",
+                    command,
+                    venv_path,
+                    env_str,
                 )
+                # Copy over *nspkg.pth files so that we can handle namespace packages.
+                # This is strictly required by Python 2.7 and <3.3 (see PEP 420).
+                ns_files = []
                 try:
-                    # Pipe the command output directly to `out` since we
-                    # don't need to store it.
+                    sitepkgs = inst.site_packages_path
+                    assert sitepkgs is not None, inst
+                    for path in (_ for _ in inst.site_packages_list if _ != sitepkgs):
+                        if path:
+                            try:
+                                for ns in (
+                                    _
+                                    for _ in os.listdir(path)
+                                    if _.endswith("nspkg.pth")
+                                ):
+                                    ns_dest = os.path.join(sitepkgs, ns)
+                                    if os.path.isfile(ns_dest):
+                                        # child overrides parent
+                                        continue
+                                    ns_src = os.path.join(path, ns)
+                                    with open(ns_src) as ns_in:
+                                        # https://github.com/pypa/setuptools/blob/b62705a84ab599a2feff059ececd33800f364555/setuptools/namespaces.py#L44
+                                        content = ns_in.read().replace(
+                                            "sys._getframe(1).f_locals['sitedir']",
+                                            f"'{path}'",
+                                        )
+                                        with open(ns_dest, "w") as ns_out:
+                                            ns_out.write(content)
+
+                                    ns_files.append(ns_dest)
+                            except FileNotFoundError:
+                                pass
                     output = self.run_cmd_venv(venv_path, command, stdout=out, env=env)
                     result.output = output.stdout
                 except CmdFailure as e:
                     raise CmdFailure(
                         f"Test failed with exit code {e.proc.returncode}", e.proc
                     )
+                finally:
+                    # We need to clean up as some layers might be shared.
+                    for ns in ns_files:
+                        os.remove(ns)
             except CmdFailure as e:
                 result.code = e.code
                 click.echo(click.style(e.msg, fg="red"))
@@ -628,9 +678,7 @@ def list_venvs(self, pattern, venv_pattern, pythons=None, out=sys.stdout):
 
             if not venv_pattern.search(inst.venv_path):
                 continue
-            pkgs_str = " ".join(
-                f"'{get_pep_dep(name, version)}'" for name, version in inst.pkgs
-            )
+            pkgs_str = inst.full_pkg_str
             env_str = env_to_str(inst.env)
             py_str = f"Python {inst.py}"
             click.echo(f"{inst.name} {env_str} {py_str} {pkgs_str}")
@@ -731,15 +779,15 @@ def get_pep_dep(libname: str, version: str) -> str:
     return f"{libname}{version}"
 
 
-def env_to_str(envs: t.Sequence[t.Tuple[str, str]]) -> str:
+def env_to_str(envs: t.Dict[str, str]) -> str:
     """Return a human-friendly representation of environment variables.
 
-    >>> env_to_str([("FOO", "BAR")])
+    >>> env_to_str({"FOO": "BAR"})
     'FOO=BAR'
-    >>> env_to_str([("K", "V"), ("K2", "V2")])
+    >>> env_to_str({"K": "V", "K2": "V2"})
     'K=V K2=V2'
     """
-    return " ".join(f"{k}={v}" for k, v in envs)
+    return " ".join(f"{k}={v}" for k, v in envs.items())
 
 
 def run_cmd(
@@ -800,3 +848,13 @@ def expand_specs(specs: t.Dict[_K, t.List[_V]]) -> t.Iterator[t.Tuple[t.Tuple[_K
 
     # Need to cast because the * star typeshed of itertools.product returns Any
     return t.cast(t.Iterator[t.Tuple[t.Tuple[_K, _V]]], itertools.product(*all_vals))
+
+
+def pip_deps(pkgs: t.Dict[str, str]) -> str:
+    return " ".join(
+        [
+            f"'{get_pep_dep(lib, version)}'"
+            for lib, version in pkgs.items()
+            if version is not None
+        ]
+    )

tests/test_cli.py

@@ -67,6 +67,8 @@ def test_list_with_venv_pattern(cli: click.testing.CliRunner) -> None:
                 "pytest543$",
             ],
         )
+        if result.exception:
+            raise result.exception
         assert result.exit_code == 0, result.stdout
         assert result.stdout == "test  Python Interpreter(_hint='3') 'pytest==5.4.3'\n"
 
@@ -317,7 +319,7 @@ def test_run_suites_cmdargs(
             subprocess_run.return_value.returncode = 0
             args = ["run", name] + cmdargs
             result = cli.invoke(riot.cli.main, args, catch_exceptions=False)
-            assert result.exit_code == 0
+            assert result.exit_code == 0, result.stdout
 
             subprocess_run.assert_called()
 

tests/test_integration.py

@@ -555,6 +555,8 @@ def test_interpreter_pythonpath(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> N
     py_dot_version = ".".join((str(_) for _ in sys.version_info[:2]))
     assert env["PYTHONPATH"] == ":".join(
         (
+            "",
+            str(tmp_path),
             str(
                 tmp_path
                 / os.path.join(
@@ -565,7 +567,6 @@ def test_interpreter_pythonpath(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> N
                     "site-packages",
                 )
             ),
-            str(tmp_path),
         )
     )
 
@@ -628,7 +629,7 @@ def test_venv_instance_pythonpath(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) ->
     )
 
     paths = env["PYTHONPATH"].split(":")
-    assert paths == [venv_path, parent_venv_path, py_venv_path, str(tmp_path)]
+    assert paths == ["", str(tmp_path), venv_path, parent_venv_path, py_venv_path]
 
 
 def test_venv_instance_path(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
@@ -647,8 +648,7 @@ def test_venv_instance_path(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
     )
     result = tmp_run("riot run -s test")
     env = dict(_.split("=") for _ in result.stdout.splitlines() if "=" in _)
-    print(result.stderr)
-    assert result.returncode == 0
+    assert result.returncode == 0, result.stderr
 
     venv_name = "venv_py{}_pytest".format(
         "".join((str(_) for _ in sys.version_info[:3]))
@@ -677,3 +677,28 @@ def test_venv_instance_path(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
 
     paths = env["PATH"].split(":")
     assert paths.index(venv_path) < paths.index(parent_venv_path)
+
+
+def test_env_bubble_up(tmp_path: pathlib.Path, tmp_run: _T_TmpRun) -> None:
+    rf_path = tmp_path / "riotfile.py"
+    rf_path.write_text(
+        """
+from riot import Venv
+venv = Venv(
+    pys=[3],
+    pkgs={"pip": ""},
+    name="test",
+    command="env",
+    env={"foo": "42", "baz": "stillthesame"},
+    venvs=[Venv(pkgs={"pytest": ""}, env={"bar": "43", "foo": "40"})],
+)
+""",
+    )
+    result = tmp_run("riot -v run -s test")
+    print(result.stdout)
+    env = dict(_.split("=", maxsplit=1) for _ in result.stdout.splitlines() if "=" in _)
+    assert result.returncode == 0, (result.stdout, result.stderr)
+
+    assert env["foo"] == "40"
+    assert env["bar"] == "43"
+    assert env["baz"] == "stillthesame"

tests/test_unit.py

@@ -56,9 +56,9 @@ def test_interpreter_venv_path(current_interpreter: Interpreter) -> None:
 def test_venv_instance_venv_path(current_interpreter: Interpreter) -> None:
     venv = VenvInstance(
         command="echo test",
-        env=(("env", "test"),),
+        env={"env": "test"},
         name="test",
-        pkgs=(("pip", ""),),
+        pkgs={"pip": ""},
         py=current_interpreter,
     )