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.

Sign up for GitHub

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
Merged

Fix unwanted transitive updates to compile only references #46019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec4dcaf
Fix unwanted transitive updates to compile only references
ericstj Jan 16, 2025
6b44a03
Include PackageValidation and update comment
ericstj Jan 16, 2025
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
2 changes: 2 additions & 0 deletions ...tibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/Microsoft.DotNet.ApiCompat.Task.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFrameworks>$(NetToolMinimum);$(NetFrameworkToolCurrent)</TargetFrameworks>
<!-- Disable transitive pinning, we shouldn't upgrade dependencies that are excluded for runtime and provided externally. -->
<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:

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>

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.

<IsPackable>true</IsPackable>
<IsShippingPackage>true</IsShippingPackage>
<IncludeBuildOutput>false</IncludeBuildOutput>
Expand Down
2 changes: 2 additions & 0 deletions ...lity/ApiCompat/Microsoft.DotNet.ApiCompatibility/Microsoft.DotNet.ApiCompatibility.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFrameworks>$(NetToolMinimum);$(NetFrameworkToolCurrent)</TargetFrameworks>
<!-- Disable transitive pinning, we shouldn't upgrade dependencies that are excluded for runtime and provided externally. -->
<CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled>
<!-- We need to compare ISymbols in a special way (by name) and roslyn symbol comparers take more heuristics into consideration.-->
<NoWarn>$(NoWarn);RS1024</NoWarn>
</PropertyGroup>
Expand Down
2 changes: 2 additions & 0 deletions ...ty/ApiCompat/Microsoft.DotNet.PackageValidation/Microsoft.DotNet.PackageValidation.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFrameworks>$(NetToolMinimum);$(NetFrameworkToolCurrent)</TargetFrameworks>
<!-- Disable transitive pinning, we shouldn't upgrade dependencies that are excluded for runtime and provided externally. -->
<CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled>
</PropertyGroup>

<ItemGroup>
Expand Down
2 changes: 2 additions & 0 deletions ...tibility/Microsoft.DotNet.ApiSymbolExtensions/Microsoft.DotNet.ApiSymbolExtensions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

<PropertyGroup>
<TargetFrameworks>$(NetToolMinimum);$(NetFrameworkToolCurrent)</TargetFrameworks>
<!-- Disable transitive pinning, we shouldn't upgrade dependencies that are excluded for runtime and provided externally. -->
<CentralPackageTransitivePinningEnabled>false</CentralPackageTransitivePinningEnabled>
</PropertyGroup>

<!-- Exclude files that depend on Microsoft.Build.Framework and Microsoft.Build.Utilities.Core. These should be manually included by other projects. -->
Expand Down
Loading