Fix: pyvenv.cfg corruption and wrong Python distribution selection when multiple Python versions are installed (3.13 after 3.12) - #1698
Conversation
…/writer - Remove dependency on Salaros.Configuration.ConfigParser for pyvenv.cfg serialization in both PyVenvRunner and UvVenvRunner SetPyvenvCfg methods - Adds PyVenvConfigHelper.WritePyVenvCfg that reads, updates, and writes the key=value lines directly without section-header round-tripping - Fixes silent failure where ConfigParser.SetValue would not update the existing "home" key in a sectionless INI file, while successfully adding new keys (base-prefix, base-exec-prefix, base-executable), producing a corrupt config with mixed Python distribution paths - Preserve all non-path keys (include-system-site-packages, version, executable, command, etc.) in their original line order - Append missing path keys if the venv was created by an older version that did not write them
…re installed
- Fix fallback directory scanner in UvManager.InstallPythonVersionAsync using Contains("3.12") which also matched "3.13.12" directory names, causing the wrong Python distribution to be selected when UV listing failed and the newer 3.13 installation had a more recent creation timestamp
- Switched to strict version prefix matching ("3.12.") with an EndsWith fallback for edge cases like "pypy-3.12" naming
- Fix installedOnly parameter in ListAvailablePythonsAsync being ignored, causing uninstalled Python entries with null Path to reach the PyInstallation constructor and throw ArgumentException, which aborted the entire UV discovery loop via the catch-all in GetAllInstallationsAsync
- Wire PyVenvConfigHelper.WritePyVenvCfg into PyVenvRunner and UvVenvRunner SetPyvenvCfg, replacing the Salaros.Configuration.ConfigParser round-trip that silently failed to update the existing "home" key
- Remove unused Salaros.Configuration using directives from both runner files
- Parse each line into key and value by splitting on '=', then compare the key with ordinal case-insensitive Equals rather than StartsWith - Preserve lines with no '=' delimiter as-is - Eliminates ordering dependency between key checks. Each key is now matched exactly and independently, so reordering the checks or adding a new key like "base" cannot silently swallow "base-prefix" or "base-executable" through prefix collision
… UvManager - When installedOnly is false the preceding Where clause allows e.Path to be null, making the null-forgiving operator (!) semantically incorrect and misleading - Replace with a conditional that uses Path.GetDirectoryName only when e.Path is non-null, falling back to string.Empty otherwise
|
@mohnjiles @ionite34 can also remove the salaros reference in the package.prop and csproj files. The NuGet package still gets pulled during |
ionite34
left a comment
There was a problem hiding this comment.
Heya, Lykos here :3 poking through this alongside Ionite
First up: this fixes a real thing and we want it in. you chased the bug down to the actual writer, and the "pyvenv.cfg isn't INI, stop pretending" instinct is right. CPython's site.py splits on the first =, lowercases the key, no sections, no quoting, so the [top] prepend → SetValue → ToString() → Replace roundtrip was always a hack living on borrowed time.
When we built the PR head into a .NET file-based app with a #:project ref to StabilityMatrix.Core and tested with the real types, couple things fell out ->
Bug B doesn't reproduce. we ran the PyInstallationManager.GetAllInstallationsAsync with a stubbed IUvManager handing it an empty InstallPath entry sandwiched between two good ones:
Path.GetDirectoryName(null) -> returned null, no throw
new PyInstallation(v, "") -> threw ArgumentException (this half is true)
GetAllInstallationsAsync -> completed, returned 2 installations
So the ctor does throw on empty... but nothing ever hands it an empty path? Path.GetDirectoryName(null) returns null rather than throwing, the ?? string.Empty absorbs it, and PyInstallationManager.cs:71 already does if (string.IsNullOrWhiteSpace(uvPythonInfo.InstallPath)) continue; before constructing, so the catch-all never fires and the entry after the bad one survives.
The installedOnly fix is still worth keeping though! The param is documented as filtering and straight up didn't. it's just a dead-parameter cleanup rather than the thing that broke stuff really. worth rewording in the description so the next person debugging this doesn't chase it?
Bug C is realer than Bug B but the mechanism's different. we swept 15 pyvenv.cfg shapes through the old code vs the new helper:
case OLD (ConfigParser) NEW (helper)
baseline (control) OK OK
UTF-8 BOM OK OK
path with spaces OK OK
value contains ';' / '#' OK OK
blank line / leading ws OK OK
comment line first OK OK
uv-style relocatable+prompt OK OK
home last / base-* present OK OK
leftover [top] header threw ConfigParserException OK
duplicate home key home:STALE OK <-- there it is
ConfigParser updates home just fine on every well-formed file. it breaks on exactly one shape: a pyvenv.cfg with two home keys. Salaros updates only the first occurrence and appends base-* after it. CPython is last-wins, so you get:
home = ...cpython-3.13.12... <- Salaros updated this one
base-prefix=...cpython-3.13.12...
home = ...cpython-3.12.10... <- survived, and this is the one Python reads
home at 3.12, base-* at 3.13. the exact bug report, reproduced
one thread still dangling though: we round-tripped the old writer 1x, 2x, 5x-alternating and home stays at exactly one key every time, so the old code doesn't create the duplicate itself. something else is writing that second home (we're thinking it might be uv re-seeding pyvenv.cfg on an existing venv).
(good news though: your helper handles the duplicate case correctly rewriting every match. which is the right behaviour, it just happens by way of it being a loop rather than anyone deciding it. more on that below...)
the helper itself — the shape's what's rattling around
PyVenvConfigHelper is doing the right thing, we're just squinting at the shape
- the doc says "reading and writing pyvenv.cfg files" but there's no reading, and it can only ever write four hardcoded keys. it's really
SetVenvBasePaths(path, pyDir, exe)wearing a general-purpose name. - the set-a-key logic is written eight times (4 branches + 4
has*bools + 4 append blocks). adding a fifth key means touching three places. - "home, base-prefix and base-exec-prefix all get the same value" is the caller's thing, not a fact about the file format, so baking it into the file helper means it can't be reused for anything, which kinda defeats extracting it.
something like this would carry its own meaning better?
/// <summary>Ordered, sectionless key = value config, as used by pyvenv.cfg.</summary>
public sealed class PyVenvCfg
{
public static PyVenvCfg Parse(string content); // testable with no disk!
public static PyVenvCfg Load(FilePath path);
public string? this[string key] { get; set; } // case-insensitive, order-preserving
public override string ToString();
public void Save(FilePath path);
}then the runner reads as its own intent (set four keys, save) and the duplicate-key semantics become a thing the type states instead. failing that, even just WritePyVenvCfg(string cfgPath, IReadOnlyDictionary<string, string> values) kills the bool soup. no strong feelings on which, just... the current one can't grow? what do you think :3
and the bigger duplication is still sitting there -> PyVenvRunner.SetPyvenvCfg and UvVenvRunner.SetPyvenvCfg are now byte-identical, guards and lastSetPyvenvCfgPath and the Compat.IsWindows bit and all. we get that de-duping those two is a bigger swing and maybe not this PR's job, but since we're already in here...
tests StabilityMatrix.Tests/Core/ exists and this seems the most test-shaped code in the whole PR — pure string transform, no IO needed if we add a Parse/ToString pair. and the failure mode is silent, which is exactly the kind that comes back. the cases we'd want:
- two
homekeys (the actual bug! ← headline test) - existing key updates in place
- missing keys get appended
- unrelated keys keep their original order
home=Xwith no spaces around=- a value containing
=
smol stuff
UvManager.cs:309— theEndsWith($"-{major}.{minor}")fallback never actually fires? uv dirs arecpython-3.12.10-windows-x86_64-none, so the version is segment[1]and never terminal. reads like coverage but it's dead.UvManager.cs:327— deeper one: the fallback returnsnew UvPythonInfo(version, actualInstallPath, ...), stamping the requested version onto whatever dir the substring matched. so a fuzzy match doesn't just pick wrong, it reports wrong. sincePyVersion.TryParsealready exists, parsing segment[1]and requiring Major/Minor to match structurally would kill the collision AND the lie in one go — and it'd be unit-testable. worth a think?- old code threw loudly on a malformed file (leftover
[top], UTF-16); the new helper never throws, and on UTF-16 it quietly drops every other key. failing loud was better for something that mangles a config in place, maybe worth a guard? Salaros.ConfigParseris still referenced inStabilityMatrix.Core.csproj,StabilityMatrix.csproj, andDirectory.Packages.props. this PR removes the last usages — so all three can go, which makes "drops a third-party dep" actually true :3
sorry that got chonky, none of it is "this is bad", you found a real bug that's been silently eating people's venvs and the fix direction is right. mostly we just want the description to match what's actually happening, and the helper to be shaped so it can grow 🐺
Problem
Installing forge-neo (requires Python 3.13.12) alongside an existing WebUI package installation (using Python 3.12.10) silently corrupts that pre-existing package's
pyvenv.cfg, causingerror no: 2at launch. Manually correcting the file has no effect as it is rewritten incorrectly on every subsequent launch.Steps to reproduce:
Data/Assets/Python/cpython-3.12.10-...and a venv is created with a correctpyvenv.cfgData/Assets/Python/cpython-3.13.12-...and its own venv is created correctlypyvenv.cfgnow hasbase-prefix,base-exec-prefix, andbase-executablepointing to the 3.13.12 distribution, whilehomeremains the original 3.12.10 pathRoot cause (three compounding bugs)
Bug A: Fallback directory scanner matches wrong version (
UvManager.cs):When UV's
python listfails and the fallback scanner runs,Contains("3.12")matches bothcpython-3.12.10-...andcpython-3.13.12-...(the substring "3.12" appears in "3.13.12").Results are ordered by
CreationTimeUtcdescending, so the more recently installed 3.13.12 directory is selected as the "discovered" 3.12.10 installation.Bug B:
installedOnlyparameter is dead code (UvManager.cs):ListAvailablePythonsAsync(installedOnly: true)never filters to installed-only entries.Uninstalled Python entries with
Path = nullproduce an emptyInstallPath, which throwsArgumentExceptioninPyInstallation's constructor, aborting the entire UV discovery loop via the catch-all inGetAllInstallationsAsync. This pushes the system into Bug A's fallback path.Bug C: ConfigParser silently fails on pre-existing
homekey(
PyVenvRunner.cs/UvVenvRunner.cs):The
SetPyvenvCfgmethod prepends[top]to make the sectionlesspyvenv.cfgparseable bySalaros.Configuration.ConfigParser, then callsSetValue("top", "home", ...). The ConfigParser silently refuses to update the existinghomekey while successfully adding the new keys (base-prefix,base-exec-prefix,base-executable), producing the mixed-path config withhomeat 3.12 and the other three at 3.13.Changes
StabilityMatrix.Core/Python/PyVenvConfigHelper.cs(new file)Replaces the
ConfigParserroundtrip with a direct line-by-line key=value reader/writer.Extracts the key before
=on each line, compares with exactEquals, updates matching keys, and appends missing ones.Preserves all non-path keys in their original order. This eliminates the fragile
[top]section-header hack, the silent key-update failure, and the dependency on a third-party INI parser for a format that is not INI.Why a new helper instead of fixing ConfigParser:
pyvenv.cfgis a simplekey = valueformat with no sections, no quoting, and no escaping. A section-based INI parser adds indirection without adding value.[top]→ parse →SetValue→ToString()→ strip-[top]roundtrip has three fragility points: the section injection, the key update semantics on a sectionless file, and the section removal viaReplace.StabilityMatrix.Core/Python/PyVenvRunner.csSetPyvenvCfgreduced from a 15-lineConfigParserroundtrip to a 3-line call toPyVenvConfigHelper.WritePyVenvCfg. Removedusing Salaros.Configuration.StabilityMatrix.Core/Python/UvVenvRunner.csIdentical change to
PyVenvRunner.cs. Removedusing Salaros.Configuration.StabilityMatrix.Core/Python/UvManager.csListAvailablePythonsAsync: TheinstalledOnlyparameter now actually filters whentrue, entries withPath == nullare excluded. Explicit null check one.Pathin theSelectprojection rather than a null-forgiving operator.InstallPythonVersionAsyncfallback scanner:Contains("3.12")replaced withContains("3.12.")plus an EndsWith("-3.12")` fallback for PyPy-style directory names. The trailing dot prevents substring collision with higher versions.