-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
sdk/src/Compatibility/Directory.Packages.props
Lines 5 to 9 in ec4dcaf
<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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Failures are all Known Issues |
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: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.