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

Debug support #858

Merged
merged 1 commit into from
May 12, 2016
Merged

Debug support #858

merged 1 commit into from
May 12, 2016

Conversation

bjorkstromm
Copy link
Member

@bjorkstromm bjorkstromm commented May 1, 2016

Solves #114

WIP Debug support. Currently only supports Stable version of Roslyn, the Nightly should be a no brainer, but I wanted to discuss this more in detail before implementing.

Done:

  • Added support for --debug (short -d) argument
  • Added Debug command + all that jazz
  • Added DebugScriptHost, which waits for a debugger to be attached
  • Added RoslynDebugSession, which compiles the cake-script and emits the dll + pdb to a memorystream which is loaded and executed.

I'm not sure about the Roslyn Session and SessionFactory implementations. Refactored out the common bits, but still feels kind of wierd? Any suggestions on this before proceeding with the Nightly version of Roslyn.

ALSO Still lacks Unit Tests, some bits should be refactored to allow testing, e.g. DebugScriptHost..

HOWTO:

  • launch cake.exe with -debug flag. cake.exe cakefile.cake -debug
  • Process will stop in prior to executing script and report Attach debugger to process XYZ to continue
  • Open Visual Studio, open the cake file, add a breakpoint.
  • Open Debug > Attach to Process... and attach to process with PID reported in step 2.
  • Enjoy debugging...
    • Sometimes VS will fail and report "Source not found", if this is the case, close the open cake-file and then from the call stack window right click on the first row and select "Go to source". This should open the source file and everything is fine :) Don't know why VS behaves like this, breakpoint hits, but it can't open source when it's already open.
    • Another nice way is to add System.Diagnostics.Debugger.Break(); to the line where you would like the debugger to break. In this case you can completely skip opening the file in VS, the debugging session will open it automatically once breakpoint is hit.

@patriksvensson patriksvensson changed the title WIP: Debug support [WIP] Debug support May 2, 2016
@patriksvensson
Copy link
Member

@mholo65

Great job! 👍

Would it be possible to Rename the RoslynSessionFactory to DefaultRoslynSessionFactory and RoslynSession to DefaultRoslynSession, and after that rename AbstractRoslynSessionFactory to just RoslynSessionFactory and AbstractRoslynSession to RoslynSession.

public sealed class DefaultRoslynSessionFactory : RoslynSessionFactory { }
public sealed class DebugRoslynSessionFactory : RoslynSessionFactory { }

public sealed class DefaultRoslynSession : RoslynSession { }
public sealed class DebugRoslynSession : RoslynSession { }

Not sure that Default is a better prefix, but I'm really not a fan of the Abstract prefix.

@gep13 @devlead: Any input on naming?

@bjorkstromm
Copy link
Member Author

Thanx! Default/Debug sounds better than current naming. Will wait and see what the others have to say about naming.

Another thing that cane to mind. What about adding #break pre-processor directive that would inject System.Diagnostics.Debugger.Break();? That could be a separate PR though.

@devlead
Copy link
Member

devlead commented May 2, 2016

@mholo65 +1 on naming 👍 Any thoughts @gep13?

New preprocessors should as you suggest be part of separate PR, so we keep this one as focused as possible.

@bjorkstromm
Copy link
Member Author

Would you also like to have debug support for experimental (nightly) version of Roslyn as separate PR? It is not yet implemented in this PR.

@patriksvensson
Copy link
Member

@mholo65 I think debug support for experimental Roslyn could be a part of this PR.

@devlead
Copy link
Member

devlead commented May 2, 2016

I agree with @patriksvensson, would be great with support for experimental in this pr too.

@bjorkstromm
Copy link
Member Author

@patriksvensson @devlead Thanks. I'll fix the naming, add experimental roslyn support and add some unit tests. I'll probably have it by the end of this week.

@gep13
Copy link
Member

gep13 commented May 3, 2016

Sorry for not replying sooner, had a couple things on...

Yes, I agree with the suggested naming. I really like the idea of the #debug pre processor as well, I think that will help during debugging sessions, but I agree that this should be a separate PR.

This really is great work @mholo65! This is going to make a lot of people very happy! 👍

private readonly RoslynScriptSessionFactory _stableFactory;
private readonly RoslynNightlyScriptSessionFactory _nightlyFactory;
private readonly RoslynSessionFactory _stableFactory;
private readonly RoslynNightlySessionFactory _nightlyFactory;
Copy link
Member Author

Choose a reason for hiding this comment

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

@patriksvensson hmm, maybe I should keep Script in the new name. RoslynNightlySession sounds ratherh kinky 😄

Copy link
Member

Choose a reason for hiding this comment

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

@mholo65 Lol 😄 Keep the Script 😉

@gep13
Copy link
Member

gep13 commented May 4, 2016

@mholo65 quick random question... will the debugger within say VSCode also be able to attach to this, in the same way as you can with Visual Studio?

@bjorkstromm
Copy link
Member Author

bjorkstromm commented May 4, 2016

@gep13 haven't tried that. But I could investigate.

Edit
@gep13 coreclr-debug (in omnisharp-vscode) gave no luck. Do you know if it's even possible to debug full framework programs in VS Code?

MDbg worked like a charm. Too bad nobody has created an extension for it in VSCode.

{
var strategy = new DefaultExecutionStrategy(_log);
var message = "Attach debugger to process {0} to continue";
var pid = System.Diagnostics.Process.GetCurrentProcess().Id;
Copy link
Member Author

Choose a reason for hiding this comment

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

@patriksvensson any wishes for abstracting this in order to unit test?

@bjorkstromm
Copy link
Member Author

@patriksvensson I think I'm confident enough to remove the [WIP] tag now 😄

//cc @devlead @gep13

@bjorkstromm bjorkstromm changed the title [WIP] Debug support Debug support May 9, 2016
@gep13
Copy link
Member

gep13 commented May 9, 2016

Damn! Really looking forward to taking a look at this, and having a play!

@mholo65 on a side note, did you take a look at using VSCode as the debugger? Is that going to work, or is additional work required? Cheers!

@bjorkstromm
Copy link
Member Author

@gep13 I checked up on VSCode debugging and unfortunately it won't work until Cake is ported to .NET Core as the Omnisharp VSCode Debugger uses CLRDBG which is designed for .NET Core.

However, the .NET Framework Command-Line Debugger, MDbg works like a charm. But this debugger is unfortunately not available as an extension for VSCode.

A third alternative is the Mono debugger, which may work. I'm currently not in a position to test that as I'm lacking an environment atm. (read not supported on Windows)

@bjorkstromm
Copy link
Member Author

@gep13 and forget the third alternative as we don't do the Roslyn dance on Linux/Mac and this PR doesn't implement debugging support for Mono...

@patriksvensson
Copy link
Member

@mholo65 Awesome work! Really impressed. Will take a closer look at it tonight when I'm off work.

@gep13 Have you seen the Mono debug adapter? https://github.com/Microsoft/vscode-mono-debug


namespace Cake.Scripting.Roslyn.Nightly
{
using Core.IO;
Copy link
Member

Choose a reason for hiding this comment

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

Any special reason this isn't by the other using statements above? If not move up so all in one place.

@bjorkstromm
Copy link
Member Author

ping @devlead @patriksvensson @gep13 fixed, rebased and squashed!

@patriksvensson
Copy link
Member

@mholo65 Looks good to me! 😄

@devlead Will you take lead on this?

@devlead
Copy link
Member

devlead commented May 12, 2016

@patriksvensson agree 👍 Just waiting for CI to complete.

@bjorkstromm
Copy link
Member Author

@devlead seems like Travis osx build is stalled.. :(

@devlead
Copy link
Member

devlead commented May 12, 2016

@mholo65 I've restarted the Travis build.

@devlead devlead merged commit f75166c into cake-build:develop May 12, 2016
@devlead
Copy link
Member

devlead commented May 12, 2016

@mholo65 This is now merged 👍 Big thanks for your contribution!

@cake-build/team FYI 1/2 travis builds didn't complete(the Mac build - restarted 4 times), all other CI success and tested locally, so took decision to merge anyway.

@bjorkstromm
Copy link
Member Author

@devlead Thank You! It was a pleasure doing this PR 😄

@devlead devlead mentioned this pull request May 13, 2016
@DamianReeves
Copy link

Is there any plans on producing a write-up on using this feature. At this point debug support and intellisense are the only things blocking me from adopting cake as my go to build tool.

@bjorkstromm
Copy link
Member Author

@DamianReeves yes, there's an issue here cake-build/website#83 to track this.

@devlead
Copy link
Member

devlead commented May 26, 2016

@mholo65 @DamianReeves there will likely be an intro blog post too shortly.

@patriksvensson
Copy link
Member

@DamianReeves A blog post have been posted about this: http://cakebuild.net/blog/2016/05/debug-cake-file

@gep13
Copy link
Member

gep13 commented May 27, 2016

@DamianReeves said...
At this point debug support and intellisense are the only things blocking me from adopting cake as my go to build tool.

As a point of curiosity, what are you currently using as your build tool?

@DamianReeves
Copy link

@gep13 I go back and forth between psake and FAKE

@gep13 gep13 mentioned this pull request Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants