-
Notifications
You must be signed in to change notification settings - Fork 646
Fix debug file not getting deleted #2332
Fix debug file not getting deleted #2332
Conversation
vladbarosan
commented
Feb 15, 2019
- execute detach even when halt has failed. Fixes Debug file not getting cleared after debugging on Windows #2265
Sorry for getting so late to this one guys, please have a look when you have time |
bc46efa
to
045f23f
Compare
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.
Summarizing all previous discussions here.
- Detach/Restart should only be called if Halt succeeds or fails with the
has exited with status 0
error - Consider Detach (without kill) instead of Restart for remote debugging
- Clear the timer to kill debug process once Halt completes (regardless of success or failure). The main reason this was put in place was for cases when Halt failed.
37e4203
to
f3ff11f
Compare
@ramya-rao-a so there are a couple of scenarios for which I think there are tracking issues:
Right now we do for 1 the a option regardless of "accept-multiclient" and we don't support 2. but 1 would be easy to add if we want and 2 is doable also. @ramya-rao-a @jhendrixMSFT I know for 2 there is an open issue, not sure if there is an ask for 1.b ? Thoughts ? Otherwise let me know if this is good to go. |
f3ff11f
to
b2fceb4
Compare
@ramya-rao-a @jhendrixMSFT as a final thing to close this I added an extra delete if for some reason the file is still around. For htis I moved the |
a47f4a9
to
823cd2d
Compare
@vladbarosan Why not use the built in |
@ramya-rao-a , I was thinking of force removing directories but in this case is always a file so I can. Still the question regarding dependencies stands. |
We dont have any official policy on this yet. The basic idea is that we won't add dependencies that we can avoid, as it will increase the size of the extension payload. Did you have any other specific concerns? |
67b65fc
to
6836de6
Compare
@jhendrixMSFT @ramya-rao-a sorry i got back late to this. I think I addressed the remaining comments. |
@vladbarosan I pushed 2 commits with tiny changes, please take a look and let me know if you have any concerns |
Also, did you get a chance to try the remote debugging scenario? |
@ramya-rao-a but again reattaching in this scenario wont work unless delve is started with multiclient. @jhendrixMSFT is working on the attach feature. I think it might even make sense to remove the remote debugging part from launch and just leave it for attach ( since we aren't really launching) |
@vladbarosan For future PRs, please use the Squash option while merging. |
ah yea, my bad :| , im being used to have only that option enabled, @ramya-rao-a can we enable only squash merge on the repo ? |
Good point. Done. |