Use live dependencies for netcoreapp3.0 depencies (#48176) #48344

ViktorHofer · 2021-02-16T16:33:17Z

Backport of d77092d.
Fixes #45560

Users are hitting an issue with needing to add an explicit reference to System.Text.Encodings.Web when they have System.Text.Json 5.0.0 in a netcoreapp3.1 project.

This is because the build of System.Text.Json for netcoreapp3.0 doesn't have a reference to the "live" built System.Text.Encoding.Web, instead it references the version from the shared framework. This creates a compatibility problem because the netstandard2.0 version references a higher version of the dependency than is inbox on netcoreapp3.0

Reported in dotnet/aspnetcore#28354 and dotnet/aspnetcore#28632. Documented in dotnet/AspNetCore.Docs#21054.

Customer Impact

Reported by customers who want to use the System.Text.Json 5.0.x package in a netcoreapp3.1 project and need to add an additional PackageReference to System.Text.Encodings.Web.

Regression?

Regressed between preview8 and rc1 of the 5.0.0 package.

Testing

Packages created locally and installed in a netcoreapp3.1 project. Transitive dependencies are coming along. NuSpec lists the dependencies.

Risk

Low/Medium. NuSpec and assembly metadata change.

ghost · 2021-02-16T16:33:21Z

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of d77092d.
Fixes #45560

Users are hitting an issue with needing to add an explicit reference to System.Text.Encodings.Web when they have System.Text.Json 5.0.0 in a netcoreapp3.1 project.

This is because the build of System.Text.Json for netcoreapp3.0 doesn't have a reference to the "live" built System.Text.Encoding.Web, instead it references the version from the shared framework. This creates a compatibility problem because the netstandard2.0 version references a higher version of the dependency than is inbox on netcoreapp3.0

Reported in dotnet/aspnetcore#28354 and dotnet/aspnetcore#28632. Documented in dotnet/AspNetCore.Docs#21054.

Customer Impact

Reported by customers who want to use the System.Text.Json 5.0.x package in a netcoreapp3.1 project and need to add an additional PackageReference to System.Text.Encodings.Web.

Regression?

Regressed between preview8 and rc1 of the 5.0.0 package.

Testing

Packages created locally and installed in a netcoreapp3.1 project. Transitive dependencies are coming along. NuSpec lists the dependencies.

Risk

Low/Medium. NuSpec and assembly metadata change.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`Servicing-consider`, `area-Infrastructure-libraries`
Milestone:	-

ericstj · 2021-02-16T18:43:42Z

Looks like the build breaks are due to Unsafe.As returning a nullable type:

runtime/src/libraries/System.Runtime.CompilerServices.Unsafe/ref/System.Runtime.CompilerServices.Unsafe.cs

Line 22 in 4823e7c

public static T? As<T>(object? o) where T : class? { throw null; }

Odd that we don't hit this in master. Any idea why?

ViktorHofer · 2021-02-16T22:30:02Z

Odd that we don't hit this in master. Any idea why?

No idea and absolutely confused about this. Unsafe's reference assembly uses nullable annotation for all configurations, so it can't be a tfm mismatch case between P2P and simple name reference.

ericstj · 2021-02-17T01:40:23Z

Do we end up using the reference assembly in the same failing configuration, or is something else winning giving the compiler a different asset?

ViktorHofer · 2021-02-18T18:18:45Z

Failure is #48469.

ViktorHofer · 2021-03-01T17:08:21Z

@Anipik is the 5.0 branch already open?

Anipik · 2021-03-01T17:28:05Z

@Anipik is the 5.0 branch already open?

not yet, i will take care of merging this when branch is open

ViktorHofer · 2021-03-01T17:41:38Z

cool, thanks a lot. I think the package version and packageindex changes are also necessary before merging.

Anipik · 2021-03-02T18:07:42Z

cool, thanks a lot. I think the package version and packageindex changes are also necessary before merging.

correct

Anipik · 2021-03-11T22:25:46Z

@ViktorHofer any update on this one ?

ViktorHofer · 2021-03-11T22:31:46Z

Yes on it right now

ViktorHofer · 2021-03-11T23:13:18Z

src/libraries/System.Text.Json/Directory.Build.props

-    <PackageVersion>5.0.1</PackageVersion>
-    <AssemblyVersion Condition="$(TargetFramework.StartsWith('net4'))">5.0.0.1</AssemblyVersion>
+    <PackageVersion>5.0.2</PackageVersion>
+    <AssemblyVersion Condition="$(TargetFramework.StartsWith('net4'))">5.0.0.2</AssemblyVersion>
    <SkipValidatePackage>true</SkipValidatePackage>


@Anipik is that setting intentional?

Found out that this is necessary when incrementing the assembly version for only a subset of supported TargetFrameworks. Unfortunately the servicing docs don't mention that.

ViktorHofer · 2021-03-11T23:13:33Z

@Anipik the change is ready, please take a look.

src/libraries/System.Reflection.MetadataLoadContext/Directory.Build.props

src/libraries/pkg/baseline/packageIndex.json

ericstj · 2021-03-12T17:03:37Z

src/libraries/pkg/baseline/packageIndex.json

@@ -928,11 +928,13 @@
        "3.1.0",
        "3.1.1",
        "3.1.2",
-        "5.0.0"
+        "5.0.0",
+        "5.0.1"


Should you include baselineVersion here?

ericstj

If Microsoft.Extensions.Logging.EventSource is referencing the latest Primitives on all frameworks and build succeeds, then this is safe to merge.

We should follow up by making sure all packages we build are baselined, otherwise our selective version bumps in servicing can result in some packages shipping references to GA packages even when the dependency is serviced. I suspect the Extensions packages never added baselines since they were added in 5.0 and used P2P but now that we're selectively versioning it matters.

ViktorHofer · 2021-03-12T18:39:42Z

/azp run runtime

azure-pipelines · 2021-03-12T18:50:37Z

Comment was made before the most recent commit for PR 48344 in repo dotnet/runtime

ericstj

This looks like it addressed my concern. Thanks!

ghost · 2021-03-12T18:58:41Z

Hello @ericstj!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

safern · 2021-03-12T19:39:07Z

Do we end up using the reference assembly in the same failing configuration, or is something else winning giving the compiler a different asset?

Could it be that in main we have an issue that for this case it is referencing the src project output? Unsafe is written in IL and only the ref assembly is nullable annotated.

Anipik · 2021-03-12T20:08:28Z

cc @ViktorHofer @ericstj

C:\h\w\A0D708EB\p\packageTest.targets(82,5): error : Assembly 'System.Text.Json' has insufficient version for dependency 'System.Text.Encodings.Web' : 5.0.0.0 < 5.0.0.1. [C:\h\w\A0D708EB\w\BFB00A58\u\net5.0\project.csproj]
C:\h\w\A0D708EB\p\packageTest.targets(82,5): error : Assembly 'System.Text.Json' has insufficient version for dependency 'System.Text.Encodings.Web' : 5.0.0.0 < 5.0.0.1. [C:\h\w\A0D708EB\w\BFB00A58\u\net5.0\project.csproj]
  Testing System.Text.Json TFM=net5.0 RID=ubuntu.16.04-x64

ericstj · 2021-03-12T21:26:49Z

So this appears to be happening because f27d337 updated the assembly version of S.T.E.W, and claims the updated version is part of .NET5.0 (which is not the case for GA). As a result we don't emit a PackageReference on .NET5.0 and the version referenced is unsatisfied.

ericstj · 2021-03-12T21:31:03Z

We can't really undo this assembly-version bump now since it's been exposed. So I suggest we just correct the index. This will cause S.T.J to include the reference to S.T.E.W even on .NET5.0.

ericstj · 2021-03-12T22:22:40Z

@Anipik -- please take note. That version change has made the ref-pack unserviceable. We would need to undo it for the net5.0 if we wanted to ship the ref-pack again from 5.0.

ViktorHofer added Servicing-consider Issue for next servicing release review area-Infrastructure-libraries labels Feb 16, 2021

ViktorHofer requested review from Anipik and safern February 16, 2021 16:33

ViktorHofer requested a review from ahsonkhan as a code owner February 16, 2021 16:33

ViktorHofer self-assigned this Feb 16, 2021

ViktorHofer requested review from jozkee, layomia and steveharter as code owners February 16, 2021 16:33

ViktorHofer requested review from ericstj and removed request for ahsonkhan, steveharter, layomia and jozkee February 16, 2021 16:54

leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 18, 2021

leecow added this to the 5.0.5 milestone Feb 18, 2021

ViktorHofer force-pushed the NetCoreapp30DepsFixRel50 branch from c30edc8 to d50e45d Compare March 11, 2021 23:08

ViktorHofer commented Mar 11, 2021

View reviewed changes

Disable package validation for two packages

1fe00e6

ericstj reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Reflection.MetadataLoadContext/Directory.Build.props Show resolved Hide resolved

ericstj reviewed Mar 12, 2021

View reviewed changes

src/libraries/pkg/baseline/packageIndex.json Show resolved Hide resolved

ericstj reviewed Mar 12, 2021

View reviewed changes

ericstj approved these changes Mar 12, 2021

View reviewed changes

ViktorHofer added 2 commits March 12, 2021 19:03

Add baselines for touched packages

ee08cf8

Build packages on allconfigurations leg only

006c446

ViktorHofer closed this Mar 12, 2021

ViktorHofer reopened this Mar 12, 2021

ericstj approved these changes Mar 12, 2021

View reviewed changes

ericstj added the auto-merge label Mar 12, 2021

Anipik approved these changes Mar 12, 2021

View reviewed changes

Fix inbox version of System.Text.Encodings.Web

cbfeb62

This was referenced Mar 12, 2021

Mono SIGABRT in System.Drawing.Common affecting some test runs #37838

Closed

HTTP2 test intermittently failing CI #43405

Closed

ViktorHofer merged commit a55e6c6 into dotnet:release/5.0 Mar 13, 2021

ViktorHofer deleted the NetCoreapp30DepsFixRel50 branch March 13, 2021 12:45

runfoapp bot mentioned this pull request Mar 22, 2021

Test failure: System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Http2.Http2_MultipleConnectionsEnabled_InfiniteRequestsCompletelyBlockOneConnection_RemaningRequestsAreHandledByNewConnection #45204

Closed

ghost locked as resolved and limited conversation to collaborators Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use live dependencies for netcoreapp3.0 depencies (#48176) #48344

Use live dependencies for netcoreapp3.0 depencies (#48176) #48344

ViktorHofer commented Feb 16, 2021

ghost commented Feb 16, 2021

Customer Impact

Regression?

Testing

Risk

ericstj commented Feb 16, 2021 •

edited

Loading

ViktorHofer commented Feb 16, 2021

ericstj commented Feb 17, 2021

ViktorHofer commented Feb 18, 2021

ViktorHofer commented Mar 1, 2021

Anipik commented Mar 1, 2021

ViktorHofer commented Mar 1, 2021

Anipik commented Mar 2, 2021

Anipik commented Mar 11, 2021

ViktorHofer commented Mar 11, 2021

ViktorHofer Mar 11, 2021

ViktorHofer Mar 12, 2021

ViktorHofer commented Mar 11, 2021

ericstj Mar 12, 2021

ericstj left a comment

ViktorHofer commented Mar 12, 2021

azure-pipelines bot commented Mar 12, 2021

ericstj left a comment

ghost commented Mar 12, 2021

safern commented Mar 12, 2021

Anipik commented Mar 12, 2021

ericstj commented Mar 12, 2021 •

edited

Loading

ericstj commented Mar 12, 2021 •

edited

Loading

ericstj commented Mar 12, 2021

Use live dependencies for netcoreapp3.0 depencies (#48176) #48344

Use live dependencies for netcoreapp3.0 depencies (#48176) #48344

Conversation

ViktorHofer commented Feb 16, 2021

Customer Impact

Regression?

Testing

Risk

ghost commented Feb 16, 2021

Customer Impact

Regression?

Testing

Risk

ericstj commented Feb 16, 2021 • edited Loading

ViktorHofer commented Feb 16, 2021

ericstj commented Feb 17, 2021

ViktorHofer commented Feb 18, 2021

ViktorHofer commented Mar 1, 2021

Anipik commented Mar 1, 2021

ViktorHofer commented Mar 1, 2021

Anipik commented Mar 2, 2021

Anipik commented Mar 11, 2021

ViktorHofer commented Mar 11, 2021

ViktorHofer Mar 11, 2021

Choose a reason for hiding this comment

ViktorHofer Mar 12, 2021

Choose a reason for hiding this comment

ViktorHofer commented Mar 11, 2021

ericstj Mar 12, 2021

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

ViktorHofer commented Mar 12, 2021

azure-pipelines bot commented Mar 12, 2021

ericstj left a comment

Choose a reason for hiding this comment

ghost commented Mar 12, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

safern commented Mar 12, 2021

Anipik commented Mar 12, 2021

ericstj commented Mar 12, 2021 • edited Loading

ericstj commented Mar 12, 2021 • edited Loading

ericstj commented Mar 12, 2021

ericstj commented Feb 16, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ericstj commented Mar 12, 2021 •

edited

Loading

ericstj commented Mar 12, 2021 •

edited

Loading