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

Update SDK #48252

Merged
merged 6 commits into from
May 30, 2023
Merged

Update SDK #48252

merged 6 commits into from
May 30, 2023

Conversation

halter73
Copy link
Member

No description provided.

@halter73 halter73 requested review from a team and wtgodbe as code owners May 15, 2023 23:59
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 15, 2023
@halter73
Copy link
Member Author

@jjonescz @sharwell @333fred @chsienki After updating the .NET SDK from 8.0.100-preview.5.23257.1 to 8.0.100-preview.5.23265.3, we're seeing errors complaining about async without await in the razor source generator.

/mnt/vss/_work/1/s/src/Identity/UI/src/Microsoft.NET.Sdk.Razor.SourceGenerators/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Areas_Identity_Pages_V5_Account_Manage__Layout_cshtml.g.cs(102,46): error CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. [/mnt/vss/_work/1/s/src/Identity/UI/src/Microsoft.AspNetCore.Identity.UI.csproj]

Could this be related to dotnet/razor#8346 which was merged last week and was intended to suppress these warnings?

@jjonescz
Copy link
Member

@halter73 You're right, that bug fix was wrong in some cases. I opened a follow up to fix this: dotnet/razor#8709

@333fred
Copy link
Member

333fred commented May 18, 2023

dotnet/razor#8719 was merged, reverting the change here.

@wtgodbe
Copy link
Member

wtgodbe commented May 18, 2023

dotnet/sdk#32650 brings that change into the SDK. Once that goes in & sdk -> installer flow is done we can grab the new SDK

@wtgodbe
Copy link
Member

wtgodbe commented May 19, 2023

@eerhardt we're starting to see trimming test failures here

@eerhardt
Copy link
Member

I think this looks very similar to dotnet/sdk#32272.

FYI - @nagilson and @sbomer

We could try switching:

<PublishSelfContained>true</PublishSelfContained>

to be <SelfContained>true</SelfContained>.

But honestly - I'm not sure I understand the benefit of PublishSelfContained if this scenario doesn't work. FYI - @baronfel @richlander @dsplaisted

@nagilson
Copy link
Member

@eerhardt
But honestly - I'm not sure I understand the benefit of PublishSelfContained if this scenario doesn't work. FYI

dotnet/sdk#32277 (comment) This comment should explain the feature a bit better, and I agree with your suggestion.

@javiercn
Copy link
Member

@adityamandaleeka can you have someone on your team investigate?

@@ -57,7 +57,7 @@
<_Command Condition=" '$(ProjectAssetsFile)' != '' ">$(_Command) --assets-file "$(ProjectAssetsFile)"</_Command>
<_Command Condition=" '$(PlatformTarget)' != '' ">$(_Command) --platform "$(PlatformTarget)"</_Command>
<_Command Condition=" '$(PlatformTarget)' == '' AND '$(Platform)' != '' ">$(_Command) --platform "$(Platform)"</_Command>
<_Command Condition=" '$(RuntimeIdentifier)' != '' ">$(_Command) --runtime "$(RuntimeIdentifier)"</_Command>
<_Command Condition=" '$(RuntimeIdentifier)' != '' ">$(_Command) --runtime "$(RuntimeIdentifier) --self-contained"</_Command>
Copy link
Member

Choose a reason for hiding this comment

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

I don't know anything about this project. @captainsafia is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this looks correct based on the breaking change details.

@BrennanConroy
Copy link
Member

Had to react to the sdk breaking change: dotnet/docs#33726

@mitchdenny
Copy link
Member

These checks are green but I'm unsure if this is being held up by anything else? @halter73?

@BrennanConroy BrennanConroy merged commit 9123301 into main May 30, 2023
@BrennanConroy BrennanConroy deleted the halter73/update-sdk branch May 30, 2023 17:10
@ghost ghost added this to the 8.0-preview6 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.