-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Check shipping packages for poison #47176
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
<!-- Include shipping nuget packages. --> | ||
<PoisonFileToCheck Include="$(ArtifactsShippingPackagesDir)*.nupkg" /> | ||
<ShippingPackageToCheck Include="$(ArtifactsShippingPackagesDir)**/*.nupkg" /> | ||
<Error Condition="'@(ShippingPackageToCheck)' == ''" Text="No shipping packages will be poison checked - this is unexpected!" /> |
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.
Add another similar check for PoisonFileToCheck
?
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.
Sure, good idea - thanks!
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.
Checking at the end would never work as the new check I'm adding for shipping packages would fail first. But, I can add a check for assets, i.e. the value of PoisonFileToCheck
, right before adding shipping packages.
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.
Fixed with deb765a
I've added a check for assets.
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.
Actually, can we divide these categories of files into separate item groups that we can then validate individually? For example, defined an item group for the PSB artifacts and a separate one for the SBRPs. Check that each of those item groups are non-empty, just as you do for the separate item group of the shipping packages. Then at the end, add them all to PoisonFileToCheck
.
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 was also thinking of that and it does make sense. Best time to do it is now 👍
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.
Fixed with 5d80078
@MichaelSimons @mmitche can you help me merge this? The failure is unrelated to my changes. |
/backport to release/9.0.1xx |
Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/13635815357 |
Fixes: dotnet/source-build#4921
Shipping packages are in a repo-named sub-directory.