Fix debug file not getting deleted

[Vlad Barosan (vladbarosan)]

• execute detach even when halt has failed. Fixes Debug file not getting cleared after debugging on Windows #2265

[Vlad Barosan (vladbarosan)]

Sorry for getting so late to this one guys, please have a look when you have time

[Ramya Rao (ramya-rao-a)]

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.

[Vlad Barosan (vladbarosan)]

Ramya Rao (@ramya-rao-a) so there are a couple of scenarios for which I think there are tracking issues:

1. User starts delve with an executable that runs and finishes or some code that runs only once
   a. in this case normally we can just halt and detach which would close the target and delve remotely ( so users would need to restart delve with the program )
   b. if delve is started with --accept-multiclient, we could actually restart without halting or detaching so then on new attaches the code is rerun.

2. Program starts and executes and we just "attach" and "detach".

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 (@ramya-rao-a) Joel Hendrix (@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.

[Vlad Barosan (vladbarosan)]

Ramya Rao (@ramya-rao-a) Joel Hendrix (@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 fs-extra as a dependency ( instead of a dev dependency). Any issue with that? and in general, ill take the occasion to ask what is our policies on adding dependencies ( I saw they are fairly minimal right now ).

[Ramya Rao (ramya-rao-a)]

Vlad Barosan (@vladbarosan) Why not use the built in fs module instead?

[Vlad Barosan (vladbarosan)]

Ramya Rao (@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.

[Ramya Rao (ramya-rao-a)]

what is our policies on adding dependencies

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?

[Vlad Barosan (vladbarosan)]

Joel Hendrix (@jhendrixMSFT) Ramya Rao (@ramya-rao-a) sorry i got back late to this. I think I addressed the remaining comments.

[Ramya Rao (ramya-rao-a)]

Vlad Barosan (@vladbarosan) I pushed 2 commits with tiny changes, please take a look and let me know if you have any concerns

[Ramya Rao (ramya-rao-a)]

Also, did you get a chance to try the remote debugging scenario?

[Vlad Barosan (vladbarosan)]

Ramya Rao (@ramya-rao-a) but again reattaching in this scenario wont work unless delve is started with multiclient. Joel Hendrix (@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)

[Ramya Rao (ramya-rao-a)]

Vlad Barosan (@vladbarosan) For future PRs, please use the Squash option while merging.

[Vlad Barosan (vladbarosan)]

ah yea, my bad :| , im being used to have only that option enabled, Ramya Rao (@ramya-rao-a) can we enable only squash merge on the repo ?

[Ramya Rao (ramya-rao-a)]

Good point. Done.