From 48969d95fce02ec55a67cf7e0a785047cb61e5ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Br=C3=A9nainn=20Woodsend?= Date: Tue, 21 Nov 2023 01:21:58 +0000 Subject: [PATCH 1/2] makespec: Use portable path separators. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use forward slashes – even on Windows – in filenames in generated spec files so that the spec file is independent of the platform it was generated. --- PyInstaller/building/makespec.py | 46 +++++++++++++++---------------- news/8004.feature.rst | 1 + tests/functional/test_cliutils.py | 25 +++++++++++++++++ 3 files changed, 48 insertions(+), 24 deletions(-) create mode 100644 news/8004.feature.rst diff --git a/PyInstaller/building/makespec.py b/PyInstaller/building/makespec.py index 790e1a9477..3b40610d5b 100644 --- a/PyInstaller/building/makespec.py +++ b/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): @@ -78,7 +78,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)) + getattr(namespace, self.dest).append((portable_filepath(src), portable_filepath(dest))) def make_variable_path(filename, conversions=path_conversions): @@ -143,8 +143,8 @@ 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) + ")" + return repr(portable_filepath(self.path)) + return "os.path.join(%s, %r)" % (self.variable_prefix, portable_filepath(self.filename_suffix)) # An object used to construct extra preamble for the spec file, in order to accommodate extra collect_*() calls from the @@ -713,7 +713,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 +724,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 [] @@ -778,7 +776,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 +789,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 +801,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 diff --git a/news/8004.feature.rst b/news/8004.feature.rst new file mode 100644 index 0000000000..d5df30013c --- /dev/null +++ b/news/8004.feature.rst @@ -0,0 +1 @@ +Use portable (i.e. forward slashes) path separators in generated spec files. diff --git a/tests/functional/test_cliutils.py b/tests/functional/test_cliutils.py index 9fab53ff83..d4b33018ed 100644 --- a/tests/functional/test_cliutils.py +++ b/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") From b825ef682b94885ecb242167b9e864e59a42d5ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Br=C3=A9nainn=20Woodsend?= Date: Fri, 24 Nov 2023 21:25:46 +0000 Subject: [PATCH 2/2] makespec: Remove *variable path* substitution. Remove the automatic substitution of PyInstaller.HOME_PATH (most likely the ``site-packages`` directory) in relative CLI path parameters to into a relocatable ``os.path.join(HOME_PATH, ...)``. I can't think of any good reason why anyone would even notice this feature exists whereas it's more likely that a PyInstaller test would unwittingly run into it. --- PyInstaller/building/makespec.py | 45 +------------------------------- news/8116.bugfix.rst | 2 ++ tests/unit/test_makespec.py | 38 +++------------------------ 3 files changed, 6 insertions(+), 79 deletions(-) create mode 100644 news/8116.bugfix.rst diff --git a/PyInstaller/building/makespec.py b/PyInstaller/building/makespec.py index 3b40610d5b..7c71c947fd 100644 --- a/PyInstaller/building/makespec.py +++ b/PyInstaller/building/makespec.py @@ -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. @@ -81,27 +75,6 @@ def __call__(self, parser, namespace, value, option_string=None): getattr(namespace, self.dest).append((portable_filepath(src), portable_filepath(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 - - def removed_key_option(x): from PyInstaller.exceptions import RemovedCipherFeatureError raise RemovedCipherFeatureError("Please remove your --key=xxx argument.") @@ -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(portable_filepath(self.path)) - return "os.path.join(%s, %r)" % (self.variable_prefix, portable_filepath(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: @@ -759,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: diff --git a/news/8116.bugfix.rst b/news/8116.bugfix.rst new file mode 100644 index 0000000000..a1f867fcaa --- /dev/null +++ b/news/8116.bugfix.rst @@ -0,0 +1,2 @@ +Fix :class:`ValueError` build error on Windows if :option:`--specpath` is set to +a different drive. diff --git a/tests/unit/test_makespec.py b/tests/unit/test_makespec.py index 5bea4e19df..4e5f49dfb7 100644 --- a/tests/unit/test_makespec.py +++ b/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):