Skip to content

Coherency fix#24973

Merged
ajcvickers merged 2 commits into
dotnet:release/6.0-preview5from
lukas-lansky:coherency-fix
May 25, 2021
Merged

Coherency fix#24973
ajcvickers merged 2 commits into
dotnet:release/6.0-preview5from
lukas-lansky:coherency-fix

Conversation

@lukas-lansky

Copy link
Copy Markdown
Contributor

System.ComponentModel.Annotations is no longer produced by dotnet/runtime (dotnet/runtime#51891)

@lukas-lansky
lukas-lansky requested a review from dougbu as a code owner May 25, 2021 12:02
@lukas-lansky
lukas-lansky requested review from a team and mmitche May 25, 2021 12:02
@lukas-lansky

Copy link
Copy Markdown
Contributor Author

Hm, maybe we should go the ASP.NET way here (dotnet/aspnetcore#32484) and fix the version to 5.0.0.

@lukas-lansky

Copy link
Copy Markdown
Contributor Author

It works with the removal too though. That wasn't the case for ASP.NET. Hm. What do you think about the current state of the PR @dotnet/efteam, @ViktorHofer, @dougbu?

@ajcvickers

Copy link
Copy Markdown
Contributor

We need to make sure that we still get the 6.0.0 latest reference. @dougbu Can you advise here?

@mmitche

mmitche commented May 25, 2021

Copy link
Copy Markdown
Member

We need to make sure that we still get the 6.0.0 latest reference. @dougbu Can you advise here?

@lukas-lansky @ajcvickers Based on the PR, it looks like the package was dead-ended and won't have a 6.0 version.

@ajcvickers

Copy link
Copy Markdown
Contributor

@mmitche Then how do we pick up the latest code? I'm pretty sure that changes have gone in since we shipped 5.0.

@ajcvickers

Copy link
Copy Markdown
Contributor

/cc @ericstj

@ViktorHofer

Copy link
Copy Markdown
Member

It works with the removal too though. That wasn't the case for ASP.NET. Hm. What do you think about the current state of the PR @dotnet/efteam, @ViktorHofer, @dougbu?

We need to make sure that we still get the 6.0.0 latest reference. @dougbu Can you advise here?

The library is exposed as part of the shared framework and is coming in by default when targeting a .NETCoreApp configuration. If the removal of the PackageReference works then this means that you don't need the library on any other configuration like .NETFramework or .NETStandard. Based on that the change looks good to me 👍

@ajcvickers

Copy link
Copy Markdown
Contributor

It wasn't originally part of the shared framework? When did it start being pulled in? If it is in the SDK, then we're fine.

@ajcvickers

Copy link
Copy Markdown
Contributor

But also, if it's in the SDK, then we definitely should not be targeting the 5.0.0 version.

@ViktorHofer

Copy link
Copy Markdown
Member

It wasn't originally part of the shared framework? When did it start being pulled in? If it is in the SDK, then we're fine.

System.ComponentModel.Annotations has been part of the .NET Core shared framework since 2.0 which is when the shared framework started to exist: https://github.com/dotnet/corefx/blob/d6173e069a9bcedfdfd7f4f41e67d23f67157b61/src/System.ComponentModel.Annotations/dir.props#L8

But also, if it's in the SDK, then we definitely should not be targeting the 5.0.0 version.

Exactly, you should be fine with just removing the PackageReference. You would see a build error if there's another TFM that requires the package.

@mmitche

mmitche commented May 25, 2021

Copy link
Copy Markdown
Member

But also, if it's in the SDK, then we definitely should not be targeting the 5.0.0 version.

Then I believe that it would come in implicitly via your dependency on the latest shared framework.

@mmitche

mmitche commented May 25, 2021

Copy link
Copy Markdown
Member

Exactly, you should be fine with just removing the PackageReference. You would see a build error if there's another TFM that requires the package.

Beat me to it!

@ajcvickers

Copy link
Copy Markdown
Contributor

That's cool; didn't realize that change was made in 2.0. Had we known, then presumably we would have not referenced this as a package even before now.

@dougbu dougbu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works for me and the CI checks all passed. Deferring to @ajcvickers for zee merge.

@ajcvickers
ajcvickers merged commit ee8d9b7 into dotnet:release/6.0-preview5 May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants