dotnet / maintenance-packages

Repository that hosts packages from the .NET platform whose original home/branch is not building any longer.
MIT License
16 stars 10 forks source link

Improve packing #79

Closed carlossanlop closed 7 months ago

carlossanlop commented 7 months ago

Follow up for the feedback left in https://github.com/dotnet/maintenance-packages/pull/65

ViktorHofer commented 7 months ago

Looks good. You can revert all the project file changes and remove the new stuff in Directory.Build.props. What remains is to be able to only build the sources without the test for that packaging leg. Right now tests are built and packed:

Successfully created package '/__w/1/s/artifacts/packages/Release/NonShipping/System.Reflection.DispatchProxy.Tests.1.0.0-ci.24168.1.nupkg'.

carlossanlop commented 7 months ago

With the latest commits, which pass the global property /p:Test=false in the packing leg for PRs, we're no longer creating nupkgs for the test projects: https://dev.azure.com/dnceng-public/public/_build/results?buildId=607838&view=logs&j=60dbaa24-68a7-5962-8c66-270b428b4658&t=0f846b6f-8d8e-592b-9ef5-850ae44e3537

The official run should still be able to generate the package if we set <IsPackable>true</IsPackable> in a csproj (tested locally).

ViktorHofer commented 7 months ago

which pass the global property /p:Test=false in the packing leg for PRs

Note that the Test property has a special meaning in Arcade's build wrapper project which could lead to unexpected behavior: https://github.com/dotnet/arcade/blob/777bc46bd883555cf89b8a68e3e2023fd4f1ee50/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj#L106 It controls whether test projects should be run.

You might want to condition on BuildTests instead and pass that in.

carlossanlop commented 7 months ago

Note that the Test property has a special meaning in Arcade's build wrapper project which could lead to unexpected behavior

Classic arcade ☹️.

You might want to condition on BuildTests instead and pass that in.

Between your first suggestion, SkipTests, and BuildTests, I think I prefer SkipTests because the default can be '' or false, and we only need to check the condition if the value is true.