Ensure all packages are created in CI

[heng-liu]

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/666
Regression: No

• Last working version:
• How are we preventing it in future:

Fix

Details:
1.Enable packing symbol (revert #3758) as it breaks the source link.

2.On CI, after packing, check if all packages are created by comparing the count of projects need to be packed and the count of .nupkg/.symbol.nupkg files.

If the three numbers match, show a message:
Count of Projects need to be packed: 25 Nupkg Count: 25 Symbol Count: 25

If the three numbers don't match, break the build and show both above message and an error as following:
There are 25 projects need to be packed, but only 25 nupkg files and 24 symbol nupkg files are created under 'C:\A\1\318\s\artifacts\ReleaseNupkgs\'.

Note: When building for RTM, the test project(Test.Utility) will not be packed.
So the projects need to be packed for RTM is less than that of nonRTM by 1.

Testing/Validation

Tests Added: No
Reason for not adding tests: Engineering change
Validation:
Run msbuild .\build\build.proj /t:EnsurePackagesExist /p:ExcludeTestProjects=$(BuildRTM) locally and change the number of packages see if the message and error are correctly shown.

[nkolev92]

This runs on one project, so this is really a self reporting target.

The name might be if it started with Is instead of Get.

See example:

NuGet.Client/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

Lines 1286 to 1299 in f308eaa

	<!--
	============================================================
	_IsProjectRestoreSupported
	Verify restore targets exist in the project.
	============================================================
	-->
	<Target Name="_IsProjectRestoreSupported"
	Returns="@(_ValidProjectsForRestore)">
	<ItemGroup>
	<_ValidProjectsForRestore Include="$(MSBuildProjectFullPath)" />
	</ItemGroup>
	</Target>

[heng-liu]

Thanks for reviewing! Fixed.

[nkolev92]

I think it would've been nicer if we validated the exact package ids.

[nkolev92]

I understand that will be more work, more complicated, so get a 2nd opinion before jumping into it. It's an idea, not necessarily a request for change.

[zivkan]

When I created the issue, I had imagined a powershell script that ran after pack that checked the filenames or package ids or something. A separate list of expected packages in any case.

This implementation avoids having redundant data to check that all the packages were created successfully, that needs to be maintained each time we add/remove packages. But a redundant list would provide an additional fail-safe. We very rarely add/remove packages, so it wouldn't be a big burden. This change has a theoretical risk that if a project accidentally stops being packable for any reason (and since it's via msbuild, it's a non-trivial risk due to automatic imports, environment variables, etc) we won't catch it.

Having said that, we also have a task to modify our publishing pipeline to validate all the packages exist before we try to push. The problem there is if we detect an error, we have to spin a new build.

Overall, I'm undecided. On one hand, I did imagine us having a list of package/file names that we expected to exist. On the other hand, I believe this change would have caught the problem that prevented our 5.8 packages from being generated correctly, and the theoretical risk of accidentally setting IsPackable to something other than true is low risk.

The more I think about it, the more fine I am, personally.

[heng-liu]

My understanding is, pack task runs pack target, which calls PackProjects target. And the PackProjects target only runs when PackProject is true.
So the count of projects with PackProject= true should be the count of packages need to be created. If the count of *.nupkg/*.symbols.nupkg files both match with the count of projects need to be packed, the only risk is there is some old or other *.nupkg/*.symbols.nupkg in the folder, making the count exactly match(miss packing certain packages, but has exactly numbers of other packages, so the counts still match), which has a very low chance when running on CI, as we create a new folder on CI for each build.
I'll add a checklist to check for exact package ids in another PR later.

[nkolev92]

Overall, I'm undecided. On one hand, I did imagine us having a list of package/file names that we expected to exist. On the other hand, I believe this change would have caught the problem that prevented our 5.8 packages from being generated correctly, and the theoretical risk of accidentally setting IsPackable to something other than true is low risk.

I think either way is good enough as well.

[heng-liu]

I've added checking on each package ids. @nkolev92 @zivkan

[heng-liu]

Reverted the last commit as it doesn't provide any additional safety.

[zivkan]

When I created the issue, I had imagined a powershell script that ran after pack that checked the filenames or package ids or something. A separate list of expected packages in any case.

This implementation avoids having redundant data to check that all the packages were created successfully, that needs to be maintained each time we add/remove packages. But a redundant list would provide an additional fail-safe. We very rarely add/remove packages, so it wouldn't be a big burden. This change has a theoretical risk that if a project accidentally stops being packable for any reason (and since it's via msbuild, it's a non-trivial risk due to automatic imports, environment variables, etc) we won't catch it.

Having said that, we also have a task to modify our publishing pipeline to validate all the packages exist before we try to push. The problem there is if we detect an error, we have to spin a new build.

Overall, I'm undecided. On one hand, I did imagine us having a list of package/file names that we expected to exist. On the other hand, I believe this change would have caught the problem that prevented our 5.8 packages from being generated correctly, and the theoretical risk of accidentally setting IsPackable to something other than true is low risk.

The more I think about it, the more fine I am, personally.