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

[wasm][debugger] Remove usage of GeneratedRegex #86911

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented May 30, 2023

The P5 BrowserDebugHost is compiled as part of the runtime build but consumed in aspnetcore at a transport package and previously used the RegexSourceGenerator. Because dotnet/runtime builds with the SDK set by arcade in global.json when BrowserDebugHost was compiled with the Preview4 SDK the and regex source generator used IndexOfAnyValues which no longer exists in the P5 runtime after dotnet/docs#35244 so the proxy fails to start using a P5 runtime.

This stops using the source generator for to regex code to avoid the missing symbol.

@ghost
Copy link

ghost commented May 30, 2023

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: thaystg
Assignees: thaystg
Labels:

area-Debugger-mono

Milestone: -

@thaystg
Copy link
Member Author

thaystg commented May 30, 2023

/backport to release/8.0-preview5

@github-actions
Copy link
Contributor

Started backporting to release/8.0-preview5: https://github.com/dotnet/runtime/actions/runs/5125322906

[GeneratedRegex(@"([:/])")]
private static partial Regex RegexForEscapeFileName();
#pragma warning disable SYSLIB1045
private static readonly Regex regexForEscapeFileName = new Regex(@"([:/])");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static readonly Regex regexForEscapeFileName = new Regex(@"([:/])");
private static readonly Regex s_regexForEscapeFileName = new(@"([:/])");

private static partial Regex RegexForReplaceVarName();

#pragma warning disable SYSLIB1045
private static Regex regexForReplaceVarName = new Regex(@"[^A-Za-z0-9_]", RegexOptions.Singleline);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static Regex regexForReplaceVarName = new Regex(@"[^A-Za-z0-9_]", RegexOptions.Singleline);
private static Regex s_regexForReplaceVarName = new(@"[^A-Za-z0-9_]", RegexOptions.Singleline);

[GeneratedRegex(@"\<(?<varName>[^)]*)\>(?<varId>[^)]*)(__)(?<scopeId>\d+)", RegexOptions.Singleline)]
private static partial Regex RegexForAsyncLocals(); //<testCSharpScope>5__1 // works
#pragma warning disable SYSLIB1045
private static Regex regexForAsyncLocals = new Regex(@"\<(?<varName>[^)]*)\>(?<varId>[^)]*)(__)(?<scopeId>\d+)", RegexOptions.Singleline);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static Regex regexForAsyncLocals = new Regex(@"\<(?<varName>[^)]*)\>(?<varId>[^)]*)(__)(?<scopeId>\d+)", RegexOptions.Singleline);
private static Regex s_regexForAsyncLocals = new(@"\<(?<varName>[^)]*)\>(?<varId>[^)]*)(__)(?<scopeId>\d+)", RegexOptions.Singleline);

.. and similar for all the other instances.

@radical
Copy link
Member

radical commented May 30, 2023

What prompted this change?

@stephentoub
Copy link
Member

Because dotnet/runtime builds with the SDK set by arcade in global.json when BrowserDebugHost was compiled with the Preview4 SDK the and regex source generator used IndexOfAnyValues which no longer exists in the P5 runtime after dotnet/docs#35244 so the proxy fails to start using a P5 runtime.
This stops using the source generator for to regex code to avoid the missing symbol.

I didn't fully follow this. We're using P4 components with P5? Should this change only be made to the P5 branch rather than to main?

Note that IndexOfAnyValues still exists but was renamed to SearchValues.

@thaystg
Copy link
Member Author

thaystg commented May 31, 2023

Yeah. I don't want to merge it on main.

@thaystg thaystg marked this pull request as draft May 31, 2023 15:49
@lewing
Copy link
Member

lewing commented Jun 1, 2023

Because dotnet/runtime builds with the SDK set by arcade in global.json when BrowserDebugHost was compiled with the Preview4 SDK the and regex source generator used IndexOfAnyValues which no longer exists in the P5 runtime after dotnet/docs#35244 so the proxy fails to start using a P5 runtime.
This stops using the source generator for to regex code to avoid the missing symbol.

I didn't fully follow this. We're using P4 components with P5? Should this change only be made to the P5 branch rather than to main?

Note that IndexOfAnyValues still exists but was renamed to SearchValues.

The issue was that the debug proxy was built with the sdk specified in global.json (which is p4 on the release/8.0-preview5 branch) so it generated source using IndexOfAnyValues, that binary was repackaged in aspnetcore then run against a p5 runtime where IndexOfAnyValues did not exist. We currently build it with the system/.dotnet sdk to avoid depending on local coreclr and source generator builds as part of the debugger lane. We hadn't really considered this mode of breakage because it hadn't come up before.

@stephentoub
Copy link
Member

Thanks for the explanation.

This PR can be closed then, as the change was merged into the preview branch, yes?

@thaystg
Copy link
Member Author

thaystg commented Jun 5, 2023

Thanks for the explanation.

This PR can be closed then, as the change was merged into the preview branch, yes?

@lewing asked me to keep it as a draft. I think we will be able to close it soon.

@thaystg
Copy link
Member Author

thaystg commented Jun 20, 2023

I'll need to merge it because preview 6 is still broken. I will revert theses changes once the SDK used to build the BrowserDebugProxy is higher than preview 5.

@thaystg thaystg marked this pull request as ready for review June 20, 2023 16:58
@thaystg
Copy link
Member Author

thaystg commented Jun 20, 2023

I'll revert it once preview 6 is branched. Preview 6 is not using SDK from preview 5 yet.

@thaystg thaystg merged commit 3fc0f5f into dotnet:main Jun 20, 2023
thaystg added a commit that referenced this pull request Jul 17, 2023
thaystg added a commit that referenced this pull request Jul 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants