Revert to the previous quoting behaviour for environment option

joerick · joerick · commit 13fc76855215 · 2022-09-17T11:15:00.000+01:00
diff --git a/cibuildwheel/environment.py b/cibuildwheel/environment.py
@@ -4,6 +4,7 @@
 from typing import Any, Mapping, Sequence
 
 import bashlex
+import bashlex.errors
 
 from cibuildwheel.typing import Protocol
 
@@ -33,7 +34,11 @@ def split_env_items(env_string: str) -> list[str]:
     if not env_string:
         return []
 
-    command_node = bashlex.parsesingle(env_string)
+    try:
+        command_node = bashlex.parsesingle(env_string)
+    except bashlex.errors.ParsingError as e:
+        raise EnvironmentParseError(env_string) from e
+
     result = []
 
     for word_node in command_node.parts:
diff --git a/cibuildwheel/options.py b/cibuildwheel/options.py
@@ -10,7 +10,7 @@
 from contextlib import contextmanager
 from dataclasses import asdict, dataclass
 from pathlib import Path
-from typing import Any, Dict, Generator, Iterator, List, Mapping, Union, cast
+from typing import Any, Callable, Dict, Generator, Iterator, List, Mapping, Union, cast
 
 if sys.version_info >= (3, 11):
     import tomllib
@@ -23,7 +23,7 @@
 from .environment import EnvironmentParseError, ParsedEnvironment, parse_environment
 from .oci_container import ContainerEngine
 from .projectfiles import get_requires_python_str
-from .typing import PLATFORMS, Literal, PlatformName, TypedDict
+from .typing import PLATFORMS, Literal, NotRequired, PlatformName, TypedDict
 from .util import (
     MANYLINUX_ARCHS,
     MUSLLINUX_ARCHS,
@@ -123,6 +123,7 @@ class Override:
 class TableFmt(TypedDict):
     item: str
     sep: str
+    quote: NotRequired[Callable[[str], str]]
 
 
 class ConfigOptionError(KeyError):
@@ -329,7 +330,7 @@ def get(
             if table is None:
                 raise ConfigOptionError(f"{name!r} does not accept a table")
             return table["sep"].join(
-                item for k, v in result.items() for item in _inner_fmt(k, v, table["item"])
+                item for k, v in result.items() for item in _inner_fmt(k, v, table)
             )
 
         if isinstance(result, list):
@@ -343,14 +344,16 @@ def get(
         return result
 
 
-def _inner_fmt(k: str, v: Any, table_item: str) -> Iterator[str]:
+def _inner_fmt(k: str, v: Any, table: TableFmt) -> Iterator[str]:
+    quote_function = table.get("quote", lambda a: a)
+
     if isinstance(v, list):
         for inner_v in v:
-            qv = shlex.quote(inner_v)
-            yield table_item.format(k=k, v=qv)
+            qv = quote_function(inner_v)
+            yield table["item"].format(k=k, v=qv)
     else:
-        qv = shlex.quote(v)
-        yield table_item.format(k=k, v=qv)
+        qv = quote_function(v)
+        yield table["item"].format(k=k, v=qv)
 
 
 class Options:
@@ -449,13 +452,13 @@ def build_options(self, identifier: str | None) -> BuildOptions:
 
             build_frontend_str = self.reader.get("build-frontend", env_plat=False)
             environment_config = self.reader.get(
-                "environment", table={"item": "{k}={v}", "sep": " "}
+                "environment", table={"item": '{k}="{v}"', "sep": " "}
             )
             environment_pass = self.reader.get("environment-pass", sep=" ").split()
             before_build = self.reader.get("before-build", sep=" && ")
             repair_command = self.reader.get("repair-wheel-command", sep=" && ")
             config_settings = self.reader.get(
-                "config-settings", table={"item": "{k}={v}", "sep": " "}
+                "config-settings", table={"item": "{k}={v}", "sep": " ", "quote": shlex.quote}
             )
 
             dependency_versions = self.reader.get("dependency-versions")
diff --git a/cibuildwheel/typing.py b/cibuildwheel/typing.py
@@ -10,6 +10,10 @@
 else:
     from typing import Final, Literal, OrderedDict, Protocol, TypedDict
 
+if sys.version_info < (3, 11):
+    from typing_extensions import NotRequired
+else:
+    from typing import NotRequired
 
 __all__ = (
     "Final",
@@ -26,6 +30,7 @@
     "OrderedDict",
     "Union",
     "assert_never",
+    "NotRequired",
 )
 
 
diff --git a/pyproject.toml b/pyproject.toml
@@ -66,6 +66,7 @@ module = [
     "setuptools",
     "pytest", # ignored in pre-commit to speed up check
     "bashlex",
+    "bashlex.*",
     "importlib_resources",
     "ghapi.*",
 ]
diff --git a/unit_test/options_test.py b/unit_test/options_test.py
@@ -62,7 +62,7 @@ def test_options_1(tmp_path, monkeypatch):
 
     default_build_options = options.build_options(identifier=None)
 
-    assert default_build_options.environment == parse_environment("FOO=BAR")
+    assert default_build_options.environment == parse_environment('FOO="BAR"')
 
     all_pinned_container_images = _get_pinned_container_images()
     pinned_x86_64_container_image = all_pinned_container_images["x86_64"]
@@ -122,14 +122,19 @@ def test_passthrough_evil(tmp_path, monkeypatch, env_var_value):
     assert parsed_environment.as_dictionary(prev_environment={}) == {"ENV_VAR": env_var_value}
 
 
+xfail_env_parse = pytest.mark.xfail(
+    raises=SystemExit, reason="until we can figure out the right way to quote these values"
+)
+
+
 @pytest.mark.parametrize(
     "env_var_value",
     [
         "normal value",
-        '"value wrapped in quotes"',
-        'an unclosed double-quote: "',
+        pytest.param('"value wrapped in quotes"', marks=[xfail_env_parse]),
+        pytest.param('an unclosed double-quote: "', marks=[xfail_env_parse]),
         "string\nwith\ncarriage\nreturns\n",
-        "a trailing backslash \\",
+        pytest.param("a trailing backslash \\", marks=[xfail_env_parse]),
     ],
 )
 def test_toml_environment_evil(tmp_path, monkeypatch, env_var_value):
@@ -163,6 +168,9 @@ def test_toml_environment_evil(tmp_path, monkeypatch, env_var_value):
         ('TEST_VAR="before:$PARAM:after"', "before:spam:after"),
         # env var extension with spaces
         ('TEST_VAR="before $PARAM after"', "before spam after"),
+        # literal $ - this test is just for reference, I'm not sure if this
+        # syntax will work if we change the TOML quoting behaviour
+        (r'TEST_VAR="before\\$after"', "before$after"),
     ],
 )
 def test_toml_environment_quoting(tmp_path: Path, toml_assignment, result_value):

Original file line number	Diff line number	Diff line change
`@@ -66,6 +66,7 @@ module = [`
`66`	`66`	`"setuptools",`
`67`	`67`	`"pytest", # ignored in pre-commit to speed up check`
`68`	`68`	`"bashlex",`
	`69`	`+ "bashlex.*",`
`69`	`70`	`"importlib_resources",`
`70`	`71`	`"ghapi.*",`
`71`	`72`	`]`