makespec: Use portable path separators

PyInstaller/building/makespec.py

@@ -15,6 +15,7 @@
 import argparse
 import os
 import re
+import pathlib
 
 from PyInstaller import DEFAULT_SPECPATH, HOMEPATH
 from PyInstaller import log as logging
@@ -29,9 +30,8 @@
 DEBUG_ALL_CHOICE = ['all']
 
 
-def escape_win_filepath(path):
-    # escape all \ with another \ after using normpath to clean up the path
-    return os.path.normpath(path).replace('\\', '\\\\')
+def portable_filepath(path):
+    return pathlib.Path(path).as_posix()
 
 
 def make_path_spec_relative(filename, spec_dir):
@@ -48,12 +48,6 @@ def make_path_spec_relative(filename, spec_dir):
         return filename
 
 
-# Support for trying to avoid hard-coded paths in the .spec files. Eg, all files rooted in the Installer directory tree
-# will be written using "HOMEPATH", thus allowing this spec file to be used with any Installer installation. Same thing
-# could be done for other paths too.
-path_conversions = ((HOMEPATH, "HOMEPATH"),)
-
-
 class SourceDestAction(argparse.Action):
     """
     A command line option which takes multiple source:dest pairs.
@@ -78,28 +72,7 @@ def __call__(self, parser, namespace, value, option_string=None):
         # replace it before modifying it to avoid changing the default.
         if getattr(namespace, self.dest) is self.default:
             setattr(namespace, self.dest, [])
-        getattr(namespace, self.dest).append((src, dest))
-
-
-def make_variable_path(filename, conversions=path_conversions):
-    if not os.path.isabs(filename):
-        # os.path.commonpath can not compare relative and absolute paths, and if filename is not absolute, none of the
-        # paths in conversions will match anyway.
-        return None, filename
-    for (from_path, to_name) in conversions:
-        assert os.path.abspath(from_path) == from_path, ("path '%s' should already be absolute" % from_path)
-        try:
-            common_path = os.path.commonpath([filename, from_path])
-        except ValueError:
-            # Per https://docs.python.org/3/library/os.path.html#os.path.commonpath, this raises ValueError in several
-            # cases which prevent computing a common path.
-            common_path = None
-        if common_path == from_path:
-            rest = filename[len(from_path):]
-            if rest.startswith(('\\', '/')):
-                rest = rest[1:]
-            return to_name, rest
-    return None, filename
+        getattr(namespace, self.dest).append((portable_filepath(src), portable_filepath(dest)))
 
 
 def removed_key_option(x):
@@ -132,21 +105,6 @@ def __call__(self, *args, **kwargs):
         raise RemovedWinSideBySideSupportError("Please remove your --win-no-prefer-redirects argument.")
 
 
-# An object used in place of a "path string", which knows how to repr() itself using variable names instead of
-# hard-coded paths.
-class Path:
-    def __init__(self, *parts):
-        self.path = os.path.join(*parts)
-        self.variable_prefix = self.filename_suffix = None
-
-    def __repr__(self):
-        if self.filename_suffix is None:
-            self.variable_prefix, self.filename_suffix = make_variable_path(self.path)
-        if self.variable_prefix is None:
-            return repr(self.path)
-        return "os.path.join(" + self.variable_prefix + "," + repr(self.filename_suffix) + ")"
-
-
 # An object used to construct extra preamble for the spec file, in order to accommodate extra collect_*() calls from the
 # command-line
 class Preamble:
@@ -713,7 +671,7 @@ def main(
     # Handle additional EXE options.
     exe_options = ''
     if version_file:
-        exe_options += "\n    version='%s'," % escape_win_filepath(version_file)
+        exe_options += "\n    version=%r," % portable_filepath(version_file)
     if uac_admin:
         exe_options += "\n    uac_admin=True,"
     if uac_uiaccess:
@@ -724,33 +682,31 @@ def main(
         if icon_file[0] == 'NONE':
             exe_options += "\n    icon='NONE',"
         else:
-            exe_options += "\n    icon=[%s]," % ','.join("'%s'" % escape_win_filepath(ic) for ic in icon_file)
+            exe_options += "\n    icon=%r," % [portable_filepath(ic) for ic in icon_file]
         # Icon file for Mac OS.
-        # We need to encapsulate it into apostrofes.
-        icon_file = "'%s'" % icon_file[0]
+        icon_file = repr(portable_filepath(icon_file[0]))
     else:
         # On Mac OS, the default icon has to be copied into the .app bundle.
         # The the text value 'None' means - use default icon.
         icon_file = 'None'
     if contents_directory:
-        exe_options += "\n    contents_directory='%s'," % (contents_directory or "_internal")
+        exe_options += "\n    contents_directory=%r," % (contents_directory or "_internal")
     if hide_console:
-        exe_options += "\n    hide_console='%s'," % hide_console
+        exe_options += "\n    hide_console=%r," % hide_console
 
     if bundle_identifier:
-        # We need to encapsulate it into apostrofes.
-        bundle_identifier = "'%s'" % bundle_identifier
+        bundle_identifier = repr(bundle_identifier)
 
     if manifest:
         if "<" in manifest:
             # Assume XML string
-            exe_options += "\n    manifest='%s'," % manifest.replace("'", "\\'")
+            exe_options += "\n    manifest=%r," % manifest
         else:
             # Assume filename
-            exe_options += "\n    manifest='%s'," % escape_win_filepath(manifest)
+            exe_options += "\n    manifest=%r," % portable_filepath(manifest)
     if resources:
-        resources = list(map(escape_win_filepath, resources))
-        exe_options += "\n    resources=%s," % repr(resources)
+        resources = [portable_filepath(r) for r in resources]
+        exe_options += "\n    resources=%r," % repr(resources)
 
     hiddenimports = hiddenimports or []
     upx_exclude = upx_exclude or []
@@ -761,8 +717,7 @@ def main(
 
     # If script paths are relative, make them relative to the directory containing .spec file.
     scripts = [make_path_spec_relative(x, specpath) for x in scripts]
-    # With absolute paths replace prefix with variable HOMEPATH.
-    scripts = list(map(Path, scripts))
+    scripts = list(map(portable_filepath, scripts))
 
     # Translate the default of ``debug=None`` to an empty list.
     if debug is None:
@@ -778,7 +733,7 @@ def main(
     )
 
     if splash:
-        splash_init = splashtmpl % {'splash_image': splash}
+        splash_init = splashtmpl % {'splash_image': portable_filepath(splash)}
         splash_binaries = "\n    splash.binaries,"
         splash_target = "\n    splash,"
     else:
@@ -791,7 +746,7 @@ def main(
 
     d = {
         'scripts': scripts,
-        'pathex': pathex or [],
+        'pathex': [portable_filepath(i) for i in pathex or []],
         'binaries': preamble.binaries,
         'datas': preamble.datas,
         'hiddenimports': preamble.hiddenimports,
@@ -803,13 +758,13 @@ def main(
         'bootloader_ignore_signals': bootloader_ignore_signals,
         'strip': strip,
         'upx': not noupx,
-        'upx_exclude': upx_exclude,
-        'runtime_tmpdir': runtime_tmpdir,
+        'upx_exclude': [portable_filepath(i) for i in upx_exclude],
+        'runtime_tmpdir': portable_filepath(runtime_tmpdir) if runtime_tmpdir else None,
         'exe_options': exe_options,
         # Directory with additional custom import hooks.
-        'hookspath': hookspath,
+        'hookspath': [portable_filepath(i) for i in hookspath],
         # List with custom runtime hook files.
-        'runtime_hooks': runtime_hooks or [],
+        'runtime_hooks': [portable_filepath(i) for i in runtime_hooks or []],
         # List of modules/packages to ignore.
         'excludes': excludes or [],
         # only Windows and Mac OS distinguish windowed and console apps

news/8004.feature.rst

@@ -0,0 +1 @@
+Use portable (i.e. forward slashes) path separators in generated spec files.

news/8116.bugfix.rst

@@ -0,0 +1,2 @@
+Fix :class:`ValueError` build error on Windows if :option:`--specpath` is set to
+a different drive.

tests/functional/test_cliutils.py

@@ -9,6 +9,8 @@
 # SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
 #-----------------------------------------------------------------------------
 
+import pytest
+
 from PyInstaller.utils.cliutils import makespec
 
 
@@ -37,3 +39,26 @@ def test_makespec_splash(tmpdir, monkeypatch):
     assert spec.exists()
     text = spec.read_text('utf-8')
     assert 'Splash' in text
+
+
+@pytest.mark.win32
+def test_makespec_path_sep_normalisation(tmp_path, monkeypatch):
+    args = [
+        "",
+        r"foo'\bar.py",
+        r"--splash=foo'\bar.png",
+        r"--add-data=foo'\bar:a\b",
+        r"--path=foo'\bar",
+        r"--icon=foo'\bar.png",
+        r"--additional-hooks-dir=foo'\bar",
+        r"--runtime-hook=foo'\bar",
+        r"--upx-exclude=foo'\bar",
+    ]
+    monkeypatch.setattr('sys.argv', args)
+    monkeypatch.setattr('PyInstaller.building.makespec.DEFAULT_SPECPATH', str(tmp_path))
+    makespec.run()
+    spec_contents = (tmp_path / "bar.spec").read_text("utf-8")
+    # All backslashes should have been converted to forward slashes
+    assert "\\" not in spec_contents
+    # Check for syntax errors (most likely from bogus quotes)
+    compile(spec_contents, "", "exec")

tests/unit/test_makespec.py

@@ -17,39 +17,6 @@
 from PyInstaller.building import makespec
 
 
-def test_make_variable_path():
-    p = os.path.join(makespec.HOMEPATH, "aaa", "bbb", "ccc")
-    assert (makespec.make_variable_path(p) == ("HOMEPATH", os.path.join("aaa", "bbb", "ccc")))
-
-
-def test_make_variable_path_regression():
-    p = os.path.join(makespec.HOMEPATH + "aaa", "bbb", "ccc")
-    assert makespec.make_variable_path(p) == (None, p)
-
-
-def test_Path_constructor():
-    p = makespec.Path("aaa", "bbb", "ccc")
-    assert p.path == os.path.join("aaa", "bbb", "ccc")
-
-
-def test_Path_repr():
-    p = makespec.Path(makespec.HOMEPATH, "aaa", "bbb", "ccc")
-    assert p.path == os.path.join(makespec.HOMEPATH, "aaa", "bbb", "ccc")
-    assert (repr(p) == "os.path.join(HOMEPATH,%r)" % os.path.join("aaa", "bbb", "ccc"))
-
-
-def test_Path_repr_relative():
-    p = makespec.Path("aaa", "bbb", "ccc.py")
-    assert p.path == os.path.join("aaa", "bbb", "ccc.py")
-    assert repr(p) == "%r" % os.path.join("aaa", "bbb", "ccc.py")
-
-
-def test_Path_regression():
-    p = makespec.Path(makespec.HOMEPATH + "-aaa", "bbb", "ccc")
-    assert p.path == os.path.join(makespec.HOMEPATH + "-aaa", "bbb", "ccc")
-    assert (repr(p) == repr(os.path.join(makespec.HOMEPATH + "-aaa", "bbb", "ccc")))
-
-
 def test_add_data(capsys):
     """
     Test CLI parsing of --add-data and --add-binary.
@@ -59,9 +26,10 @@ def test_add_data(capsys):
 
     assert parser.parse_args([]).datas == []
     assert parser.parse_args(["--add-data", "/foo/bar:."]).datas == [("/foo/bar", ".")]
-    assert parser.parse_args([r"--add-data=C:\foo\bar:baz"]).datas == [(r"C:\foo\bar", "baz")]
+    if os.name == "nt":
+        assert parser.parse_args([r"--add-data=C:\foo\bar:baz"]).datas == [(r"C:/foo/bar", "baz")]
     assert parser.parse_args([r"--add-data=c:/foo/bar:baz"]).datas == [(r"c:/foo/bar", "baz")]
-    assert parser.parse_args([r"--add-data=/foo/:bar"]).datas == [("/foo/", "bar")]
+    assert parser.parse_args([r"--add-data=/foo/:bar"]).datas == [("/foo", "bar")]
 
     for args in [["--add-data", "foo/bar"], ["--add-data", "C:/foo/bar"]]:
         with pytest.raises(SystemExit):