Repository navigation
fix(pylock): merge a partial environment over defaults in select() - #1437
yunaremaia wants to merge 2 commits into
Conversation
`select()` reads `python_full_version` straight out of the `environment`
argument:
env_python_full_version = _pep440_python_full_version(
environment["python_full_version"]
if environment
else default_environment()["python_full_version"]
)
Nine lines earlier the same parameter is built as `dict(environment or {},
...) # Marker.evaluate will fill-up`, i.e. it carries *overrides* merged
over `default_environment()` -- which is exactly what `Marker.evaluate()`
does (`current_environment |= environment`). So the override semantics
held for `packages.marker` but not for `requires-python`.
An empty mapping is falsy and accidentally took the default branch, so
`environment={}` worked. Any other partial mapping raised an unhandled
`KeyError` out of a generator whose documented failure mode is
`PylockSelectError`:
list(lock.select(environment={"sys_platform": "linux"}))
KeyError: 'python_full_version'
Read the key from the merged default instead, and widen the annotation to
`Environment | Mapping[str, str | AbstractSet[str]] | None` so the
parameter's declared type matches both what `select()` accepts and what
`Marker.evaluate()` accepts. A set-valued `python_full_version` is a
caller error and is now refused with `PylockSelectError` rather than
reaching the version parser.
|
Added a review below, looks like you can get mismatched 🤖 AI text below 🤖 The background
If the caller gives Two examples, with the host on Python 3.12:
Before this change, these calls raised Possible fixes:
The review found no problems in the rest of the change. The empty and full mappings behave as before. A set-valued |
|
This changes the method signature to accept a partial environment. Is this desirable? |
The two spellings of the interpreter were read from different places. Marker.evaluate merges environment over default_environment(), so the markers saw the override, while the requires-python checks subscripted environment["python_full_version"] and fell back to the host. Passing only python_version therefore checked the override against the version of the interpreter running select(). Resolve the pair once, on the mapping handed to the markers, and read every check from it. python_full_version pins the interpreter, so it wins and the major.minor spelling is derived from it. python_version alone does not pin the patch level that requires-python is checked against, so it is refused instead of being guessed at. Signed-off-by: Yunare Maia <yunare@gmail.com>
|
Thanks both — the review was right, and this was a real defect in the PR rather than a nit. The problem. The two spellings of the interpreter were read from different places. Applied in
On the signature question (@sbidoul): a partial mapping is accepted deliberately. Tests —
Verified on
CI status: the five workflows ( |
|
My point is that If you have use cases for pylock.select to accept a partial environment I'd suggest you open an issue to describe them in your own words so we can discuss if an API change is worthwhile. My current position is that it's not, since it's easy for the caller to create a full environment and update it as desired. So I'm -1 on this PR as it stands. |
|
btw, I have a separate change that achieves the same goals with a different approach, I'll open an issue first. |
Summary
Pylock.select()readspython_full_versionstraight out of itsenvironmentargument:Nine lines earlier, the same parameter is built as
dict(environment or {}, ...) # Marker.evaluate will fill-up— it carries overrides merged overdefault_environment(), which is exactly whatMarker.evaluate()does (markers.py:current_environment |= environment). So the override semantics held forpackages.markerbut not forrequires-python.An empty mapping is falsy and accidentally took the default branch, which is why
environment={}worked. Any other partial mapping raised an unhandledKeyErrorout of a generator whose documented failure mode isPylockSelectError:Changes
python_full_versionfrom the merged default, so a partial mapping behaves as documented instead of raisingKeyError.Environment | Mapping[str, str | AbstractSet[str]] | None, matching both whatselect()now accepts andMarker.evaluate()'s signature, and document the override semantics in the docstring.python_full_versionwithPylockSelectErrorinstead of letting it reach the version parser. This mirrors the existingisinstanceguard inmarkers.pyfor set-valued environment keys.Tests
tests/test_pylock_select_environment.py— 9 tests: four partial mappings select as if merged, an empty mapping still behaves like the default, a fullEnvironmentstill selects, a realpython_full_versionoverride is honoured (and still failsrequires-pythonwhen it should), an unsatisfiable lock still raisesPylockSelectErroron a partial environment, and the set-valued case is refused.Verified against
mainat4a920d6:KeyError), pass after.ruff check,ruff format --checkandmypyclean on both touched files.The change is a no-op for callers that already pass a complete
Environmentor{}, so it is safe to release in a patch release.