fix: failed to detect flavor if compiler path include white spaces (#240)

toyobayashi · cclauss · web-flow · commit f3b9753e7526 · 2024-04-23T11:09:50.000+01:00
Co-authored-by: Christian Clauss &lt;cclauss@me.com&gt;
diff --git a/pylib/gyp/common.py b/pylib/gyp/common.py
@@ -9,6 +9,7 @@
 import tempfile
 import sys
 import subprocess
+import shlex
 
 from collections.abc import MutableSet
 
@@ -424,46 +425,49 @@ def EnsureDirExists(path):
 
 def GetCrossCompilerPredefines():  # -> dict
     cmd = []
+
+    # shlex.split() will eat '\' in posix mode, but
+    # setting posix=False will preserve extra '"' cause CreateProcess fail on Windows
+    # this makes '\' in %CC_target% and %CFLAGS% work
+    def replace_sep(s):
+        return s.replace(os.sep, "/") if os.sep != "/" else s
+
     if CC := os.environ.get("CC_target") or os.environ.get("CC"):
-        cmd += CC.split(" ")
+        cmd += shlex.split(replace_sep(CC))
         if CFLAGS := os.environ.get("CFLAGS"):
-            cmd += CFLAGS.split(" ")
+            cmd += shlex.split(replace_sep(CFLAGS))
     elif CXX := os.environ.get("CXX_target") or os.environ.get("CXX"):
-        cmd += CXX.split(" ")
+        cmd += shlex.split(replace_sep(CXX))
         if CXXFLAGS := os.environ.get("CXXFLAGS"):
-            cmd += CXXFLAGS.split(" ")
+            cmd += shlex.split(replace_sep(CXXFLAGS))
     else:
         return {}
 
     if sys.platform == "win32":
         fd, input = tempfile.mkstemp(suffix=".c")
+        real_cmd = [*cmd, "-dM", "-E", "-x", "c", input]
         try:
             os.close(fd)
-            out = subprocess.Popen(
-                [*cmd, "-dM", "-E", "-x", "c", input],
-                shell=True,
-                stdout=subprocess.PIPE, stderr=subprocess.STDOUT
-            )
-            stdout = out.communicate()[0]
+            stdout = subprocess.run(
+                real_cmd, shell=True,
+                capture_output=True, check=True
+            ).stdout
         finally:
             os.unlink(input)
     else:
         input = "/dev/null"
-        out = subprocess.Popen(
-            [*cmd, "-dM", "-E", "-x", "c", input],
-            shell=False,
-            stdout=subprocess.PIPE, stderr=subprocess.STDOUT
-        )
-        stdout = out.communicate()[0]
+        real_cmd = [*cmd, "-dM", "-E", "-x", "c", input]
+        stdout = subprocess.run(
+            real_cmd, shell=False,
+            capture_output=True, check=True
+        ).stdout
 
     defines = {}
     lines = stdout.decode("utf-8").replace("\r\n", "\n").split("\n")
     for line in lines:
-        if not line:
-            continue
-        define_directive, key, *value = line.split(" ")
-        assert define_directive == "#define"
-        defines[key] = " ".join(value)
+        if (line or "").startswith("#define "):
+            _, key, *value = line.split(" ")
+            defines[key] = " ".join(value)
     return defines
 
 def GetFlavorByPlatform():
diff --git a/pylib/gyp/common_test.py b/pylib/gyp/common_test.py
@@ -10,7 +10,6 @@
 import unittest
 import sys
 import os
-import subprocess
 from unittest.mock import patch, MagicMock
 
 class TestTopologicallySorted(unittest.TestCase):
@@ -26,9 +25,8 @@ def test_Valid(self):
         def GetEdge(node):
             return tuple(graph[node])
 
-        self.assertEqual(
-            gyp.common.TopologicallySorted(graph.keys(), GetEdge), ["a", "c", "d", "b"]
-        )
+        assert gyp.common.TopologicallySorted(
+            graph.keys(), GetEdge) == ["a", "c", "d", "b"]
 
     def test_Cycle(self):
         """Test that an exception is thrown on a cyclic graph."""
@@ -60,7 +58,7 @@ def tearDown(self):
 
     def assertFlavor(self, expected, argument, param):
         sys.platform = argument
-        self.assertEqual(expected, gyp.common.GetFlavor(param))
+        assert expected == gyp.common.GetFlavor(param)
 
     def test_platform_default(self):
         self.assertFlavor("freebsd", "freebsd9", {})
@@ -90,47 +88,84 @@ def test_GetCrossCompilerPredefines(self, mock_mkstemp, mock_unlink, mock_close)
         mock_unlink.return_value = None
         mock_mkstemp.return_value = (0, "temp.c")
 
-        def mock_run(env, defines_stdout):
-            with patch("subprocess.Popen") as mock_popen:
+        def mock_run(env, defines_stdout, expected_cmd):
+            with patch("subprocess.run") as mock_run:
                 mock_process = MagicMock()
-                mock_process.communicate.return_value = (
-                    TestGetFlavor.MockCommunicate(defines_stdout), None)
-                mock_process.stdout = MagicMock()
-                mock_popen.return_value = mock_process
+                mock_process.returncode = 0
+                mock_process.stdout = TestGetFlavor.MockCommunicate(defines_stdout)
+                mock_run.return_value = mock_process
                 expected_input = "temp.c" if sys.platform == "win32" else "/dev/null"
                 with patch.dict(os.environ, env):
                     defines = gyp.common.GetCrossCompilerPredefines()
                     flavor = gyp.common.GetFlavor({})
                 if env.get("CC_target"):
-                    mock_popen.assert_called_with(
-                        [env["CC_target"], "-dM", "-E", "-x", "c", expected_input],
+                    mock_run.assert_called_with(
+                        [
+                            *expected_cmd,
+                            "-dM", "-E", "-x", "c", expected_input
+                        ],
                         shell=sys.platform == "win32",
-                        stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+                        capture_output=True, check=True)
                 return [defines, flavor]
 
-        [defines1, _] = mock_run({}, "")
-        self.assertDictEqual({}, defines1)
+        [defines1, _] = mock_run({}, "", [])
+        assert {} == defines1
 
         [defines2, flavor2] = mock_run(
             { "CC_target": "/opt/wasi-sdk/bin/clang" },
-            "#define __wasm__ 1\n#define __wasi__ 1\n"
+            "#define __wasm__ 1\n#define __wasi__ 1\n",
+            ["/opt/wasi-sdk/bin/clang"]
         )
-        self.assertDictEqual({ "__wasm__": "1", "__wasi__": "1" }, defines2)
-        self.assertEqual("wasi", flavor2)
+        assert { "__wasm__": "1", "__wasi__": "1" } == defines2
+        assert flavor2 == "wasi"
 
         [defines3, flavor3] = mock_run(
-            { "CC_target": "/opt/wasi-sdk/bin/clang" },
-            "#define __wasm__ 1\n"
+            { "CC_target": "/opt/wasi-sdk/bin/clang --target=wasm32" },
+            "#define __wasm__ 1\n",
+            ["/opt/wasi-sdk/bin/clang", "--target=wasm32"]
         )
-        self.assertDictEqual({ "__wasm__": "1" }, defines3)
-        self.assertEqual("wasm", flavor3)
+        assert { "__wasm__": "1" } == defines3
+        assert flavor3 == "wasm"
 
         [defines4, flavor4] = mock_run(
             { "CC_target": "/emsdk/upstream/emscripten/emcc" },
-            "#define __EMSCRIPTEN__ 1\n"
+            "#define __EMSCRIPTEN__ 1\n",
+            ["/emsdk/upstream/emscripten/emcc"]
+        )
+        assert { "__EMSCRIPTEN__": "1" } == defines4
+        assert flavor4 == "emscripten"
+
+        # Test path which include white space
+        [defines5, flavor5] = mock_run(
+            {
+                "CC_target": "\"/Users/Toyo Li/wasi-sdk/bin/clang\" -O3",
+                "CFLAGS": "--target=wasm32-wasi-threads -pthread"
+            },
+            "#define __wasm__ 1\n#define __wasi__ 1\n#define _REENTRANT 1\n",
+            [
+                "/Users/Toyo Li/wasi-sdk/bin/clang",
+                "-O3",
+                "--target=wasm32-wasi-threads",
+                "-pthread"
+            ]
+        )
+        assert {
+            "__wasm__": "1",
+            "__wasi__": "1",
+            "_REENTRANT": "1"
+        } == defines5
+        assert flavor5 == "wasi"
+
+        original_sep = os.sep
+        os.sep = "\\"
+        [defines6, flavor6] = mock_run(
+            { "CC_target": "\"C:\\Program Files\\wasi-sdk\\clang.exe\"" },
+            "#define __wasm__ 1\n#define __wasi__ 1\n",
+            ["C:/Program Files/wasi-sdk/clang.exe"]
         )
-        self.assertDictEqual({ "__EMSCRIPTEN__": "1" }, defines4)
-        self.assertEqual("emscripten", flavor4)
+        os.sep = original_sep
+        assert { "__wasm__": "1", "__wasi__": "1" } == defines6
+        assert flavor6 == "wasi"
 
 if __name__ == "__main__":
     unittest.main()
