-
Notifications
You must be signed in to change notification settings - Fork 483
Configure SourceLink, .snupkg symbols package format & update test projects
#722
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
|
appveyor build need to be fixed |
Not the Appveyor build per se, but the build shell scripts for both Windows and Linux used by AppVeyor. You could proceed in either of the following ways:
For the Windows build script ( echo --------------------
echo Running NET462 Tests
echo --------------------
-src\Castle.Core.Tests\bin\%Configuration%\net462\Castle.Core.Tests.exe --result=DesktopClrTestResults.xml;format=nunit3 || exit /b 1
-src\Castle.Core.Tests.WeakNamed\bin\%Configuration%\net462\Castle.Core.Tests.WeakNamed.exe --result=DesktopClrWeakNamedTestResults.xml;format=nunit3 || exit /b 1
+dotnet test src\Castle.Core.Tests -f net462 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="DesktopClrTestResults" || exit /b 1
+dotnet test src\Castle.Core.Tests.WeakNamed -f net462 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="DesktopClrWeakNamedTestResults" || exit /b 1
echo ---------------------------
echo Running NET8.0 Tests
echo ---------------------------
-dotnet .\src\Castle.Core.Tests\bin\%Configuration%\net8.0\Castle.Core.Tests.dll --result=Net80TestResults.xml;format=nunit3 || exit /b 1
-dotnet .\src\Castle.Core.Tests.WeakNamed\bin\%Configuration%\net8.0/Castle.Core.Tests.WeakNamed.dll --result=Net80WeakNamedTestResults.xml;format=nunit3 || exit /b 1
+dotnet test src\Castle.Core.Tests -f net8.0 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="Net80TestResults" || exit /b 1
+dotnet test src\Castle.Core.Tests.WeakNamed -f net8.0 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="Net80WeakNamedTestResults" || exit /b 1
echo ---------------------------
echo Running NET9.0 Tests
echo ---------------------------
-dotnet .\src\Castle.Core.Tests\bin\%Configuration%\net9.0\Castle.Core.Tests.dll --result=Net90TestResults.xml;format=nunit3 || exit /b 1
-dotnet .\src\Castle.Core.Tests.WeakNamed\bin\%Configuration%\net9.0/Castle.Core.Tests.WeakNamed.dll --result=Net90WeakNamedTestResults.xml;format=nunit3 || exit /b 1
+dotnet test src\Castle.Core.Tests -f net9.0 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="Net90TestResults" || exit /b 1
+dotnet test src\Castle.Core.Tests.WeakNamed -f net9.0 -c %Configuration% --no-build -- NUnit.TestOutputXml="%CD%" NUnit.TestOutputXmlFileName="Net90WeakNamedTestResults" || exit /b 1The Linux build script ( echo ---------------------------
echo Running NET8.0 Tests
echo ---------------------------
-dotnet ./src/Castle.Core.Tests/bin/Release/net8.0/Castle.Core.Tests.dll --result=Net80TestResults.xml;format=nunit3
-dotnet ./src/Castle.Core.Tests.WeakNamed/bin/Release/net8.0/Castle.Core.Tests.WeakNamed.dll --result=Net80WeakNamedTestResults.xml;format=nunit3
+dotnet test src\Castle.Core.Tests -f net8.0 -c Release --no-build -- NUnit.TestOutputXml="$PWD" NUnit.TestOutputXmlFileName="Net80TestResults"
+dotnet test src\Castle.Core.Tests.WeakNamed -f net8.0 -c Release --no-build -- NUnit.TestOutputXml="$PWD" NUnit.TestOutputXmlFileName="Net80WeakNamedTestResults"
...
echo ---------------------------
echo Running NET9.0 Tests
echo ---------------------------
-dotnet ./src/Castle.Core.Tests/bin/Release/net9.0/Castle.Core.Tests.dll --result=Net90TestResults.xml;format=nunit3
-dotnet ./src/Castle.Core.Tests.WeakNamed/bin/Release/net9.0/Castle.Core.Tests.WeakNamed.dll --result=Net90WeakNamedTestResults.xml;format=nunit3
+dotnet test src\Castle.Core.Tests -f net9.0 -c Release --no-build -- NUnit.TestOutputXml="$PWD" NUnit.TestOutputXmlFileName="Net90TestResults"
+dotnet test src\Castle.Core.Tests.WeakNamed -f net9.0 -c Release --no-build -- NUnit.TestOutputXml="$PWD" NUnit.TestOutputXmlFileName="Net90WeakNamedTestResults" |
5056214 to
63f421d
Compare
stakx
left a comment
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.
Not my final review, but a couple things I stumbled over.
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.
This is looking mostly good, from what I can tell. Just a few details. After those have been taken care of, this should be ready for merging.
P.S. perhaps you could also add a brief description of your changes in the CHANGELOG.md? For exampe:
Enhancements:
...
+Set up deterministic builds, SourceLink, and switched to `.snupkg` symbol packages (@Romfos, #722)P.P.S: Are we sure that an explicit <PackageReference Include="Microsoft.SourceLink.GitHub" Version="..." PrivateAssets="All" /> is no longer needed these days for SourceLink to work correctly? Has that since been integrated diretly into the .NET SDK? (https://github.com/dotnet/sourcelink?tab=readme-ov-file#using-source-link-in-net-projects)
| <PropertyGroup Condition="'$(CI)' == 'True'"> | ||
| <!--Deterministic Build and Source Link settings --> | ||
| <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild> | ||
| <EmbedUntrackedSources>true</EmbedUntrackedSources> | ||
| <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.
I'm noticing that on local builds, we still get (legacy?) .symbols.nupkg files (like before), while on CI, we get .snupkg files. I am not sure whether the former symbols package format makes particular sense for local builds... but I suspect not.
Perhaps it might be better to put both <IncludeSymbols> and <SymbolPackageFormat> in a non-conditional <PropertyGroup>, such that we always get the same symbols package format.
Or do you have a better recommendation?
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 for me - better solution is to implement it in a right way via dotnet pack and pass this CI = true
some articles:
https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#continuousintegrationbuild
When set to true, this property enables settings that only apply to official builds as opposed to local builds on a developer machine.
For example, stored file paths are normalized for official builds.
But on a local development machine, the debugger isn't able to find local source files if file paths are normalized.
for now this change is not in production =)
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're not quite there yet however.
Are you saying that non-CI builds should not emit any NuGet packages at all? In that case we'd still need to change something here.
(Note that I wasn't suggesting taking the <ContinuousIntegrationBuild> out of the conditional group – that one clearly belongs there. I was referring only to the two last properties.)
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 can build nuget packages locally but probably you don't want to publish to nuget your local package symbols that could contains your local file paths, maybe other sensitive information
As I understand best practice here is to use it on CI
sorry, maybe I'm not filly understand this topic
|
|
.snupkg symbols package format & update test projects
this PR is copy of #700 with removed appveyor changes
key points:
related #700