Fix for issue #11396

[adam-zlatniczki]

Fixes #11396

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
• Title summarizes what is changing.
• Has a news entry file (remember to thank yourself!).
• Appropriate comments and documentation strings in the code.
• Has sufficient logging.
• Has telemetry for feature-requests.
• Unit tests & system/integration tests are added/updated.
• Test plan is updated as appropriate.
• package-lock.json has been regenerated by running npm install (if dependencies have changed).

[ghost]

All CLA requirements met.

[roblourens]

Thanks for the investigation and fix! I see a bunch of places around the debugger that use the path module, and they are all possibly suspicious. You haven't seen any other issues while debugging cross-OS? Also, are you sure there is no scenario where it's needed? I can't think of anything, I found this commit from the PR that originally added it, e9bfc0d, but no explanation unfortunately

[adam-zlatniczki]

Thanks for the investigation and fix! I see a bunch of places around the debugger that use the path module, and they are all possibly suspicious. You haven't seen any other issues while debugging cross-OS? Also, are you sure there is no scenario where it's needed? I can't think of anything, I found this commit from the PR that originally added it, e9bfc0d, but no explanation unfortunately

Regarding the kernelDebugAdapter.ts for notebooks, I don't really see any risks: if you search for fileToCell and cellToFile, the only results are in the kernelDebugAdapter and kernelDebugAdapterBase, and they are only used as lookups. Manual testing also didn't reveal any issues for me.

Now that you mentioned it I got curious, so I played around a bit with different setups and debug scenarios. It looks like there's a problem with debugging inside an interactive window kernel too. Judging by a quick glance at the error log, it seems to be something with the path convention too, but I need to dig deeper into this. I'll be getting back to you as soon as I discover something.

[roblourens]

I figured out why we have that, it's because dumpCell used to return mixed slashes, \ and / on windows, and those need to be normalized before we use them to set breakpoints. I filed this issue before realizing it was fixed this year: ipython/ipykernel#995 I think we need to keep the code because there will be plenty of users with environments that have the bug.

Instead of calling the generic normalize method, we can do a simple check for whether the path looks like a windows path with a drive letter (matches [a-z]:\\) and call the correct normalize method.

If you see something weird going on with the interactive window too, this call is suspicious

vscode-jupyter/src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts

Line 93 in 947e18f

const norm = path.normalize((response as IDumpCellResponse).sourcePath);

[adam-zlatniczki]

Ooh, I see, that ipython issue is something that I wouldn't have figured out in a thousand years. I guess this renders my pull-request obsolote. If you agree, I'll remove the PR and move this conversation to the comment section of issue #11396. In the meantime I did some preliminary investigation for the interactive window debugger problem. I also started exactly where you pointed to, but it didn't solve the problem entirely. After normalization is done properly, there are no failed requests on the kernel side, but the UI still ignores the breakpoints. Unfortunately I wasn't able to figure out why, but I saw something interesting: the notebook uri looks like file:///c:/... Notice that there's an extra /, making the uri translate to a file located at /c:/..., which is clearly an invalid windows path. Any ideas on this? Rob Lourens ***@***.***> ezt írta (időpont: 2022. szept. 21., Sze 5:13):

…

I figured out why we have that, it's because dumpCell used to return mixed slashes, \ and / on windows, and those need to be normalized before we use them to set breakpoints. I filed this issue before realizing it was fixed this year: ipython/ipykernel#995 <ipython/ipykernel#995> I think we need to keep the code because there will be plenty of users with environments that have the bug. Instead of calling the generic normalize method, we can do a simple check for whether the path looks like a windows path with a drive letter (matches [a-z]:\\) and call the correct normalize method. If you see something weird going on with the interactive window too, this call is suspicious https://github.com/microsoft/vscode-jupyter/blob/947e18f8b0252877362ea2491019b9fb2872e755/src/interactive-window/debugger/jupyter/kernelDebugAdapter.ts#L93 — Reply to this email directly, view it on GitHub <#11397 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AITBLK6BMXJ5CFQK3BG3FBDV7J4OPANCNFSM6AAAAAAQOLNX4U> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[roblourens]

Continuing in the issue