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

[release/7.0] Fixes not find assembly and pdb if it's not using the default path #92955

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Oct 3, 2023

Customer Impact

Fixes #93016
When a customer is setting another path for the assets during the Blazor start the debugger is not working.

await Blazor.start({
    loadBootResource: function (type: string, name: string, defaultUri: string, integrity: string) {
        return `${defaultUri.replace("_framework", "assets/dotnet")}?${integrity}`;
    }
});

Testing

Manually tested.

Risk

Low. Only moving the try catch.

@thaystg thaystg requested a review from lewing October 3, 2023 18:32
@thaystg thaystg requested a review from radical as a code owner October 3, 2023 18:32
@ghost ghost assigned thaystg Oct 3, 2023
@ghost
Copy link

ghost commented Oct 3, 2023

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

Issue Details

Fixes #93016

When a customer is setting another path for the assets during the Blazor start the debugger is not working.

await Blazor.start({
    loadBootResource: function (type: string, name: string, defaultUri: string, integrity: string) {
        return `${defaultUri.replace("_framework", "assets/dotnet")}?${integrity}`;
    }
});
Author: thaystg
Assignees: thaystg
Labels:

area-Debugger-mono

Milestone: -

@thaystg thaystg added the Servicing-consider Issue for next servicing release review label Oct 3, 2023
}
catch (Exception ex)
{
logger.LogError($"Failed to load {step.Url} ({ex.Message})");
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why ex is not passed as an argument to LogError? This might provide the user with more information?

}
catch (Exception ex)
{
logger.LogError($"Failed to load {step.Url} ({ex})");
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I meant. I would expect that to should up in the output of the console application that is being launched when running the debugger? Now also wondering if the e should also be passed to the logger.LogDebug method.

if (tryUseDebuggerProtocol)
{
string unescapedFileName = Uri.UnescapeDataString(step.Url);
bytes = await context.SdbAgent.GetBytesFromAssemblyAndPdb(Path.GetFileName(unescapedFileName), token);
Copy link
Member

Choose a reason for hiding this comment

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

ConfigureAwait(false)

@dlemstra
Copy link
Contributor

dlemstra commented Oct 3, 2023

And thanks for this quick fix thaystg. Happy that you could reproduce this error and find what was causing this so quickly. I am wondering if extra unit tests should be added for this?

@thaystg
Copy link
Member Author

thaystg commented Oct 3, 2023

And thanks for this quick fix thaystg. Happy that you could reproduce this error and find what was causing this so quickly. I am wondering if extra unit tests should be added for this?

This is a scenario that has a different implementation for .net8, which doesn't matter the file url, I don't think we need unit tests for it.

@carlossanlop
Copy link
Member

@thaystg has all feedback been addressed? If yes, can you please send an email to Tactics requesting approval?

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 16, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email.

@carlossanlop carlossanlop merged commit 317a01f into dotnet:release/7.0-staging Oct 16, 2023
68 of 77 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Debugger-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants