Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 63 additions & 4 deletions src/rez/rex.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
from enum import Enum
from contextlib import contextmanager
from string import Formatter
from collections.abc import MutableMapping
from typing import Any, Iterable, Mapping
from collections.abc import Mapping, MutableMapping
from typing import Any, Iterable

from rez.system import system
from rez.config import config
Expand Down Expand Up @@ -187,7 +187,10 @@ def __init__(self, interpreter: ActionInterpreter,
interpreter: string or `ActionInterpreter`
the interpreter to use when executing rex actions
parent_environ: environment to execute the actions within. If None,
defaults to the current environment.
defaults to the current environment. On Windows, a caller-supplied
mapping is wrapped in a read-only, case-insensitive snapshot so that
lookups against it are case-insensitive, matching how ``os.environ``
behaves natively (see #2089); `os.environ` and None are used as-is.
Comment thread
JeanChristopheMorinPerso marked this conversation as resolved.
Outdated
parent_variables: List of variables to append/prepend to, rather than
overwriting on first reference. If this is set to True instead of a
list, all variables are treated as parent variables.
Expand All @@ -200,9 +203,24 @@ def __init__(self, interpreter: ActionInterpreter,
'''
self.interpreter = interpreter
self.verbose = verbose
self.parent_environ = os.environ if parent_environ is None else parent_environ
# On Windows env-var keys are case-insensitive; os.environ already
# behaves that way but a plain dict supplied by the caller does not,
# so wrap it on the Windows path only. See #2089.
self.parent_environ: Mapping[str, str]
if parent_environ is None or parent_environ is os.environ:
# os.environ is already case-insensitive on Windows, so use it
# as-is (preserving liveness and identity); None means "current".
self.parent_environ = os.environ
elif platform_.name == "windows" and not isinstance(
parent_environ, _CaseInsensitiveEnvironProxy):
self.parent_environ = _CaseInsensitiveEnvironProxy(parent_environ)
else:
self.parent_environ = parent_environ
self.parent_variables = True if parent_variables is True \
else set(parent_variables or [])
# Variables set during rex execution. Keys are stored verbatim and are
# not case-folded on Windows (unlike the parent_environ snapshot above);
# that broader normalization is a separate concern, tracked in #2164.
self.environ = {}
self.formatter = formatter or str
self.actions = []
Expand Down Expand Up @@ -447,6 +465,47 @@ def _keytoken(self, key):
return self.interpreter.get_key_token(key)


class _CaseInsensitiveEnvironProxy(Mapping):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class _CaseInsensitiveEnvironProxy(Mapping):
class _CaseInsensitiveEnvironProxy(Mapping[str, str]):

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr: happy to take the Mapping[str, str] change, but it should use typing.Mapping to stay 3.8-safe. Separately, the failing Ruff check looks like a pre-existing issue on main, not something in this PR.

On the base class: Mapping here is imported from collections.abc, and using one of those as a subscripted base (class X(Mapping[str, str])) only works on Python 3.9+. On 3.8 it raises TypeError at import, and we still support 3.8 (python_requires >= 3.8). The other parametrized bases in the repo, like AttrDictWrapper(MutableMapping[str, Any]) and PackageOrderList(List[...]), go through typing for that reason. I can switch this to from typing import Mapping and apply Mapping[str, str], or leave it bare, whichever you prefer.

On the Ruff failure: it's ISC004 at src/rez/utils/platform_.py:524 (the PowerShell command list), which this branch doesn't touch. It reproduces on a clean main (4946f9de) with ruff 0.16.0, the newest release matching >= 0.15.0 that ruff-action pulls in. So it reads as the new ISC004 rule catching an existing line rather than anything from here. I'm happy to fold the one-line fix into this PR if that unblocks things, otherwise it may fit better as its own change on main.

Thanks for the sphinx pass too, the :data:/:class: refs read a lot cleaner.

"""Read-only, case-insensitive snapshot of an env-var mapping.

Used by `ActionManager` to wrap a caller-supplied `parent_environ`
Comment thread
JeanChristopheMorinPerso marked this conversation as resolved.
Outdated
on Windows so lookups against it are case-insensitive regardless of
the casing used to populate the input dict, matching how os.environ
Comment thread
JeanChristopheMorinPerso marked this conversation as resolved.
Outdated
behaves natively on Windows.

Keys are normalized (upper-cased) once at construction, so this is a
point-in-time snapshot: later mutations to the source mapping are not
reflected. That matches how `parent_environ` is used in practice (fixed
Comment thread
JeanChristopheMorinPerso marked this conversation as resolved.
Outdated
for the lifetime of an executor).

Kept private to this module on purpose; promote to a shared util
only when a second caller needs it.
"""
def __init__(self, data):
# keys are upper-cased once on the way in; same shape as
# os.environ on Windows. Last write wins on case collisions,
# which mirrors how os.environ resolves them too.
self._data = {k.upper(): v for k, v in data.items()}

def __getitem__(self, key):
return self._data[key.upper()]

def __contains__(self, key):
return key.upper() in self._data

def __iter__(self):
return iter(self._data)

def __len__(self):
return len(self._data)

def copy(self):
# Return a plain dict with normalized (upper-cased) keys, mirroring
# os.environ.copy() on Windows. Defensive: lets callers that expect a
# dict-like parent_environ (e.g. `self.parent_environ.copy()`) work.
return dict(self._data)


#===============================================================================
# Interpreters
#===============================================================================
Expand Down
106 changes: 106 additions & 0 deletions src/rez/tests/test_rex.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@
from rez.exceptions import RexError, RexUndefinedVariableError
from rez.config import config
import unittest
from unittest import mock
from rez.version import Version
from rez.version import Requirement
from rez.tests.util import TestBase
from rez.utils.backcompat import convert_old_commands
from rez.utils.platform_ import platform_
from rez.package_repository import package_repository_manager
from rez.packages import iter_package_families
import inspect
Expand Down Expand Up @@ -547,6 +549,110 @@ def test_intersects_ephemerals(self) -> None:
self.assertRaises(RuntimeError, # no default
intersects, ephemerals.get_range("foo.bar"), "0")

def test_case_insensitive_environ_proxy(self) -> None:
"""Unit-test the case-normalizing proxy directly (platform independent).

Runs on every host so the core logic behind the #2089 fix has real
CI coverage even though the ActionManager wiring is Windows-only.
"""
from rez.rex import _CaseInsensitiveEnvironProxy

proxy = _CaseInsensitiveEnvironProxy({"SomeVariable": "covfefe"})
for key in ("SomeVariable", "SOMEVARIABLE", "somevariable"):
self.assertEqual(proxy[key], "covfefe")
self.assertIn(key, proxy)
self.assertNotIn("OtherVariable", proxy)
self.assertRaises(KeyError, lambda: proxy["OtherVariable"])

# copy() yields a plain dict with normalized (upper-cased) keys,
# mirroring os.environ.copy() on Windows.
copied = proxy.copy()
self.assertIsInstance(copied, dict)
self.assertEqual(copied, {"SOMEVARIABLE": "covfefe"})

# normalized iteration / length
self.assertEqual(len(proxy), 1)
self.assertEqual(set(proxy), {"SOMEVARIABLE"})

# last-write-wins on case collision, mirroring os.environ
collided = _CaseInsensitiveEnvironProxy({"Foo": "a", "FOO": "b"})
self.assertEqual(collided["foo"], "b")

@unittest.skipIf(platform_.name == "windows",
"parent_environ passthrough is non-Windows behaviour")
def test_parent_environ_identity_passthrough_on_non_windows(self) -> None:
"""On non-Windows, a caller dict is used as-is (no wrapping). #2089."""
parent_env = {"SomeVariable": "covfefe"}
ex = self._create_executor(parent_env)
self.assertIs(ex.manager.parent_environ, parent_env)

@unittest.skipIf(platform_.name != "windows", "Windows-only behaviour")
def test_parent_environ_case_insensitive_on_windows(self) -> None:
"""Regression for #2089.

On Windows env vars are case-insensitive; os.environ already
behaves that way, but a plain dict copy of it doesn't. Passing
such a dict as parent_environ used to break env.<MixedCase>
lookups inside commands(). We now wrap on the Windows side.
"""
parent_env = {"SomeVariable": "covfefe"}
ex = self._create_executor(parent_env)

# any casing of the key resolves
self.assertEqual(ex.manager.parent_environ["SomeVariable"], "covfefe")
self.assertEqual(ex.manager.parent_environ["SOMEVARIABLE"], "covfefe")
self.assertEqual(ex.manager.parent_environ["somevariable"], "covfefe")
self.assertIn("SOMEVARIABLE", ex.manager.parent_environ)
self.assertNotIn("OtherVariable", ex.manager.parent_environ)

# end-to-end via the real package-commands entrypoint: rex references
# a different casing than the caller's dict, exercised through both
# execute_function and execute_code (what commands() actually run).
# Pre-fix this raised RexUndefinedVariableError. Note: the caller dict
# is named `parent_env`, not `env`, so the nested rex function does not
# close over it and shadow the rex `env` builtin.
def _rex() -> None:
env.RESULT = env.SOMEVARIABLE # noqa: F821 - rex builtin

self._test(
func=_rex,
env=parent_env,
expected_actions=[Setenv("RESULT", "covfefe")],
expected_output={"RESULT": "covfefe"},
)

def test_parent_environ_os_environ_passthrough(self) -> None:
"""Explicit os.environ is used as-is (identity), never wrapped or
copied -- it is already case-insensitive on Windows. #2089.
"""
ex = self._create_executor(os.environ)
self.assertIs(ex.manager.parent_environ, os.environ)

@mock.patch.object(platform_, "name", "windows")
def test_parent_environ_case_insensitive_windows_mocked(self) -> None:
"""Companion to test_parent_environ_case_insensitive_on_windows that
runs on every host by mocking the platform, so the Windows-only
wrapping branch keeps CI coverage (and local validation) off Windows
too. #2089.
"""
from rez.rex import _CaseInsensitiveEnvironProxy

parent_env = {"SomeVariable": "covfefe"}
ex = self._create_executor(parent_env)
self.assertIsInstance(
ex.manager.parent_environ, _CaseInsensitiveEnvironProxy)
self.assertEqual(ex.manager.parent_environ["SOMEVARIABLE"], "covfefe")

def _rex() -> None:
env.RESULT = env.SOMEVARIABLE # noqa: F821 - rex builtin

self._test(
func=_rex,
env=parent_env,
expected_actions=[Setenv("RESULT", "covfefe")],
expected_output={"RESULT": "covfefe"},
)


if __name__ == '__main__':
unittest.main()
Loading