-
Notifications
You must be signed in to change notification settings - Fork 366
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
Enable Central Package Management #1386
base: release/v6
Are you sure you want to change the base?
Enable Central Package Management #1386
Conversation
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.
A few things to look over
Directory.Packages.props
Outdated
<PackageVersion Include="xunit.runner.visualstudio" Version="2.5.7"> | ||
<PrivateAssets>all</PrivateAssets> | ||
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
</PackageVersion> |
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 believe PrivateAssets
, IncludeAssets
etc belongs to PackageReference
and not PackageVersion
. I pushed a fix for this.
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.
You are correct, thanks for noticing.
<IncludeSymbols>true</IncludeSymbols> | ||
<SymbolPackageFormat>snupkg</SymbolPackageFormat> | ||
</PropertyGroup> | ||
|
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.
Aha, it seems .NET8 includes these by default now. Nice.
Starting with .NET 8, Source Link for the following source control providers is included in the .NET SDK and enabled by default
https://github.com/dotnet/sourcelink
However, we target netstandard2.0
in this project, so I guess we need to keep those?
Including the packagereference Microsoft.SourceLink.GitHub
below.
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.
The way I understand this is that these settings and packagereference are only used at build/package time, not at runtime, so as long as UnitsNet is build with the .NET 8 SDK it should work with netstandard2.0
.
I want to validate this but have some trouble. Even if I build the latest master unmodified with the dotnet build
and dotnet pack
commands from Build/build-functions.psm1
the resulting .nupkg
has missing symbols and compiler flags when checked with https://nuget.info
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.
@angularsen any thoughts on this? Am I doing something wrong? Does this only work in CI?
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.
@Muximize CI should not do anything different I can think of.
As far as I can tell, the symbols package .snupkg
includes the .pdb
file as expected and .nupkg
includes source code and has valid SourceLink for the commit I had currently checked out: 91a85e2dac60b420fabc44ebde71fc85dab33d6c
git checkout origin/master # Where origin is the original repository
cd UnitsNet
dotnet pack
UnitsNet -> X:\unitsnet\Artifacts\UnitsNet\net7.0\UnitsNet.dll
Successfully created package 'X:\unitsnet\Artifacts/UnitsNet\UnitsNet.5.50.0.nupkg'.
Successfully created package 'X:\unitsnet\Artifacts/UnitsNet\UnitsNet.5.50.0.snupkg'.
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.
Alright, I finally looked into this. Turns out IncludeSymbols
and SymbolPackageFormat
are indeed needed after al, so I restored that but as arguments to dotnet pack
in build-functions.psm1
. This also enables sourcelink for the NumberExtensions project without needing per-project settings.
The packagereference is not needed, the symbols and sourcelink are also generated for netstandard2.0
if build with the .NET 8 SDK.
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.
Is it not better to keep it in csproj? Or for consistency, move it to Directory.Build.props
at the repo root?
This way, it packs nugets the same way regardless of doing it manually in command line, or with scripts or with built-in tools in IDEs.
<!-- Optional: Build symbol package (.snupkg) to distribute the PDB containing Source Link --> | ||
<IncludeSymbols>true</IncludeSymbols> | ||
<SymbolPackageFormat>snupkg</SymbolPackageFormat> | ||
</PropertyGroup> |
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.
Same here, I think we need to keep this
337c22f
to
00fbc0e
Compare
d5598e0
to
9968d82
Compare
This PR removes some seemingly unused dependencies, and moves all dependency versions to a single file according to Central Package Management.