-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
building: allow clickable .app on OSX #5419
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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