Pickup bootstrap site-packages location from sysconfig purelib path (#1211)

Hi,

could you please review patch to pickup the boostrap site-packages
location from the `sysconfig` `purelib` path. It fixes #1208.

The bootstrap options and code now operate on a single path instead of a
list, likely because `getsitepackages` previously returned a list of
paths, which is no longer relevant (unless there's another reason I'm
missing).

I've also updated the `bootstrap` command to print the file's creation
path, which I find useful for awareness.

For testing, I couldn't think of a case other than creating a virtual
environment to test the `bootstrap` within it. I implemented a minimal
solution using the `pytest_virtualenv` plugin, which provides a fixture
for this. Since creating a Basilisp environment takes 20+ seconds, I
added a pytest decorator to run it only on GitHub CI. Let me know if you
have other testing suggestions or if it's better to leave this out
altogether. I consider these type of tests as part of integration
testing (which ideally run as a separate command/job), but we don't have
that setup (yet?).

Thanks

---------

Co-authored-by: ikappaki <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed 
  * Fix a regression introduced in #1176 where the testrunner couldn't handle relative paths in `sys.path`, causing `basilisp test` to fail when no arugments were provided (#1204)
  * Fix a bug where `basilisp.process/exec` could deadlock reading process output if that output exceeded the buffer size (#1202)
+ * Fix `basilisp boostrap` issue on MS-Windows where the boostrap file loaded too early, before Basilisp was in `sys.path` (#1208)
 
 ## [v0.3.6]
 ### Added

src/basilisp/cli.py

@@ -425,11 +425,12 @@ def bootstrap_basilisp_installation(_, args: argparse.Namespace) -> None:
         ):
             print_("No Basilisp bootstrap files were found.")
         else:
-            for file in removed:
-                print_(f"Removed '{file}'")
+            if removed is not None:
+                print_(f"Removed '{removed}'")
     else:
-        basilisp.bootstrap_python(site_packages=args.site_packages)
+        path = basilisp.bootstrap_python(site_packages=args.site_packages)
         print_(
+            f"(Added {path})\n\n"
             "Your Python installation has been bootstrapped! You can undo this at any "
             "time with with `basilisp bootstrap --uninstall`."
         )
@@ -473,7 +474,6 @@ def _add_bootstrap_subcommand(parser: argparse.ArgumentParser) -> None:
     # Not intended to be used by end users.
     parser.add_argument(
         "--site-packages",
-        action="append",
         help=argparse.SUPPRESS,
     )
 

src/basilisp/main.py

@@ -1,5 +1,6 @@
 import importlib
-import site
+import os
+import sysconfig
 from pathlib import Path
 from typing import Optional
 
@@ -56,42 +57,41 @@ def bootstrap(
         getattr(mod, fn_name)()
 
 
-def bootstrap_python(site_packages: Optional[list[str]] = None) -> None:
-    """Bootstrap a Python installation by installing a ``.pth`` file in the first
-    available ``site-packages`` directory (as by
-    :external:py:func:`site.getsitepackages`).
+def bootstrap_python(site_packages: Optional[str] = None) -> str:
+    """Bootstrap a Python installation by installing a ``.pth`` file
+    in ``site-packages`` directory (corresponding to "purelib" in
+    :external:py:func:`sysconfig.get_paths`). Returns the path to the
+    ``.pth`` file.
 
     Subsequent startups of the Python interpreter will have Basilisp already
     bootstrapped and available to run."""
     if site_packages is None:  # pragma: no cover
-        site_packages = site.getsitepackages()
+        site_packages = sysconfig.get_paths()["purelib"]
 
-    assert site_packages, "Expected at least one site-package directory"
+    assert site_packages, "Expected a site-package directory"
 
-    for d in site_packages:
-        p = Path(d)
-        with open(p / "basilispbootstrap.pth", mode="w") as f:
-            f.write("import basilisp.sitecustomize")
-        break
+    pth = Path(site_packages) / "basilispbootstrap.pth"
+    with open(pth, mode="w") as f:
+        f.write("import basilisp.sitecustomize")
 
+    return str(pth)
 
-def unbootstrap_python(site_packages: Optional[list[str]] = None) -> list[str]:
-    """Remove any `basilispbootstrap.pth` files found in any Python site-packages
-    directory (as by :external:py:func:`site.getsitepackages`). Return a list of
-    removed filenames."""
+
+def unbootstrap_python(site_packages: Optional[str] = None) -> Optional[str]:
+    """Remove the `basilispbootstrap.pth` file found in the Python site-packages
+    directory (corresponding to "purelib" in :external:py:func:`sysconfig.get_paths`).
+    Return the path of the removed file, if any."""
     if site_packages is None:  # pragma: no cover
-        site_packages = site.getsitepackages()
-
-    assert site_packages, "Expected at least one site-package directory"
-
-    removed = []
-    for d in site_packages:
-        p = Path(d)
-        for file in p.glob("basilispbootstrap.pth"):
-            try:
-                file.unlink()
-            except FileNotFoundError:  # pragma: no cover
-                pass
-            else:
-                removed.append(str(file))
+        site_packages = sysconfig.get_paths()["purelib"]
+
+    assert site_packages, "Expected a site-package directory"
+
+    removed = None
+    pth = Path(site_packages) / "basilispbootstrap.pth"
+    try:
+        os.unlink(pth)
+    except FileNotFoundError:  # pragma: no cover
+        pass
+    else:
+        removed = str(pth)
     return removed

tests/basilisp/cli_test.py

@@ -10,6 +10,7 @@
 import sys
 import tempfile
 import time
+import venv
 from collections.abc import Sequence
 from threading import Thread
 from typing import Optional
@@ -108,6 +109,7 @@ def test_install(self, tmp_path: pathlib.Path, run_cli):
         assert bootstrap_file.read_text() == "import basilisp.sitecustomize"
 
         assert res.out == (
+            f"(Added {bootstrap_file})\n\n"
             "Your Python installation has been bootstrapped! You can undo this at any "
             "time with with `basilisp bootstrap --uninstall`.\n"
         )
@@ -135,6 +137,41 @@ def test_install_quiet(self, tmp_path: pathlib.Path, run_cli, capsys):
         res = capsys.readouterr()
         assert res.out == ""
 
+    @pytest.mark.slow
+    def test_install_import(self, tmp_path: pathlib.Path):
+        venv_path = tmp_path / "venv"
+        venv.create(venv_path, with_pip=True)
+
+        venv_bin = venv_path / ("Scripts" if sys.platform == "win32" else "bin")
+        pip_path = venv_bin / "pip"
+        python_path = venv_bin / "python"
+        basilisp_path = venv_bin / "basilisp"
+
+        result = subprocess.run(
+            [pip_path, "install", "."], capture_output=True, text=True, cwd=os.getcwd()
+        )
+
+        lpy_file = tmp_path / "boottest.lpy"
+        lpy_file.write_text("(ns boottest) (defn abc [] (println (+ 155 4)))")
+
+        cmd_import = [python_path, "-c", "import boottest; boottest.abc()"]
+        result = subprocess.run(
+            cmd_import, capture_output=True, text=True, cwd=tmp_path
+        )
+        assert "No module named 'boottest'" in result.stderr, result
+
+        result = subprocess.run(
+            [basilisp_path, "bootstrap"], capture_output=True, text=True, cwd=tmp_path
+        )
+        assert (
+            "Your Python installation has been bootstrapped!" in result.stdout
+        ), result
+
+        result = subprocess.run(
+            cmd_import, capture_output=True, text=True, cwd=tmp_path
+        )
+        assert result.stdout.strip() == "159", result
+
     def test_nothing_to_uninstall(self, tmp_path: pathlib.Path, run_cli, capsys):
         bootstrap_file = tmp_path / "basilispbootstrap.pth"
         assert not bootstrap_file.exists()

tests/conftest.py

@@ -1 +1,19 @@
+import pytest
+
 pytest_plugins = ["pytester"]
+
+
+def pytest_configure(config):
+    config.addinivalue_line("markers", "slow: Marks tests as slow")
+
+
+def pytest_addoption(parser):
+    parser.addoption("--run-slow", action="store_true", help="Run slow tests")
+
+
+@pytest.fixture(autouse=True)
+def skip_slow(request):
+    if request.node.get_closest_marker("slow") and not request.config.getoption(
+        "--run-slow"
+    ):
+        pytest.skip("Skipping slow test. Use --run-slow to enable.")

tox.ini

@@ -20,6 +20,8 @@ commands =
              -m pytest \
              --import-mode=importlib \
              --junitxml={toxinidir}/junit/pytest/{envname}.xml \
+             # also enable pytest marked as slow \
+             --run-slow \
              {posargs}
 
 [testenv:coverage]