Add bootstrapping and isolation of development Python versions #1105

zanieb · 2024-01-25T19:48:05Z

Replaces #1068 and #1070 which were more complicated than I wanted.

Introduces a .python-versions file which defines the Python versions needed for development
Adds a Bash script at scripts/bootstrap/install which installs the required Python versions from python-build-standalone to ./bin
Checks in a versions.json file with metadata about available versions on each platform and a fetch-version Python script derived from rye for updating the versions
Updates CI to use these Python builds instead of the setup-python action
Updates to the latest packse scenarios which require Python 3.8+ instead of 3.7+ since we cannot use 3.7 anymore and includes new test coverage of patch Python version requests
Adds a PUFFIN_PYTHON_PATH variable to prevent lookup of system Python versions for isolation during development

Tested on Linux (via CI) and macOS (locally) — presumably it will be a bit more complicated to do proper Windows support.

# Conflicts: # scripts/scenarios/update.py

# Conflicts: # crates/puffin-interpreter/src/python_query.rs # crates/puffin/src/commands/venv.rs

zanieb · 2024-01-25T20:31:45Z

crates/puffin-interpreter/src/interpreter.rs

+    pub fn find_executable<R: AsRef<OsStr> + Into<OsString> + Copy>(
+        requested: R,
+    ) -> Result<PathBuf, Error> {
+        if let Some(isolated) = std::env::var_os("PUFFIN_PYTHON_PATH") {
+            if let Ok(cwd) = std::env::current_dir() {
+                which::which_in(requested, Some(isolated), cwd)
+                    .map_err(|err| Error::Which(requested.into(), err))
+            } else {
+                which::which_in_global(requested, Some(isolated))
+                    .map_err(|err| Error::Which(requested.into(), err))
+                    .and_then(|mut paths| paths.next().ok_or(Error::PythonNotFound))
+            }
+        } else {
+            which::which(requested).map_err(|err| Error::Which(requested.into(), err))
+        }
+    }


I'd like to improve the error handling here in a follow-up

crates/puffin-interpreter/src/lib.rs

crates/puffin-interpreter/src/python_query.rs

crates/puffin/tests/pip_compile_scenarios.rs

scripts/bootstrap/fetch-versions

scripts/scenarios/update.py

scripts/bootstrap/install

zanieb · 2024-01-25T21:26:02Z

crates/puffin-interpreter/src/interpreter.rs

+    pub fn find_executable<R: AsRef<OsStr> + Into<OsString> + Copy>(
+        requested: R,
+    ) -> Result<PathBuf, Error> {
+        if let Some(isolated) = std::env::var_os("PUFFIN_PYTHON_PATH") {


I worry about this name being too close to PYTHONPATH which has a very different meaning.

Considering PUFFIN_PYTHON_EXECUTABLE_PATH but geesh.

I'm okay with PUFFIN_PYTHON_EXECUTABLE_PATH.

I'll address this once I merge this full stack to avoid annoying rebases

"path" is so overloaded anyway already

.envrc

charliermarsh

Very impressive.

crates/puffin-interpreter/src/python_query.rs

scripts/bootstrap/install

scripts/bootstrap/fetch-versions

charliermarsh · 2024-01-25T22:22:53Z

scripts/bootstrap/fetch-versions

+    "pgo+lto",
+    "lto",
+    "pgo",
+]


One question though - does this download one interpreter per version? Or multiple? How does it choose which one?

This just generates the version.json file which has links to downloads. I believe the flavor preferences section here is used to select a single interpreter flavor per combination of OS, architecture, and Python version.

scripts/bootstrap/install

charliermarsh · 2024-01-25T22:23:54Z

scripts/bootstrap/install

+    rm "$this_dir/$filename"
+done
+
+echo "Done!"


I don't know that I feel qualified to review this.

As long as you test it I'm not too worried about the implementation.

CONTRIBUTING.md

.github/workflows/ci.yml

konstin

The install script errors on windows 11 git bash:

$ scripts/bootstrap/install.sh
which: no grealpath in (/mingw64/bin:/usr/bin:/c/Users/Konstantin/bin:/c/Python37/Scripts:/c/Python37:/c/Program Files (x86)/Intel/iCLS Client:/c/Program Files/Intel/iCLS Client:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/IPT:/c/Program Files/Intel/Intel(R) Management Engine Components/IPT:/c/Program Files/Intel/WiFi/bin:/c/Program Files/Common Files/Intel/WirelessCommon:/c/Windows/System32/OpenSSH:/cmd:/c/Program Files/nodejs:/c/ProgramData/chocolatey/bin:/c/Program Files/PowerShell/7:/c/Users/Konstantin/AppData/Local/Programs/Python/Python312/Scripts:/c/Users/Konstantin/AppData/Local/Programs/Python/Python312:/c/Users/Konstantin/AppData/Local/Programs/Python/Python38/Scripts:/c/Users/Konstantin/AppData/Local/Programs/Python/Python38:/c/Users/Konstantin/.cargo/bin:/c/Users/Konstantin/AppData/Local/Microsoft/WindowsApps:/c/Users/Konstantin/AppData/Local/JetBrains/Toolbox/scripts:/c/users/konstantin/.local/bin:/c/Users/Konstantin/AppData/Roaming/npm:/c/Users/Konstantin/AppData/Local/Microsoft/WindowsApps:/c/Users/Konstantin/AppData/Local/Microsoft/WinGet/Packages/BurntSushi.ripgrep.MSVC_Microsoft.Winget.Source_8wekyb3d8bbwe/ripgrep-14.1.0-x86_64-pc-windows-msvc:/c/Users/Konstantin/AppData/Local/Microsoft/WinGet/Packages/jqlang.jq_Microsoft.Winget.Source_8wekyb3d8bbwe)
Installing cpython-3.8.12-mingw64_nt-10.0-22621-x86_64
No matching download for cpython-3.8.12-mingw64_nt-10.0-22621-x86_64

konstin · 2024-01-26T13:01:58Z

CONTRIBUTING.md

+Then add the Python binaries to your path:
+
+```
+export PATH=$PWD/bin:$PATH
+```
+
+We also strongly recommend setting the `PUFFIN_PYTHON_PATH` variable during development; this will prevent your
+system Python versions from being found during tests:
+
+```
+export PUFFIN_PYTHON_PATH=$PWD/bin
+```


Shouldn't this be something like

PATH=$PWD/bin:$PATH PUFFIN_PYTHON_PATH=$PWD/bin cargo nextest run ...

Seems cumbersome to always having to copy those two commands before doing anything when not using direnv

konstin · 2024-01-26T13:02:29Z

crates/puffin-interpreter/src/interpreter.rs

+    pub fn find_executable<R: AsRef<OsStr> + Into<OsString> + Copy>(
+        requested: R,
+    ) -> Result<PathBuf, Error> {
+        if let Some(isolated) = std::env::var_os("PUFFIN_PYTHON_PATH") {


"path" is so overloaded anyway already

crates/puffin-interpreter/src/lib.rs

konstin · 2024-01-26T13:04:51Z

crates/puffin-interpreter/src/python_query.rs

+        .map(str::parse::<u8>)
+        .collect::<Result<Vec<_>, _>>();
+    if let Ok(versions) = versions {
+        // `-p 3.10` or `-p 3.10.1`


The function docstring needs to be updated

crates/puffin-interpreter/src/python_query.rs

konstin · 2024-01-26T13:09:43Z

scripts/bootstrap/install.sh

@@ -0,0 +1,101 @@
+#!/usr/bin/env bash


This script introduces a jq dependency which needs to be documented. For windows:

winget install jqlang.jq

or https://jqlang.github.io/jq/download/.

Additionally, I'm a fan of explicitly checking for dependencies used in a shell script if any outside of core tools are needed. For example, see https://github.com/BurntSushi/dotfiles/blob/2431b0d87614eabc0f022fd3617dcb0e5a5b4de4/bin/backup#L315-L322 and https://github.com/BurntSushi/dotfiles/blob/2431b0d87614eabc0f022fd3617dcb0e5a5b4de4/bin/backup#L341.

It's a big quality of life improvement because it avoids the scenario of getting half-way through execution and then getting a "jq not found" error.

We could also write this in Python to avoid adding new dependencies. And it might work on Windows. (Although I see some symlinking below...)

Ah thanks missed that dependency in my list and I'm happy to add explicit checks.

I wrote the whole thing in Python first at #1068 — but it was annoying that it had more Python requirements and I really didn't like bootstrapping Python development with Python.

BurntSushi

Nice! Does this mean I don't need pyenv any more?

BurntSushi · 2024-01-26T13:32:00Z

.envrc

@@ -0,0 +1,3 @@
+PATH=$PWD/bin:$PATH
+export PUFFIN_PYTHON_PATH=$PWD/bin


Should these variables be quoted?

(It would be odd for these to require quoting on a Unix system, but if this gets used on Windows, spaces are much more common there IIRC.)

I'm not sure about the envrc syntax, but probably.

BurntSushi · 2024-01-26T13:32:38Z

.github/workflows/ci.yml

-        run: cargo nextest run --workspace --all-features --status-level skip --failure-output immediate-final --no-fail-fast -j 12
+        run: |
+          export PATH=$PWD/bin:$PATH
+          export PUFFIN_PYTHON_PATH=$PWD/bin


Can you use direnv here instead?

BurntSushi · 2024-01-26T13:41:18Z

scripts/bootstrap/fetch-version-metadata.py

+"""
+)
+_suffix_re = re.compile(
+    r"""(?x)^(.*?)-(%s)$"""


I think this can just be "-(%s)$", although you would need to use _suffix_re.search(...) instead of _suffix_re.match(...) below.

This is copied directly from Rye, hesitant to change?

BurntSushi · 2024-01-26T13:45:32Z

scripts/bootstrap/install.sh

@@ -0,0 +1,101 @@
+#!/usr/bin/env bash


Additionally, I'm a fan of explicitly checking for dependencies used in a shell script if any outside of core tools are needed. For example, see https://github.com/BurntSushi/dotfiles/blob/2431b0d87614eabc0f022fd3617dcb0e5a5b4de4/bin/backup#L315-L322 and https://github.com/BurntSushi/dotfiles/blob/2431b0d87614eabc0f022fd3617dcb0e5a5b4de4/bin/backup#L341.

It's a big quality of life improvement because it avoids the scenario of getting half-way through execution and then getting a "jq not found" error.

BurntSushi · 2024-01-26T13:46:42Z

scripts/bootstrap/install.sh

@@ -0,0 +1,101 @@
+#!/usr/bin/env bash


We could also write this in Python to avoid adding new dependencies. And it might work on Windows. (Although I see some symlinking below...)

BurntSushi · 2024-01-26T13:49:29Z

scripts/bootstrap/install.sh

+
+# Determine system metadata
+os=$(uname | tr '[:upper:]' '[:lower:]')
+arch=$(arch)


arch isn't a standard command. (It's not on my system for example.) I think it is Debian specific. uname --machine might be what you're looking for?

👍 weird its present on macOS and Ubuntu but can change

zanieb · 2024-01-26T15:53:22Z

I plan on addressing this round of feedback then merging since I have a stack on top of this and we really need to codify these versions. The bin folder can be moved elsewhere if you want to use these versions of Python globally on your system (and avoid setting variables when using the project). I don't think we'll have Windows support initially since I can't test it, but I'm happy to collaborate on it. I can work on it via GitHub Actions in a follow-up if needed.

Does this mean I don't need pyenv any more?

That's the goal!

…rsions (#1106) In #1040 we broke the pip compile scenarios designed to test failure when a required Python version is not available — resolution succeeded because all of the Python versions were available in CI. Following #1105 we have the ability to isolate tests from Python versions available in the system. Here, we limit the scenarios to only the Python version in the current environment, restoring our ability to test the error messages. With astral-sh/packse#95, we will be able to specify scenarios with access to additional system Python versions. This will allow us to include test coverage where resolution can succeed by using a version available elsewhere on the system. See #1111 for this follow-up.

#1131 shows that `direnv` installation is _most_ of the CI overhead introduced by #1105. Instead of using `direnv`, let's just use a simple `.env` file that can be loaded with `source` or [`direnv`'s `dotenv` directive](https://direnv.net/man/direnv-stdlib.1.html#codedotenv-ltdotenvpathgtcode).

zanieb force-pushed the zb/bootstrap-python branch from 6e72219 to f3a9802 Compare January 25, 2024 19:49

zanieb added 2 commits January 25, 2024 13:58

Add simple bootstrapping of Python versions

5ba3fed

Use current executable when calling into packse

52b4753

# Conflicts: # scripts/scenarios/update.py

zanieb force-pushed the zb/bootstrap-python branch from f3a9802 to 787b8ee Compare January 25, 2024 20:00

zanieb mentioned this pull request Jan 25, 2024

Use bootstrapped Python versions in CI #1070

Closed

zanieb added 5 commits January 25, 2024 14:25

Allow requesting patch Python versions on Unix

f503ddb

Update scenarios to use Python 3.8+

9ac88a9

Add environment variable to isolate Python version lookups

a58902a

# Conflicts: # crates/puffin-interpreter/src/python_query.rs # crates/puffin/src/commands/venv.rs

Add contributing documentation

9331f6c

Add direnv configuration file

6b0f69d

zanieb force-pushed the zb/bootstrap-python branch from dfb9bf0 to 6b0f69d Compare January 25, 2024 20:25

zanieb added the testing Internal testing of behavior label Jan 25, 2024

Lint the install script

f352e32