-
-
Notifications
You must be signed in to change notification settings - Fork 221
add central nuget version mgmt #4422
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
base: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- add central nuget version mgmt ([#4422](https://github.com/getsentry/sentry-dotnet/pull/4422)) If none of the above apply, you can opt out of this check by adding |
<PackageReference Include="Microsoft.EntityFrameworkCore" Version="3.1.32" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="3.1.32" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" /> |
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.
Bug
The introduction of Central Package Management (CPM) via Directory.Packages.props
has inadvertently removed framework-specific package versioning from individual .csproj
files.
Previously, projects used different versions of packages (e.g., Microsoft.Extensions.*
, Microsoft.AspNetCore.Components.WebAssembly
, Serilog
, EntityFramework
, Microsoft.EntityFrameworkCore
) based on their target framework (e.g., net8.0
vs net9.0
, net462
vs netstandard2.0
). Now, all target frameworks are forced to use a single, often newer, version defined in Directory.Packages.props
.
This unification causes compatibility issues, leading to build or runtime failures for projects targeting older frameworks (e.g., net8.0
, netstandard2.0
, net462
, net48
) that are incompatible with the newer package versions. Specifically, it breaks the intended test matrix for Microsoft.EntityFrameworkCore
across different .NET target frameworks.
Additional Locations (9)
Directory.Packages.props#L41-L45
src/Sentry.Extensions.Logging/Sentry.Extensions.Logging.csproj#L18-L33
src/Sentry.AspNetCore/Sentry.AspNetCore.csproj#L19-L24
src/Sentry.AspNetCore.Blazor.WebAssembly/Sentry.AspNetCore.Blazor.WebAssembly.csproj#L13-L19
Directory.Packages.props#L58-L59
Directory.Packages.props#L104-L105
Directory.Packages.props#L77-L78
src/Sentry.Serilog/Sentry.Serilog.csproj#L29-L36
src/Sentry.EntityFramework/Sentry.EntityFramework.csproj#L23-L30
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="2.1.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" Version="2.1.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'"> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="8.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" Version="8.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'net9.0'"> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="9.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="9.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" Version="9.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging.Configuration" /> | ||
<PackageReference Include="Microsoft.Extensions.Http" /> | ||
</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.
thought: CPM for non-Packable source only
Although I do like CPM, as it simplifies package updates, I have the feeling it's a bit "dangerous" for our /src/..
packages, as we may accidentally update a dependency of our NuGet packages. We could also accidentally update transitive dependencies of our packable production projects should we use <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>
(not in this PR, but should we add it later we may miss something).
This makes - IMO - reviewing Version
upgrades of PackageReference
harder, as we would need to verify first whether a change to Directory.Packages.props
also impacts any packable /src/..
project, and if so, whether that change was intended.
I am now thinking about enabling CPM for non-production/packable-Projects only:
- explicitly enable for
/benchmarks/
,/samples/
, and/test/
- explicitly disable for
/src/
This would make PackageReference
-related "Repository Maintenance" tasks easier and quicker,
while keeping our Production NuGet packages explicit and intentional.
WIP
Resolves: #4421