-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Update to newer SDK #14206
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 to newer SDK #14206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an additional small fix: Add a _._
file or two to ensure the package's files and .nuspec align. @JunTaoLuo did this recently though I'm not sure where to hunt for that fix. @JunTaoLuo❔
Something like https://github.com/aspnet/AspNetCore/blob/master/src/Analyzers/shared/FeatureDetection/Microsoft.AspNetCore.Analyzers.FeatureDetection.Sources.csproj#L25-L28? Note that I added a placeholder file: lib{TFM}_._ |
@dougbu @JunTaoLuo how can I tell where the placeholder file(s) would be needed? All builds were successful here, the only failures were in flaky tests (plus the template tests timed out) |
Hmm come to think of it, the fixes I made in 3.1 should already be merged to master so there shouldn't be a need to make them here |
@JunTaoLuo @dougbu I think we've seen this one before in the template tests - do either of you remember what the workaround was? CC also @rynowak @pranavkm @BrennanConroy |
That means the workaround like
was probably applied twice somehow. Essentially there's a duplicate item in KnownAppHostPack or KnownFrameworkReference itemgroup. |
From the binlog, looks like we get a duplicate entry for Specifically, the ItemGroup gets created (with the duplicate) here: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L160-L166. The Looks like the original duplicate comes from |
Looks like I need to apply the workaround from |
Looks like the same thing is happening here - dotnet/razor#1173 (comment). CC @NTaylorMullen |
All build failures are the MSBuild thing. We're down to 25 test template failures:
|
Looks like the failing template tests aren't restoring from the local build like they should be. Will dig further after tactics |
All failures are the MSBuild 16.3 thing. Seems to be happening much more consistently now - I'm optimistic that this means machines are offline getting an update rolled out 🤞 |
This should be good to go now, @JunTaoLuo @dougbu any concerns before I merge? |
@@ -16,22 +16,12 @@ | |||
|
|||
<KnownFrameworkReference | |||
Update="Microsoft.AspNetCore.App" | |||
TargetFramework="${DefaultNetCoreTargetFramework}" | |||
DefaultRuntimeFrameworkVersion="${MicrosoftAspNetCoreAppRuntimePackageVersion}" | |||
LatestRuntimeFrameworkVersion="${MicrosoftAspNetCoreAppRuntimePackageVersion}" | |||
TargetingPackVersion="${MicrosoftAspNetCoreAppRefPackageVersion}" | |||
RuntimePackRuntimeIdentifiers="${SupportedRuntimeIdentifiers}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this look more like the @(KnownFrameworkReference)
changes done in eng/Workarounds.targets and related files? Main issue is this updates the Microsoft.AspNetCore.App reference for all TFMs to use the latest ref and runtime packages. While this might be fine now, it could cause problems when "new" TFM isn't recognized in the SDK yet or if we have any tests using downlevel TFMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes, but that hits the issue from #14670 (comment), where msbuild complains about conditions on item metadata here. Will open an issue on this after I merge, since it will take some investigation to figure out why that's happening only here.
No concerns other than my question. At least file an issue to revisit the *.in file used in template tests. |
Replaces #14168 because I was having some troubles with git...
Part of aspnet/AspNetCore-Internal#3016
Update to a newer 5.0 SDK which includes the netcoreapp5.0 TFM.
CC @dougbu @JunTaoLuo