Include the SDK tarball in poison checks #48550
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
NikolaMilosavljevic
changed the title
| <AssetToCheck Include="$(ArtifactsAssetsDir)*$(ArchiveExtension)" /> | ||
| <AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" /> | ||
| <!-- Include dotnet-sdk-*.tar.gz --> | ||
| <AssetToCheck Include="$(ArtifactsAssetsDir)dotnet-sdk-*$(ArchiveExtension)" /> |
There was a problem hiding this comment.
Do we need to use the (hardlink) copy instead of just $(SdkTarballPath)? The copy only exists as a convenience for SB partners:
There was a problem hiding this comment.
This model preserves an option to add more asset files in the future.
There was a problem hiding this comment.
I'm not sure I'm following. The current include only matches on the SDK archive itself, not on other files.
There was a problem hiding this comment.
To clarify. Currently, this will only match the SDK. The intent is to preserve a possibility to expand AssetToCheck in the future, with additional files in source-build preferred root assets dir. Since we already copy (hard-link) the SDK to this directory, I'm keeping it consistent to reference the SDK from the same dir, using a pattern.
There was a problem hiding this comment.
Does this list indicate that the product has a ton of poisoned artifacts? (i.e. SBRP)
There was a problem hiding this comment.
Not likely, as some of those are ref. assemblies and the rest are in netfx folders that aren't usable in source build.
I am investigating those issues at the moment. Will create a tracking issue as well.
| <AssetToCheck Include="$(ArtifactsAssetsDir)*$(ArchiveExtension)" /> | ||
| <AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" /> | ||
| <!-- Include dotnet-sdk-*.tar.gz --> | ||
| <AssetToCheck Include="$(ArtifactsAssetsDir)dotnet-sdk-*$(ArchiveExtension)" /> |
There was a problem hiding this comment.
Consider adding an Error Task that throws when AssetToCheck is empty so that this doesn't regress easily again.
There was a problem hiding this comment.
We already have it:
It wasn't failing before due to us collecting 2 symbols tarballs, so AssetToCheck wasn't empty.
There was a problem hiding this comment.
Right. You could change this to verify that the sdk archive is included.
There was a problem hiding this comment.
Example:
<SdkArchive Include="..." />
<Error Condition="'@(SdkArchive)' == ''" Text="eRROR" />
<AssetToCheck Include="@(SdkArchive)" />
There was a problem hiding this comment.
Yeah, I'll add that. It doesn't hurt to have this specific check.
There was a problem hiding this comment.
Will get rid of AssetToCheck items and only have SdkArchive - we can add additional asset files if we need them in the future. Being explicit makes sense here. I will continue to obtain the SDK archive from source-build specific location.
There was a problem hiding this comment.
Fixed with 3cd22d9
Fixes: dotnet/source-build#5052
We used to scan the sdk before, but this was regressed by some change in build infra. This PR adds the required
DependsOnTargetattribute, that would ensure that SDK is present in the assets dir.As we do not need to scan other tarballs in the assets dir, I've updated the pattern to only select the SDK tarball. We do not need to scan symbols tarballs. By explicitly including the SDK, the existing check will ensure that there are no regressions in the future.
This PR also updates the poison baseline to account for those discovered in the SDK. Tracking issue for fixing poisons: dotnet/source-build#5057