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

PR: Fix debug filename path for remote debugging #389

Merged
merged 12 commits into from
Jul 1, 2022

Conversation

impact27
Copy link
Contributor

On linux when processing window paths:

IPdb [5]: os.path.abspath("C:/Users")
Out  [5]: '/home/debian/spyder-kernels/C:/Users'

This PR changes the filename handling to fix spyder-ide/spyder#18330

# Tell IPython to hide this frame (>7.16)
__tracebackhide__ = True
ipython_shell = get_ipython()
if filename is None:
filename = get_current_file_name()
if filename is None:
return
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not needed because remote filename and canonic filename are separated

@@ -655,7 +657,7 @@ def debugfile(filename=None, args=None, wdir=None, post_mortem=False,
if shell.is_debugging():
# Recursive
code = (
"runfile({}".format(repr(normalise_filename(filename))) +
Copy link
Contributor Author

@impact27 impact27 Jun 20, 2022

Choose a reason for hiding this comment

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

removed normalise_filename because filename is now the remote one, unchanged

@dalthviz dalthviz added the type:Bug Something isn't working label Jun 23, 2022
@dalthviz dalthviz added this to the v2.3.2 milestone Jun 23, 2022
@ccordoba12 ccordoba12 changed the title PR: Fix debug path PR: Fix debug filename path for remote debugging Jun 23, 2022
@impact27
Copy link
Contributor Author

I think the debugger._user_requested_quit = False line in spyderpdb.py is useless and can be removed

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your help with this @impact27! Just two tiny suggestions for you.

spyder_kernels/customize/spydercustomize.py Outdated Show resolved Hide resolved
spyder_kernels/customize/spyderpdb.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

@impact27, how confident do you feel about these changes for a bug fix release? The thing is I wouldn't like to break our debugger just to fix a bug for remote environments. That's important, of course, but maintaining our debugger's reliability is more important at this point in the Spyder 5 development cycle.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@impact27
Copy link
Contributor Author

@impact27, how confident do you feel about these changes for a bug fix release? The thing is I wouldn't like to break our debugger just to fix a bug for remote environments. That's important, of course, but maintaining our debugger's reliability is more important at this point in the Spyder 5 development cycle.

I mean it touches file paths, so there might be some hard-to-understand problem. I am relatively confident but because it touches file paths I am not completely comfortable.

@ccordoba12
Copy link
Member

I think the debugger._user_requested_quit = False line in spyderpdb.py is useless and can be removed

I missed this comment, sorry. It's up to you if you want to remove it as part of this PR.

@impact27
Copy link
Contributor Author

impact27 commented Jul 1, 2022

I missed this comment, sorry. It's up to you if you want to remove it as part of this PR.

I searched for _user_requested_quit. The only place it is used is in pdb.py::main(). The relevant code is:

if run_as_module:
    pdb._runmodule(mainpyfile)
else:
    pdb._runscript(mainpyfile)
if pdb._user_requested_quit:
    break

_user_requested_quit is set in both _runmodule and _runscript. I therefore not see the point in setting it in our functions.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @impact27!

@ccordoba12 ccordoba12 merged commit f35d7bc into spyder-ide:2.x Jul 1, 2022
ccordoba12 added a commit that referenced this pull request Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants