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

Add SuppressMetaPackage to break circular dependency during package restore #28042

Merged
merged 1 commit into from
Mar 15, 2018
Merged

Add SuppressMetaPackage to break circular dependency during package restore #28042

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...System.Runtime.CompilerServices.Unsafe/pkg/System.Runtime.CompilerServices.Unsafe.pkgproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
<SupportedFramework>net45;netcore45;wp8;wpa81;netcoreapp1.0;$(AllXamarinFrameworks)</SupportedFramework>
</ProjectReference>
<ProjectReference Include="..\src\System.Runtime.CompilerServices.Unsafe.ilproj" />

<!-- this package is part of the implementation closure of NETStandard.Library
therefore it cannot reference NETStandard.Library -->
<SuppressMetaPackage Include="NETStandard.Library" />
Copy link
Member

Choose a reason for hiding this comment

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

Are there other packages this should be added to, as well?

Copy link
Member Author

@ahsonkhan ahsonkhan Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From my discussion with @ericstj yesterday, this was the only package that did not have it and needed it.

Other packages, like System.Buffers, System.Diagnostics.DiagnosticSource, etc. all have it already (https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=SuppressMetaPackage&type=)

The remaining packages (for example those under Reflection.*) no longer publish newer versions.

A potential candidate for this could be System.Memory but so far no packages that are OOB (or partially OOB) reference System.Memory.

If, for instance, System.Threading.Tasks.Extensions were to reference System.Memory, we would probably need to add the property to System.Memory as well.

The validation tests that Eric mentioned should discover circular dependencies that we might miss in the future once we can get it up and running again.

Interestingly, System.Security.Cryptography.Cng does reference System.Memory, but doesn't trigger the circular dependency:

corefx/src/System.Security.Cryptography.Cng/src/System.Security.Cryptography.Cng.csproj

Line 308 in 7be44d2

<Reference Include="System.Memory" />

From System.Memory nuspec:

      <group targetFramework=".NETStandard1.1">
        <dependency id="NETStandard.Library" version="1.6.1" />
        <dependency id="System.Buffers" version="4.5.0-preview2-26313-0" exclude="Compile" />
        <dependency id="System.Runtime.CompilerServices.Unsafe" version="4.5.0-preview2-26313-0" exclude="Compile" />
      </group>

This is most likely because nothing in NETStandard.Library depends on System.Memory (not part of the implementation closure of NETStandard.Library, along with System.Security.Cryptography.Cng). Please let me know if I am mistaken.

Copy link
Member

@ericstj ericstj Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Interestingly, System.Security.Cryptography.Cng does reference System.Memory, but doesn't trigger the circular dependency:

That's because CNG isn't in the package closure of NETStandard.Library. The problem comes about when something in the closure of NETStandard.Library starts referencing NETStandard.Library. For everything subsumed by NETStandard.Library this isn't an issue since we no longer build packages for them. There were, however, a handful of packages which were previously in the implementation closure of NETStandard.Library, but not added to the NETStandard2.0 surface area (not subsumed):

  • System.Buffers
  • System.Diagnostics.DiagnosticSource
  • System.Threading.Tasks.Extensions
  • System.Reflection.Emit
  • System.Reflection.Emit.ILGeneration
  • System.Reflection.Emit.Lightweight
  • System.Reflection.TypeExtensions
  • System.Security.Cryptography.OpenSsl

For any of these we have to make sure that newer versions of the packages don't bring in a transitive reference to NETStandard.Library, otherwise it will create a cycle. The one that did introduce this was System.Threading.Tasks.Extensions > System.Runtime.CompilerServices.Unsafe > NETStandard.Library.

Copy link
Member Author

@ahsonkhan ahsonkhan Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The one that did introduce this was System.Reflection.TypeExtensions > System.Runtime.CompilerServices.Unsafe > NETStandard.Library.

Shouldn't it be System.Threading.Tasks.Extensions?

Copy link
Member

Choose a reason for hiding this comment

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

Right you are. Corrected.

</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>