Skip to content

Fix unwanted transitive updates to compile only references #46019

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.

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

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jan 16, 2025

Fix #45004

What's happening is the CPM+TransitivePinning is updating Roslyn's transitive dependencies. Some of these are not used, but some are.

In the case of APICompat, it was using System.Memory which wasn't directly updated, but was updated as a result of updating System.Text.Encoding.Codepages:

  [net472]
   │
   └─ Microsoft.DotNet.ApiSymbolExtensions (v10.0.100-dev)
      └─ Microsoft.CodeAnalysis.CSharp (v4.4.0)
         └─ Microsoft.CodeAnalysis.Common (v4.4.0)
            ├─ System.Collections.Immutable (v6.0.0)
            │  └─ System.Memory (v4.6.0)
            ├─ System.Memory (v4.6.0)
            ├─ System.Reflection.Metadata (v5.0.0)
            │  └─ System.Collections.Immutable (v6.0.0)
            │     └─ System.Memory (v4.6.0)
            └─ System.Text.Encoding.CodePages (v10.0.0-alpha.1.25063.12)
               └─ System.Memory (v4.6.0)

  [net8.0]
   │
   └─ Microsoft.DotNet.ApiSymbolExtensions (v10.0.100-dev)
      └─ Microsoft.CodeAnalysis.CSharp (v4.4.0)
         └─ Microsoft.CodeAnalysis.Common (v4.4.0)
            └─ System.Memory (v4.5.5)

What we really need here is a way to tell NuGet not to update transitive references from compile-only packages, in addition to excluding those from audit. cc @nkolev92 @zivkan this is what we've been discussing around plugin handling.

@ericstj ericstj requested a review from a team as a code owner January 16, 2025 02:44
@ghost ghost added Area-ApiCompat untriaged Request triage from a team member labels Jan 16, 2025
@ericstj ericstj requested a review from ViktorHofer January 16, 2025 02:44
@@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFrameworks>$(NetToolMinimum);$(NetFrameworkToolCurrent)</TargetFrameworks>
<!-- Disable transitive pinning, we can't upgrade dependencies that are excluded from shipping -->
<CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled>
Copy link
Member Author

Choose a reason for hiding this comment

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

I did this to all the places that were contributing to APICompat task. It should not impact the tool.

We might also want something similar for GenAPI - but I doubt we ever run that in desktop MSBuild.

Copy link
Member

Choose a reason for hiding this comment

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

What about Microsoft.DotNet.PacakgeValidation.csproj?

Copy link
Member

Choose a reason for hiding this comment

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

As an alternative, could we pin the problematic packages to the lower version? We are already doing this for CodeAnalysis:

<ItemGroup Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(SkipMicrosoftCodeAnalysisCSharpPinning)' != 'true'">
<!-- We pin the code analysis version when not in source build as we need to support running on older
SDKs when the OOB package is used. -->
<PackageVersion Update="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" />
</ItemGroup>

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd need to pin the entire closure of CodeAnalysis, including anything it might take a dependency on in the future. IMO that's not a great solution - pretty fragile and difficult to maintain.

We should pin our "entrypoint" packages where we have a clear understanding of target version. We let them define which other packages they can provide and don't do so ourselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add -- it's not great that we have to apply this to the entire project. The hero feature here would be that we can apply this to a single PackageReference - that package is pinned to the version that's referenced, all it's dependencies are exempt from transitive pinning, only exposed for compile, and also exempt from audit and CG.

@ericstj
Copy link
Member Author

ericstj commented Jan 16, 2025

Failures are all Known Issues

@ericstj ericstj merged commit 0b84be0 into dotnet:main Jan 16, 2025
35 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ApiCompat untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APICompat is failing when being used with desktop msbuild
3 participants