Skip to content

Update to newer SDK #14168

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.

Sign up for GitHub

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

Jump to bottom
Closed
wtgodbe wants to merge 9 commits into from
Closed

Update to newer SDK #14168

wtgodbe wants to merge 9 commits into from

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Sep 19, 2019

Part of https://github.com/aspnet/AspNetCore-Internal/issues/3016

Update to a newer 5.0 SDK which includes the netcoreapp5.0 TFM.

CC @dougbu @JunTaoLuo

@wtgodbe wtgodbe requested a review from a team September 19, 2019 20:13
@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Sep 19, 2019

Need to add NoWarn for NU5131:

    <!-- Working around https://github.com/NuGet/Home/issues/8467 -->
    <NoWarn>$(NoWarn);NU5131</NoWarn>

and NU5048: https://github.com/aspnet/AspNetCore-Internal/issues/3103. This will probably affect several SDK update PRs.

dougbu
dougbu suggested changes Sep 19, 2019
View reviewed changes
@dougbu dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 19, 2019
@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 20, 2019

CI is green - @dougbu @JunTaoLuo does this look good to go? Should I add back the NetCoreAppMaximumVersion & KnownFrameworkReference workarounds?

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 20, 2019

Added the workaround for vNext SDK

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 20, 2019

eng/Workarounds.targets(37,7): error MSB4113: Specified condition "!(@(KnownAppHostPack->AnyHaveMetadataValue('TargetFramework', '$(DefaultNetCoreTargetFramework)')))" evaluates to "!" instead of a boolean.

Not sure why this isn't working here, the exact same works fine in dotnet/ef6#1276 & dotnet/efcore#17945

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 20, 2019

Trying 7e5b057, which short-circuits when KnownFrameworkReference/KnownAppHostPack are empty/don't exist. I'm still not sure why I didn't hit this in the EF repos

dougbu
dougbu approved these changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Change looks straightforward. Not sure why builds are still failing but know you'll figure it out 😄

eng/Workarounds.targets
<KnownAppHostPack
Include="@(KnownAppHostPack->WithMetadataValue('TargetFramework', 'netcoreapp3.0'))"
TargetFramework="$(DefaultNetCoreTargetFramework)"
Condition="(@(KnownAppHostPack->Count()) != '0') AND (!(@(KnownAppHostPack->AnyHaveMetadataValue('TargetFramework', '$(DefaultNetCoreTargetFramework)'))))"
Copy link
Contributor

@dougbu dougbu Sep 20, 2019

Choose a reason for hiding this comment

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

Why the extra layer of parentheses around both of the AND'ed clauses?

Copy link
Member Author

@wtgodbe wtgodbe Sep 20, 2019

Choose a reason for hiding this comment

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

Paranoia 😆

@wtgodbe wtgodbe mentioned this pull request Sep 20, 2019
Merged
@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 20, 2019

Closing in favor of #14206

@wtgodbe wtgodbe closed this Sep 20, 2019
@wtgodbe wtgodbe deleted the UpdateSDK branch January 5, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

@dougbu dougbu dougbu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wtgodbe @JunTaoLuo @dougbu