building: allow clickable .app on OSX #5419
Conversation
There was a problem hiding this comment.
Thanks for this pull-request. I only skimmed over the related issue discussion.
To be frank, I dislike adding another "top-level" API type for just adding a file and a plist-entry. Instead of this new type, I suggest adding just another (boolean) option to BUNDLE. This will also become the name of the command-line option. Please propose an argument name.
For this pull-request to be merged, please
- implement above change
- the option needs to be documented in the code (doc-string) and the manual (spec-files.rst).
- commit message should be something like "building: Add option --name-of-new-otpion"
- submit a changelog entry so our users can learn about your change.
I'm looking forward for your update
PyInstaller/building/osx.py
Outdated
| shell_script = """#!/bin/bash | ||
| dir=$(dirname $0) | ||
| open -a Terminal \"file://${dir}/%s\"""" % self.appname |
There was a problem hiding this comment.
- Using different quotes makes it easer to read
- file:-URL needs absolute path
- filename needs to be shell-quoted
| shell_script = """#!/bin/bash | |
| dir=$(dirname $0) | |
| open -a Terminal \"file://${dir}/%s\"""" % self.appname | |
| shell_script = '''#!/bin/bash | |
| dir=$(realpath $(dirname $0)) | |
| open -a Terminal "file://${dir}/%s"''' % shlex.quote(self.appname) |
There was a problem hiding this comment.
Hmmm, realpath is part of coreutils on linux, and does not seem to be available on macOS.
How about:
dir=$(cd "$( dirname "${0}")" && pwd )(Note the quotes around arguments to be passed to dirname and to cd - having spaces in the path to .app bundle is a pain...)
PyInstaller/building/osx.py
Outdated
| # write Info.plist | ||
| with open(app_path / 'Contents/Info.plist', 'wb') as f: | ||
| pl['CFBundleExecutable'] = 'wrapper' | ||
| plistlib.dump(pl, f) |
There was a problem hiding this comment.
PyInstaller already creates this file. So you just need to add the element to the info_plist argument in __init__(). See https://pyinstaller.readthedocs.io/en/stable/spec-files.html#spec-file-options-for-a-mac-os-x-bundle for an example.
PyInstaller/building/osx.py
Outdated
| shell_script = """#!/bin/bash | ||
| dir=$(dirname $0) | ||
| open -a Terminal \"file://${dir}/%s\"""" % self.appname | ||
| with open(app_path / 'Contents/MacOS/wrapper', 'w') as f: |
There was a problem hiding this comment.
| with open(app_path / 'Contents/MacOS/wrapper', 'w') as f: | |
| wrapper_script = app_path / 'Contents/MacOS/wrapper' | |
| with open(wrapper_script, 'w') as f: |
and use wrapper_script for chmod below.
PyInstaller/building/osx.py
Outdated
|
|
||
| # write Info.plist | ||
| with open(app_path / 'Contents/Info.plist', 'wb') as f: | ||
| pl['CFBundleExecutable'] = 'wrapper' |
There was a problem hiding this comment.
Can I also request that this pl['CFBundleExecutable'] = 'wrapper' line move above the with open(...): line so that it's clear it's not part of the process of writing the file.
Legorooj
force-pushed
the
develop
branch
4 times, most recently
from
3a8eccd
to
e20e74c
Compare
|
Hmm, doesn't seem to work if program name contains spaces. E.g., trying to freeze |
|
Ah, I was going to request that you change the test so that it uses a name with spaces so we can be sure that the |
PyInstaller/building/osx.py
Outdated
| # write new wrapper script | ||
| shell_script = '''#!/bin/bash | ||
| dir=$(cd "$( dirname "${0}")" && pwd ) | ||
| open -a Terminal "file://${dir}/%s"''' % shlex.quote(self.appname) |
There was a problem hiding this comment.
It turns out this shlex.quote() call is redundant (because the string is already in double quotes), and is actually harmful because if self.appname contains a space, it will add single quotes around the name. So we end up with open -a Terminal "file://${dir}/'program with space'" (i.e., single quotes within double quotes), which leads to an error.
And it also seems that if file:// uri schema is used, the spaces need to be url-encoded (i.e., %20), otherwise we get an error as well. I'm not sure if url-encoding from bash is feasible, though (we could url-encode self.appname on python side, but if self.appname contains a space, so will ${dir} on the script side...). Removing the file:// schema seems to make non-encoded spaces work...
There was a problem hiding this comment.
so removing file:// and shelex.quote seems to fix things? I will try that
tests/functional/test_basic.py
Outdated
| pyi_builder.test_source(""" | ||
| print('Hello Python!') | ||
| input() | ||
| """, pyi_args=args) |
There was a problem hiding this comment.
This looks suspicious, as it will wait for input forever. And pyi_builder.test_source() probably runs the onedist/onefile version of the build (instead of app bundle, which is created in addition to one of those builds), so the test will be stuck until it times out. Even if the bundle was ran instead, it would remain open forever, and we'd have no way of knowing its status (because opening a bundle returns immediately)...
So the test should be modeled after tests from test_apple_events.py: freeze an app bundle, whose code checks if console is available (e.g., by checking sys.stdin.isatty()), and writes the console status into a file (whose path can be injected into the script source via format(), or perhaps via environment variables); the main test then calls the bundle via subprocess and open call, then waits for some time (10 seconds?), and checks if file exists and if its content indicates that the console was available to the bundle.
This way, we'll know that the app bundle ran successfully and that it had console.
And the test app bundle should have a space in its name, to explicitly test for that.
Work in progress, addressing issue #5154