Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makespec: Use portable path separators #8116

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

bwoodsend
Copy link
Member

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.

@rokm
Copy link
Member

rokm commented Nov 21, 2023

Hmm, I'm quite sure that the splash image path needs to receive the repr_filepath() treatment as well - as this is what originally sparked #8004.

@rokm
Copy link
Member

rokm commented Nov 21, 2023

Is there a reason why repr() is included in repr_filepath() (i.e., why it returns representation)? I think it would make more sense for the string building part to use %r, while the separator-fixing-function returned only posix path.

(Actually, perhaps it would make sense for all fields in the generated template to use %r as a format string?).

@bwoodsend bwoodsend force-pushed the portable-pathsep branch 2 times, most recently from 836966e to 1e3ce10 Compare November 23, 2023 19:13
@bwoodsend
Copy link
Member Author

Mind if I scrap the whole makespec.make_variable_path() functionality? The only substitution it knows is replacing the directory of PyInstaller itself which no one should have any reason to use?

@rokm
Copy link
Member

rokm commented Nov 24, 2023

Mind if I scrap the whole makespec.make_variable_path() functionality? The only substitution it knows is replacing the directory of PyInstaller itself which no one should have any reason to use?

Hmm, I have a vague recollection of trying to make sense of that part of code sometime back...

As you say, makespec.make_variable_path by default supports only substitution with HOMEPATH. And it seems to be only used by makespec.Path (whose name I absolutely detest), which we apply to entry-point scripts. Which have no business being rooted in PyInstaller's HOMEPATH...

So yeah, go ahead and scrape it.

@rokm
Copy link
Member

rokm commented Nov 24, 2023

Mind if I scrap the whole makespec.make_variable_path() functionality? The only substitution it knows is replacing the directory of PyInstaller itself which no one should have any reason to use?

Hmm, I have a vague recollection of trying to make sense of that part of code sometime back...

As you say, makespec.make_variable_path by default supports only substitution with HOMEPATH. And it seems to be only used by makespec.Path (whose name I absolutely detest), which we apply to entry-point scripts. Which have no business being rooted in PyInstaller's HOMEPATH...

So yeah, go ahead and scrape it.

Ahh, HOMEPATH is not PyInstaller's package directory, but its parent directory (usually site-packages). So this was probably supposed to make spec portable if entry-point script was also in site-packages, "next to PyInstaller". Or maybe if for some reason you checked out pyinstaller repository and then placed your code into it, next to PyInstaller importable package. I still find it rather pointless, though.

Plus, you have a 50% chance of having it work on linux with venv, due to venv/lib64 being symlink to venv/lib, and this code not resolving symlinks. So depending of which absolute path you use, it might do that replacement or not...

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))
Copy link
Member

@rokm rokm Nov 25, 2023

Choose a reason for hiding this comment

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

I think these script paths still need to be made spec-relative, though, because the build process assumes that if they are relative, they are relative to spec directory.

For example, this used to work

pyinstaller --clean --noconfirm program/program.py --specpath spec/

and now it does not (and in contrast to the HOMEPATH thing, I imagine this is used).

Although I suspect that this scenario is party broken anyway, because all given relative paths (icons, etc.) that we later treat as spec-relative would need to be made relative to spec here... But for now, we should at least maintain status quo here.

@rokm
Copy link
Member

rokm commented Nov 25, 2023

Those test failures are not directly related to the PR; the matching pattern in

analysis_toc_file = list((pathlib.Path(tmpdir) / "build/test_source").glob("Analysis-??.toc"))

does not account for numbers greater than 99 (e.g., Analysis-105.toc). This probably depends on the test order (I recall seeing that test fail sporadically on the CI), and the changes made here might have rearranged the tests so that the issue occurs more consistently.

@rokm rokm mentioned this pull request Nov 26, 2023
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.
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.
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)
Copy link
Member

Choose a reason for hiding this comment

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

This repr() needs to be removed as it is now part of the format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants