Skip to content

Comments

Use global debug options instead of project-specific ones#6372

Merged
AdamYoblick merged 2 commits intomicrosoft:masterfrom
AdamYoblick:fix_error_when_attaching
Jan 26, 2021
Merged

Use global debug options instead of project-specific ones#6372
AdamYoblick merged 2 commits intomicrosoft:masterfrom
AdamYoblick:fix_error_when_attaching

Conversation

@AdamYoblick
Copy link
Contributor

@AdamYoblick AdamYoblick commented Jan 25, 2021

Fixes #6369

This is a fix for a regression I introduced when reading variable presentation options from the options page. The bug occurs when attaching to a remote debugpy process instead of launching the debugger directly, which is a case I did not test.

// this should never happen
Debug.Fail("adapterLaunchInfo is not valid json");
// adapterLaunchInfo.LaunchJson can be null when attaching to a remote process
if (adapterLaunchInfo.LaunchJson != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so what is AddDebuggerOptions for?

Copy link
Contributor

Choose a reason for hiding this comment

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

so, if adapterLanguageInfo.launchJson is null, when this is called (

adapterLaunchInfo.LaunchJson = _debugInfo.GetJsonString();
), what do we expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

[JsonObject(MemberSerialization.OptIn)]
internal abstract class DebugInfo {
[JsonProperty("stopOnEntry")]
public bool? StopOnEntry { get; set; }
[JsonProperty("promptBeforeRunningWithBuildError")]
public bool? PromptBeforeRunningWithBuildError { get; set; }
[JsonProperty("redirectOutput")]
public bool? RedirectOutput { get; set; }
[JsonProperty("waitOnAbnormalExit")]
public bool? WaitOnAbnormalExit { get; set; }
[JsonProperty("waitOnNormalExit")]
public bool? WaitOnNormalExit { get; set; }
[JsonProperty("breakOnSystemExitZero")]
public bool? BreakOnSystemExitZero { get; set; }
[JsonProperty("debugStdLib")]
public bool? DebugStdLib { get; set; }
[JsonProperty("showReturnValue")]
public bool? ShowReturnValue { get; set; }
[JsonProperty("subProcess")]
public bool? SubProcess { get; set; }
[JsonProperty("env")]
public Dictionary<string, string> Env { get; set; }
[JsonProperty("rules")]
public IList<PathRule> Rules { get; set; }
[JsonProperty("variablePresentation")]
public VariablePresentation VariablePresentation { get; set; }
public string GetJsonString() {
var jsonSettings = new JsonSerializerSettings() {
TypeNameHandling = TypeNameHandling.None,
NullValueHandling = NullValueHandling.Ignore
};
return JsonConvert.SerializeObject(this, Formatting.Indented, jsonSettings);
}
}
[JsonObject(MemberSerialization.OptIn)]
internal class DebugLaunchInfo : DebugInfo {
[JsonProperty("cwd")]
public string CurrentWorkingDirectory { get; set; }
[JsonProperty("python")]
public List<string> InterpreterPathAndArguments { get; set; }
[JsonProperty("console")]
public string Console { get; set; }
[JsonProperty("program")]
public string Script { get; set; }
[JsonProperty("args")]
public List<string> ScriptArguments { get; set; }
[JsonProperty("django")]
public bool DebugDjango { get; set; }
public string LaunchWebPageUrl { get; set; }
}
[JsonObject(MemberSerialization.OptIn)]
internal class DebugAttachInfo : DebugInfo {
[JsonProperty("host")]
public string Host { get; set; }
[JsonProperty("port")]
public int Port { get; set; }
public Uri RemoteUri { get; set; }
}

Note that there are two different debug info classes there - one for launch, and one for attach. But they have a common base class, and VariablePresentation is there. So it seems like it should be available regardless of how the session gets started.

Copy link
Contributor Author

@AdamYoblick AdamYoblick Jan 25, 2021

Choose a reason for hiding this comment

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

Thanks, makes sense. I guess what confuses me is the two different ways that you can get these options. The one I used in https://github.com/microsoft/PTVS/pull/6337/files seems to be the PythonToolsService, and those options get packed into the adapterLaunchInfo.LaunchJson...but these other options are being read from the global IPythonDebugOptionsService.

It doesn't make sense to me to have two different paths to the same options. Should I just always be using the global one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that I can just add some fields to the IPythonDebugOptionsService to expose the variable presentation stuff that already exists on the DebuggerOptions class. But this means that lots of the other code I added in the previous PR can go away, if we're just always going to be reading these options from the global options service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, good point. This one is really meant for properties that are project-specific; for global properties, I guess it can just read it directly, since there's no ambiguity over which project it comes from.

@karthiknadig might have some thoughts about this.

Copy link
Member

Choose a reason for hiding this comment

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

The two ways were there to handle project specific and global option. It should be fine to read non-project specific properties directly from global location.

PS: This part is also a bit messy because of the way we had to handle ptvsd4 vs debugpy at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thank you everyone, I have a clear path now. Global makes sense to me too, since these options are not project specific. And since they are moving to global in vscode too, I think that's a good approach.

@AdamYoblick AdamYoblick changed the title handle case when attaching instead of launching debugger Use global debug options instead of project-specific ones Jan 25, 2021
@AdamYoblick
Copy link
Contributor Author

Ok this all seems to be working. I tested both when launching the debugger directly through VS as well as attaching to a debugpy process, and both respect the variable presentation options from the Tools -> Options page. So this is ready for review again :)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AdamYoblick
Copy link
Contributor Author

Thanks everyone!

@AdamYoblick AdamYoblick merged commit 0c213e9 into microsoft:master Jan 26, 2021
@AdamYoblick AdamYoblick deleted the fix_error_when_attaching branch January 26, 2021 00:20
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.

Failed to launch debug adapter in romote debugging with debugpy.

5 participants