Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/2.1] Add SuppressMetaPackage to break circular dependency during System.Memory package restore #36550

Merged
merged 7 commits into from
Apr 12, 2019

Conversation

ahsonkhan
Copy link
Member

Description

System.Memory depends on the NetStandard.Library meta-package, particularly on netstandard1.1 (see https://www.nuget.org/packages/System.Memory/4.5.2).
image

Whenever libraries that make up the meta-package try to reference/depend on System.Memory, it ends up creating a circular dependency which causes package restore to fail. This change makes it such that System.Memory does not reference NETStandard.Library (meta-package) but instead references the constituent System.* packages.

Customer Impact

This issue surfaced recently when System.Diagnostics.DiagnosticSource was trying to reference System.Memory to add new span-based APIs (and the NetStandard.Library meta-package includes this package in its closure - via System.Net.Http) . This results in applications consuming such library to fail on package restore (see #33207):

PackageCycle.csproj : error NU1108: Cycle detected.
PackageCycle.csproj : error NU1108: PackageCycle -> System.Diagnostics.DiagnosticSource 4.5.1 -> System.Memory 4.5.1 -> NETStandard.Library 1.6.1 -> System.Net.Http 4.3.0 -> System.Diagnostics.DiagnosticSource (>= 4.3.0).

System.Diagnostics.DiagnosticSource has customers (such as App Insights) who depend on this new functionality (and need to run on netstandard1.x/2.0 compatible frameworks). The pkgproj contains a temporary workaround as a stop gap:

<!-- This target is a work-around and should be removed by 6/2019 (by then a fixed version of System.Memory
should be available so that System.Memory does not reference NetStandard1.X)
See Eric St John for details. -->
<Target Name="RemoveSystemMemoryNetStandard1x" AfterTargets="GetNuGetPackageDependencies">
<!-- System.Memory is causing a cycle when used for netstandard1.x.
This is due to S.M referencing the NETStandard.Library metapackage for netstandard1.x
and that meta-package includes this package (System.Diagnostics.DiagnosticSource) in
its closure.
Remove the dependency for now until we can get a new build of System.Memory that doesn't
reference the NETStandard.Library metapackage. -->
<ItemGroup>
<Dependency Remove="@(Dependency)" Condition="'%(Identity)' == 'System.Memory' AND $([System.String]::new('%(TargetFramework)').StartsWith('netstandard1.'))" />
</ItemGroup>
</Target>

Regression?

No

Packaging reviewed?

  • The fix was tested by consuming System.Diagnostics.DiagnosticSource (that references the fixed System.Memory package) from a console app targeting netcoreapp1.1. The package restore was successful and we no longer have the cycle.
  • Also, the change is correctly reflected in the generated nusepc:
    image

cc @ericstj, @wtgodbe, @safern

Risk

Low. We have made similar fixes before to break the circular dependency (for example for System.Runtime.CompilerServices.Unsafe - #28042).

@ahsonkhan ahsonkhan added this to the 2.1.x milestone Apr 2, 2019
@@ -2,7 +2,7 @@
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="..\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.0.1.0</AssemblyVersion>
<AssemblyVersion>4.0.1.1</AssemblyVersion>
<PackageVersion>4.5.1</PackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why there is a PackageVersion property both here, and in the .pkgproj?

Copy link
Member Author

@ahsonkhan ahsonkhan Apr 3, 2019

Choose a reason for hiding this comment

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

I saw that, but decided to leave it since the pkgproj overrides it. I am not sure why we have it in both places. It might have been an oversight when it was added to dir.props in #29925. I don't think it's worth removing, but if you disagree, should we remove it from the pkgproj and just keep the one in dir.props (or vice versa)?

That said, we aren't consistent in other places:


<PackageVersion>4.5.1</PackageVersion>

cc @tarekgh

See related thread where it was added to the pkgproj:
#29970 (comment)

@danmoseley danmoseley requested a review from ericstj April 3, 2019 00:16
@ahsonkhan
Copy link
Member Author

@ericstj, can you please help with the packaging failure in CI related to harvesting?

https://ci.dot.net/job/dotnet_corefx/job/pipelines/job/release_2.1/job/windows-TGroup_all+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/2/consoleFull#-129723753933fe3402-0c0f-45a7-a707-47164965bab5

D:\j\workspace\windows-TGrou---d2965379\Tools\Packaging.targets(435,5): error : Cannot harvest package 'System.Memory' version '4.5.2' because D:\j\workspace\windows-TGrou---d2965379\packages/System.Memory\4.5.2 does not exist.  Harvesting is needed to redistribute assets and ensure compatiblity with the previous release.  You can disable this by setting HarvestStablePackage=false. [D:\j\workspace\windows-TGrou---d2965379\src\System.Memory\pkg\System.Memory.pkgproj]

Where is the version 4.5.2 coming from? Do I need to add a System.Memory entry to https://github.com/dotnet/corefx/blob/release/2.1/external/harvestPackages/harvestPackages.props or opt-out of harvesting by setting HarvestStablePackage to false in the pkgproj?

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

Version comes from packageIndex. You need to run this target locally and include the change with your PR:

<Target Name="UpdateToLatestStablePackages">

@wtgodbe I thought we were going to automate this?

@ahsonkhan
Copy link
Member Author

You need to run this target locally and include the change with your PR

I tried running it locally. It didn't update the props file.

  1. cd to \corefx\external\harvestPackages\
  2. Run msbuild harvestPackages.depproj /t:UpdateToLatestStablePackages

Am I missing a step or doing something wrong?

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

Looks like that target uses the items already there in order to update. You can add an item then it will be updated. I suppose that target could get the list of package ids from pkgproj s under arc rather than using existing items. Mind fixing it?

@ahsonkhan
Copy link
Member Author

ahsonkhan commented Apr 3, 2019

Looks like that target uses the items already there in order to update. You can add an item then it will be updated.

That worked. I added the item manually and it updated to the latest version (i.e. 4.5.3). But that results in build failure since that version isn't published anywhere. Shouldn't it keep it at 4.5.2?

E:\GitHub\Fork\corefx\external\harvestPackages\harvestPackages.depproj : error NU1103: Unable to find a stable package System.Memory with version (>= 4.5.3)
...
E:\GitHub\Fork\corefx\external\harvestPackages\harvestPackages.depproj : error NU1103: - Found 9 version(s) in nuget.org [ Nearest version: 4.5.2 ]
...
E:\GitHub\Fork\corefx\Tools\packageresolve.targets(48,5): error MSB3073: The command ""E:\GitHub\Fork\corefx\Tools/dotnetcli/dotnet.exe" restore --packages "E:\GitHub\Fork\corefx\packages" --source <various feeds>
 E:\GitHub\Fork\corefx\external\harvestPackages\harvestPackages.depproj /p:TargetGroup=package /p:ConfigurationGroup=Debug /p:ArchGroup=x64 /p:OSGroup=AnyOS /p:TargetFramework=  " exited with code 1. [E:\GitHub\Fork\corefx
\external\harvestPackages\harvestPackages.depproj]

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

You cannot reference a version you haven’t built yet.

@@ -2491,14 +2491,16 @@
"StableVersions": [
"4.5.0",
"4.5.1",
"4.5.2"
"4.5.2",
"4.5.3"
Copy link
Member

Choose a reason for hiding this comment

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

Has 4.5.3 been built stable yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I see. Maybe that document should explicitly call out that StableVersion should not be changed until after you've shipped the stable release. StableVersion list represents history.

Copy link
Member

Choose a reason for hiding this comment

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

@ahsonkhan this change still needs to be removed before I can merge this

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

@wtgodbe
Copy link
Member

wtgodbe commented Apr 3, 2019

@wtgodbe I thought we were going to automate this?

I haven't done this yet, looks like I forgot to create an issue. https://github.com/dotnet/corefx/issues/36570

@ahsonkhan
Copy link
Member Author

Looks like we are hitting https://github.com/dotnet/corefx/issues/28551 for .NET Core 1.0
https://ci.dot.net/job/dotnet_corefx/job/pipelines/job/release_2.1/job/windows-TGroup_all+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/3/consoleFull#-1120818049a82fefab-f698-416f-8fca-58544c94cd4e

D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: Detected package downgrade: System.IO.FileSystem from 4.3.0 to 4.0.1. Reference the package directly from the project to select a different version. [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.unix.System.Net.Sockets 4.3.0 -> System.IO.FileSystem (>= 4.3.0) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.IO.FileSystem (>= 4.0.1) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: Detected package downgrade: System.Net.NameResolution from 4.3.0 to 4.0.0. Reference the package directly from the project to select a different version. [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.unix.System.Net.Sockets 4.3.0 -> System.Net.NameResolution (>= 4.3.0) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> System.Net.NameResolution (>= 4.0.0) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: Detected package downgrade: System.Net.Primitives from 4.3.0 to 4.0.11. Reference the package directly from the project to select a different version. [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.unix.System.Net.Sockets 4.3.0 -> System.Net.Primitives (>= 4.3.0) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.Net.Primitives (>= 4.0.11) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: Detected package downgrade: System.Threading.ThreadPool from 4.3.0 to 4.0.10. Reference the package directly from the project to select a different version. [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> NETStandard.Library 1.6.0 -> System.Net.Sockets 4.1.0 -> runtime.unix.System.Net.Sockets 4.3.0 -> System.Threading.ThreadPool (>= 4.3.0) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]
20:22:31 D:\j\workspace\windows-TGrou---d2965379\bin\testPkg\projects\System.Memory\netcoreapp1.0\project.csproj : error NU1605: project -> Microsoft.NETCore.App 1.0.11 -> System.Threading.ThreadPool (>= 4.0.10) [D:\j\workspace\windows-TGrou---d2965379\pkg\test\testPackages.proj]

Do we have a workaround or fix?

  • Skip the warning?
  • Explicitly reference the packages causing the downgrade with the new version?

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

Do this: 5f26635

@ahsonkhan
Copy link
Member Author

@dotnet-bot test NETFX x86 Release Build

@ahsonkhan
Copy link
Member Author

@dotnet-bot test Windows x86 Release Build

@danmoseley
Copy link
Member

Test failures are only because Fedora 27 queue was removed and they can be ignored.

@danmoseley danmoseley added Servicing-consider Issue for next servicing release review * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Apr 4, 2019
@vivmishra vivmishra added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 4, 2019
@vivmishra vivmishra modified the milestones: 2.1.x, 2.1.11 Apr 4, 2019
@vivmishra
Copy link

Approved for 2.1.11 and 2.2.5.
Wait for branch to open.

  • ASP.NET will take the package for both 2.1 and 2.2.

@ahsonkhan
Copy link
Member Author

/mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : error : Project is targeting runtime 'linux-musl-x64' but did not resolve any runtime-specific packages. This runtime may not be supported by the target framework.
12:22:12 
12:22:12 Build FAILED.
12:22:12 
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.ILAsm (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.ILAsm 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.ILAsm 2.2.0-preview1-26417-05 was resolved.
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.ILDAsm (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.ILDAsm 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.ILDAsm 2.2.0-preview1-26417-05 was resolved.
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.Runtime.CoreCLR (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.Runtime.CoreCLR 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.Runtime.CoreCLR 2.2.0-preview1-26417-05 was resolved.
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.ILAsm (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.ILAsm 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.ILAsm 2.2.0-preview1-26417-05 was resolved.
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.ILDAsm (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.ILDAsm 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.ILDAsm 2.2.0-preview1-26417-05 was resolved.
12:22:12 /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : warning NU1603: ilasm depends on Microsoft.NETCore.Runtime.CoreCLR (>= 2.1.10-servicing-27514-02) but Microsoft.NETCore.Runtime.CoreCLR 2.1.10-servicing-27514-02 was not found. An approximate best match of Microsoft.NETCore.Runtime.CoreCLR 2.2.0-preview1-26417-05 was resolved.
12:22:12 
/mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/ilasm/ilasm.depproj : error : Project is targeting runtime 'linux-musl-x64' but did not resolve any runtime-specific packages. This runtime may not be supported by the target framework.

Is this a known failure?

@ahsonkhan
Copy link
Member Author

Since the branch is now open, removing the no merge label.

@ahsonkhan ahsonkhan removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 10, 2019
@ahsonkhan
Copy link
Member Author

The test failure in CI for linux is a known issue: https://github.com/dotnet/corefx/issues/34675

Centos.7.Amd64.Open-x64-Release
System.IO.Tests.FileInfo_GetSetTimes/CopyToMillisecondPresent
https://mc.dot.net/#/user/ahsonkhan/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Frelease~2F2.1~2F/test~2Ffunctional~2Fcli~2F/70a151f6e44a927b76967da73dc0da19c9fbf646/workItem/System.IO.FileSystem.Tests/analysis/xunit/System.IO.Tests.FileInfo_GetSetTimes~2FCopyToMillisecondPresent

Message :
Assert.NotEqual() Failure
Expected: Not 0
Actual:   0
Stack Trace :
   at System.IO.Tests.FileInfo_GetSetTimes.CopyToMillisecondPresent() in /mnt/j/workspace/dotnet_corefx/pipelines/release_2.1/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.IO.FileSystem/tests/FileInfo/GetSetTimes.cs:line 92

@wtgodbe wtgodbe merged commit 723b1ba into dotnet:release/2.1 Apr 12, 2019
@ahsonkhan ahsonkhan deleted the S.Memory.Packaging branch April 15, 2019 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants