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

Support debug apps running under mono runtime using vsdbg. #6752

Merged
merged 14 commits into from
Dec 20, 2023

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Dec 13, 2023

No description provided.

@thaystg thaystg requested a review from a team as a code owner December 13, 2023 20:14
@thaystg thaystg marked this pull request as draft December 13, 2023 20:55
package.json Outdated
"pickRemoteDockerProcess": "csharp.listRemoteDockerProcess"
},
"configurationAttributes": {
"launch": {
Copy link
Contributor

Choose a reason for hiding this comment

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

The launch and attach configurations are generated with the generateOptionsSchema script. You will want to add updating this debugger too.

// Hard Code adding in configurationAttributes launch and attach.
// .NET Core
packageJSON.contributes.debuggers[0].configurationAttributes.launch = schemaJSON.definitions.LaunchOptions;
packageJSON.contributes.debuggers[0].configurationAttributes.attach = schemaJSON.definitions.AttachOptions;
// Full .NET Framework
packageJSON.contributes.debuggers[1].configurationAttributes.launch = schemaJSON.definitions.LaunchOptions;
packageJSON.contributes.debuggers[1].configurationAttributes.attach = schemaJSON.definitions.AttachOptions;

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you could also undo the Concord-side change to LanchOptions.json, I will leave a comment in the Concord PR with more information.

package.json Outdated Show resolved Hide resolved
package.json Outdated
"required": [
"program"
],
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect users to ever use monovsdbg in a launch.json file? If not, you might not want to document the args.

Copy link
Contributor

@gregg-miskelly gregg-miskelly Dec 14, 2023

Choose a reason for hiding this comment

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

Looking at the implementation, if I understand correctly, a lanunch request is really an attach, with the 'project system' responsible for starting the target process. To me, this suggests we probably don't want anyone directly using this, so we should probably just not document any of the arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

If users are not going to register anything in a schema, is it good enough to just register the DebugConfigurationProvider and DebugAdapterDescriptorFactory for monosvsdbg?

package.json Outdated Show resolved Hide resolved
@@ -357,6 +357,9 @@ export function GenerateOptionsSchema() {
packageJSON.contributes.debuggers[1].configurationAttributes.launch = schemaJSON.definitions.LaunchOptions;
packageJSON.contributes.debuggers[1].configurationAttributes.attach = schemaJSON.definitions.AttachOptions;

// Mono
packageJSON.contributes.debuggers[2].configurationAttributes.launch = schemaJSON.definitions.LaunchOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead we probably don't want to declare what options monovsdbg takes since we don't want people using it directly.

Copy link
Contributor

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Extension code changes look good, but I think we probably don't want to document the launch args for monovsdbg unless I am wrong and we want to allow folks to directly use it, in which case we probably want to filter out all the arguments that are only applicable for CoreCLR.

src/main.ts Outdated
@@ -425,11 +425,17 @@ function tryGetCSharpDevKitExtensionExports(csharpLogObserver: CsharpLoggerObser
);
}

let registered = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we want a comment that this flag is to ensure that profferBrokeredServices is only done once.

package.json Outdated
"required": [
"program"
],
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

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

If users are not going to register anything in a schema, is it good enough to just register the DebugConfigurationProvider and DebugAdapterDescriptorFactory for monosvsdbg?

@thaystg thaystg force-pushed the thays_support_mono_debugging branch 2 times, most recently from 59e62df to f4f9f0f Compare December 19, 2023 21:17
@thaystg thaystg force-pushed the thays_support_mono_debugging branch from 8d28461 to 6bc5dc4 Compare December 19, 2023 21:22
@thaystg thaystg marked this pull request as ready for review December 19, 2023 21:36
@thaystg thaystg marked this pull request as draft December 19, 2023 21:37
@WardenGnaw WardenGnaw dismissed gregg-miskelly’s stale review December 19, 2023 21:58

Due to the holidays and unblocking Thays🎄❄️

@thaystg thaystg force-pushed the thays_support_mono_debugging branch from ae4df7c to 2d0ea06 Compare December 19, 2023 22:24
@thaystg thaystg marked this pull request as ready for review December 19, 2023 22:25
@isadorasophia isadorasophia merged commit 7c7bada into dotnet:main Dec 20, 2023
13 checks passed
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.

5 participants