-
-
Notifications
You must be signed in to change notification settings - Fork 391
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
Add --debug
option to briefcase run
#2173
base: main
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.
I know this is marked as a draft - but it was sufficiently exciting that I've taken a quick look - it's really impressive work! I've dropped a couple of questions and notes inline.
The biggest note is about the broad structure of the debugger module itself. Whenever Briefcase adds a feature like this that involves an interface and one or more implemenations, we try to do so in a way that allows a third party to use the same interface to provide their own implementations. For example, there's an interface for defining GUI bootstraps - we provide one for Toga, and for PySide; but third parties can provide their own implementations of that bootstrap.
A similar approach is needed here. This PR is adding 2 implementations - PDB and DebugPy - of a generic "debugger" interface. Those two implementations should be registering against a generic registration interface; and Briefcase itself should be loading possible debuggers from that interface. There will only be 2 for now; we might add others into Briefcase's codebase in future (like a PyCharm implementation); but any third party should be able to write their own.
This will likely also lead to some simplification of code - the debugger.py
module currently has a bunch of "if PDB" type logic - that's really "call the X method on the debugger interface". That helps to isolate the PDB logic into a PDB module; and makes the debugger itself easier to write, because you can have a "dummy" debugger that exists for testing purposes.
Some other high level notes, based on comments you've left in the PR, and the PR description:
- Resolving the Android issue, and moving to a
.pth
based implementation will likely be a pre-requisite for merging this. If you can elaborate on what the problem is, we might be able to work out what is going on - it's possible it might be an edge case in site handling that we're not handling correctly for the Android case. - Don't worry about physical iPhone testing - if it works on simulators, that's more than enough to warrant merging; and I'm happy to take on any additional testing needed to verify something works on iOS devices.
- Regarding configuration - I agree that this PR doesn't need to solve the "user-level configuration" problem. However, I think the better "workaround" solution in this case is command line options. If we add this to pyproject.toml, it's difficult to walk that back later; but regardless of whether there is a user-level configuration mechanism, there will be a need to manually handle command line overrides to that configuration.
:param host_port: The port on the host | ||
""" | ||
try: | ||
# TODO: This prints the port to the terminal. How to remove the output? |
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.
I suspect what you're looking for is subprocess.check_output
rather than subprocess.run
. That will capture all the output of the command that is executed, returning the command output so it can be parsed/handled (if needed), but only surfacing the output to the user if an error is raised by the command.
src/briefcase/debugger.py
Outdated
debugger_cfg = DebuggerConfig.from_app(app) | ||
|
||
startup_modul = "__briefcase_startup__" | ||
startup_code_path = app_path / f"{startup_modul}.py" |
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.
Two notes here:
- There's a typo on
startup_module
- I'd be inclined to simplify this to just
__briefcase__
. - Although you're using it for debug content here, it could also be used for a
remote control
testing feature that we're potentially looking at adding. To that end, the code to install/remove the debugger bootstrap should probably be onbase.py
as a generic capability that an app can invoke under different circumastances.
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.
I'd be inclined to simplify this to just
__briefcase__
.
Double-underscore names are reserved for the Python language, so _briefcase
would be better.
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.
Good point - and it would also match the broad pattern of .pth
files that are installed by venv etc as part of "environment setup."
To the problem with .pth files on Android: When processing the .pth file I get the following error before the main module is even started. On windows executing the startup code via .pth is no problem. I guess that
|
Thanks, this is indeed a bug in the .pth initialization order on Android (chaquo/chaquopy#1338). Until this is fixed in Chaquopy, the only workaround I can think of is to import the startup module manually in the app code. So the module could catch the |
- changed command line from `--debug` to `--remote-debugger="REMOTE-DEBUGGER-CONFIG"` - removed configuration via pyproject.toml
This is an first attempt for adding (remote) debugging support for bundled apps. This is not ready yet but i would like to share this anyways.
It supports:
I have tested the following platforms:
The following points are missing:
import __briefcase_startup__
in your__main__.py
. Instead of this it is possible to use .pth files. On windows it is working, but this creates at least problems on Android... I dont have tested iOS and macOS yet.--debug
option tobriefcase run
#2147 (comment)pyproject.toml
is probably not in the right place for the configuration. @freakboy3742 suggested to add user-level configuration. But this is totally out of scope for this PR. So I used thepyproject.toml
even if it is not ideal.data_files
are not correctly working (eg. withpydevd-pycharm
) #2152).Fixes #2147
PR Checklist: