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 the sdk build to target net8.0 #28563

Merged
merged 18 commits into from
Nov 21, 2022
Merged

Update the sdk build to target net8.0 #28563

merged 18 commits into from
Nov 21, 2022

Conversation

marcpopMSFT
Copy link
Member

No description provided.

@marcpopMSFT
Copy link
Member Author

@dougbu do you happen to know how to avoid the aspnet analyzer exceptions? This is our first try to move the sdk to targeting 8.0 and using the 8.0 sdk.
CSC : error AD0001: Analyzer 'Microsoft.AspNetCore.Analyzers.Http.RequestDelegateReturnTypeAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'. [D:\a\1\s\src\Tests\Microsoft.AspNetCore.Watch.BrowserRefresh.Tests\Microsoft.AspNetCore.Watch.BrowserRefresh.Tests.csproj]
##[error]CSC(0,0): error AD0001: (NETCORE_ENGINEERING_TELEMETRY=Build) Analyzer 'Microsoft.AspNetCore.Analyzers.Http.RequestDelegateReturnTypeAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.

@dougbu
Copy link
Member

dougbu commented Oct 13, 2022

@marcpopMSFT I think @JamesNK and @captainsafia are our HTTP analyzer specialists. See a few commits from @halter73 too. Hope one of them can help you.

@JamesNK
Copy link
Member

JamesNK commented Oct 13, 2022

I’ll take a look.

@JamesNK
Copy link
Member

JamesNK commented Oct 14, 2022

I've identified the problem. The fix is in aspnetcore so you'll need to wait for the update to flow through.

@marcpopMSFT
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcpopMSFT
Copy link
Member Author

@JamesNK the SDK main branch is on aspnetcore 8.0.0-alpha.1.22517.3 and I tried rerunning this PR but it still failed with the same error. What else needs to be done to unblock this?

@JamesNK
Copy link
Member

JamesNK commented Oct 17, 2022

When I ran build on main I got 8.0.0-alpha.1.22512.9 of the SDK restored. Its commit was from before my changes - dotnet/aspnetcore@361e628

@JamesNK
Copy link
Member

JamesNK commented Oct 18, 2022

When I ran build on main I got 8.0.0-alpha.1.22512.9 of the SDK restored. Its commit was from before my changes - dotnet/aspnetcore@361e628

You're still using the same SDK without the fix...

image

Do you need to update the SDK in this PR's global.json to get a newer version with the fixed analyzer?

global.json Outdated Show resolved Hide resolved
marcpopMSFT and others added 2 commits October 18, 2022 13:00
Co-authored-by: James Newton-King <james@newtonking.com>
@nagilson
Copy link
Member

Seems this is still looking for net7.0:

    C:\h\w\B756097F\p\ex\msbuildExtensions\Microsoft\Microsoft.NET.Build.Extensions\Microsoft.NET.Build.Extensions.NETFramework.targets(114,5): error MSB4062: The "AddFacadesToReferences" task could not be loaded from the assembly 

C:\h\w\B756097F\p\ex\msbuildExtensions\Microsoft\Microsoft.NET.Build.Extensions\\tools\net7.0\Microsoft.NET.Build.Extensions.Tasks.dll.
 Could not load file or assembly 'C:\h\w\B756097F\p\ex\msbuildExtensions\Microsoft\Microsoft.NET.Build.Extensions\tools\net7.0\Microsoft.NET.Build.Extensions.Tasks.dll'. 
 The system cannot find the path specified. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [C:\h\w\B756097F\t\dotnetSdkTests\r1lisw1i.eln\Anet462Projec---F15591BC\TestApp\TestApp.csproj]

@marcpopMSFT
Copy link
Member Author

@lewing GetPackDefinitionLocations is failing because it sees Microsoft.NETCore.App.Runtime.osx-arm64 and osx.x64 in both the 8 and 7 manifests from what I can tell. CC @dsplaisted
image

@nagilson
Copy link
Member

@dotnet/aspnet-blazor-eng Looks like a lot of the remaining failures (or all) are tests in blazor-wasm. Do these need to be updated to allow targeting 8.0? May you please take a look? cc @marcpopMSFT

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner October 27, 2022 10:35
@marcpopMSFT
Copy link
Member Author

Latest change is getting closer.

@dotnet/aspnet-blazor-eng can you take a look at the blazor failures that all appear to be similar to this:
Expected File C:\h\w\9FDD0948\t\dotnetSdkTests\ep3bpqss.izk\Pack_Multiple---7F5CE8C3\bin\Debug\net8.0\PackageLibraryTransitiveDependency.dll to exist, but it does not.

@dotnet/templating-engine-maintainers there are a few template engine tests also failing. Can you take a look as well?

Fix our implicit defines behavior for the windows platform versions
@marcpopMSFT
Copy link
Member Author

@dotnet/aspnet-blazor-eng can you take a look at the blazor failures that all appear to be similar to this: Expected File C:\h\w\9FDD0948\t\dotnetSdkTests\ep3bpqss.izk\Pack_Multiple---7F5CE8C3\bin\Debug\net8.0\PackageLibraryTransitiveDependency.dll to exist, but it does not.
The tests that are still failing are compiling for net7.0. I don't think the default TFM can be switched though as other tests will fail. Not sure if the set that are failing should be modified now to target net7.0 explicitly.

@@ -28,10 +28,10 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup>
<!-- Paths to tools, tasks, and extensions are calculated relative to the BlazorWebAssemblySdkDirectoryRoot. This can be modified to test a local build. -->
<BlazorWebAssemblySdkDirectoryRoot Condition="'$(BlazorWebAssemblySdkDirectoryRoot)'==''">$(MSBuildThisFileDirectory)..\</BlazorWebAssemblySdkDirectoryRoot>
<_BlazorWebAssemblySdkTasksTFM Condition=" '$(MSBuildRuntimeType)' == 'Core'">net7.0</_BlazorWebAssemblySdkTasksTFM>
<_BlazorWebAssemblySdkTasksTFM Condition=" '$(MSBuildRuntimeType)' == 'Core'">net8.0</_BlazorWebAssemblySdkTasksTFM>
Copy link
Member

Choose a reason for hiding this comment

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

We updating this in many places, is it worth changing to this?

Suggested change
<_BlazorWebAssemblySdkTasksTFM Condition=" '$(MSBuildRuntimeType)' == 'Core'">net8.0</_BlazorWebAssemblySdkTasksTFM>
<_BlazorWebAssemblySdkTasksTFM Condition=" '$(MSBuildRuntimeType)' == 'Core'">$(SdkTargetFramework)</_BlazorWebAssemblySdkTasksTFM>

@dreddy-work
Copy link
Member

@dotnet/aspnet-blazor-eng , can anyone take a look here? both WinForms and WPF are blocked from flowing up.

@vlada-shubina
Copy link
Member

Latest change is getting closer.

@dotnet/aspnet-blazor-eng can you take a look at the blazor failures that all appear to be similar to this: Expected File C:\h\w\9FDD0948\t\dotnetSdkTests\ep3bpqss.izk\Pack_Multiple---7F5CE8C3\bin\Debug\net8.0\PackageLibraryTransitiveDependency.dll to exist, but it does not.

@dotnet/templating-engine-maintainers there are a few template engine tests also failing. Can you take a look as well?

templating tests were fixed in 4da0821

@nagilson
Copy link
Member

@dotnet/aspnet-blazor-eng PTAL, there are at least 10+ PRs blocked by this I can think of now

@javiercn
Copy link
Member

@nagilson we have PRs out to address this across all the branches, but the one on main is blocked on some other dotnet-watch tests failing. See:
#29093

These other PRs are just waiting on confirmation that we can merge on the target branches
#29118
#29119
#29120

* Updates the way the baselines are compared to generate
  a template of the baselines from the current manifest
  and compare it against the existing template instead of
  applying the current versions to the baseline and comparing
  it against the generated manifest.
* Removes all the versions and hashes from all the files Wed
  generate.
* Splits the code for generating and comparing the baselines
  into their own classes for better maintenance.
* Updates the baselines to reflect the new format.
@marcpopMSFT
Copy link
Member Author

@tmat I believe Noah said that you were already looking into dotnet watch failures. Looks like there are four failures now here in those tests.

@dreddy-work
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kasperk81
Copy link
Contributor

framework error There is not enough space on the disk. is unrelated.

merge?

@nagilson
Copy link
Member

Merge. 😏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.