Skip to content

Include the SDK tarball in poison checks #48550

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Include the SDK tarball in poison checks #48550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec7d0ac
Include SDK in poison checks
NikolaMilosavljevic Apr 17, 2025
3cd22d9
Follow up
NikolaMilosavljevic Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/SourceBuild/content/eng/finish-source-only.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,13 @@
<UsingTask TaskName="Microsoft.DotNet.SourceBuild.Tasks.LeakDetection.CheckForPoison" AssemblyFile="$(MicrosoftDotNetSourceBuildTasksLeakDetectionAssembly)" TaskFactory="TaskHostFactory" Condition="'$(EnablePoison)' == 'true'" />
<Target Name="ReportPoisonUsage"
BeforeTargets="Build"
DependsOnTargets="CopySdkArchive"
Condition="'$(EnablePoison)' == 'true'"
Inputs="$(MSBuildProjectFullPath)"
Outputs="$(BaseIntermediateOutputPath)ReportPoisonUsage.complete" >
<ItemGroup>
<!-- Exclude the Private.SourceBuilt.Artifacts archive from poison usage scan. -->
<AssetToCheck Include="$(ArtifactsAssetsDir)*$(ArchiveExtension)" />
<AssetToCheck Remove="$(ArtifactsAssetsDir)$(SourceBuiltArtifactsTarballName)*" />
<!-- Include dotnet-sdk-*.tar.gz -->
<AssetToCheck Include="$(ArtifactsAssetsDir)dotnet-sdk-*$(ArchiveExtension)" />
Copy link
Member

@ViktorHofer ViktorHofer Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to use the (hardlink) copy instead of just $(SdkTarballPath)? The copy only exists as a convenience for SB partners:

sdk/src/SourceBuild/content/eng/finish-source-only.proj

Line 306 in fe5ce36

<!-- Copy SDK archive to assets root as source-only build partners expect the file to be there. -->

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This model preserves an option to add more asset files in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm following. The current include only matches on the SDK archive itself, not on other files.

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an Error Task that throws when AssetToCheck is empty so that this doesn't regress easily again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have it:

sdk/src/SourceBuild/content/eng/finish-source-only.proj

Line 140 in fe5ce36

<Error Condition="'@(AssetToCheck)' == ''" Text="No assets will be poison checked - this is unexpected!" />

It wasn't failing before due to us collecting 2 symbols tarballs, so AssetToCheck wasn't empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. You could change this to verify that the sdk archive is included.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

<SdkArchive Include="..." />
<Error Condition="'@(SdkArchive)' == ''" Text="eRROR" />
<AssetToCheck Include="@(SdkArchive)" />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll add that. It doesn't hurt to have this specific check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 3cd22d9

<!-- Include shipping nuget packages. -->
<ShippingPackageToCheck Include="$(ArtifactsShippingPackagesDir)**/*.nupkg" />
<!-- Add and mark SBRP packages to validate that they have the correct poison attribute. -->
Expand Down
Loading
Loading