Skip to content
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

Put artifacts in well-known locations #18591

Merged
merged 25 commits into from
Feb 13, 2024
Merged

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Feb 9, 2024

Fixes dotnet/source-build#4104
Fixes dotnet/source-build#3696
Contributes to dotnet/source-build#4101
Contributes to dotnet/source-build#4158

  1. Split shipping and non-shipping packages and put them into the Arcade default locations:
    -> artifacts/packages//Shipping
    -> artifacts/packages//NonShipping

Put artifacts under -> artifacts/assets/<config>

Add the local feed for the addition package directory to NuGet.config.

  1. Remove the non-shipping packages list generation now that artifacts are grouped.

  2. Generate the intermediate PackageVersions* files in -> artifacts/obj/PackageVersions

  3. Extract the repo symbol archives intermediates to -> artifacts/obj/Symbols/

  4. Improve artifacts logging and ensuring that artifacts are created

  5. Sequence targets better and apply correct DependsOnTargets

  6. Small clean-up

  7. Allow the poison infrastructure to analyze SBRP packages based on item metadata instead of an expected directory structure in the archive. Make the poison infrastructure analyze nuget packages directly.

@ViktorHofer ViktorHofer requested a review from a team as a code owner February 9, 2024 09:42
@ViktorHofer ViktorHofer marked this pull request as draft February 9, 2024 09:42
@ViktorHofer ViktorHofer force-pushed the ArtifactsLocationRevamp branch 3 times, most recently from 69692a9 to fbea002 Compare February 9, 2024 10:30
Comment on lines +292 to +302
<Target Name="SetSourceBuiltSdkOverrides"
Condition="'@(SourceBuiltSdkOverride)' != ''">
<ItemGroup>
<!-- Set the environment variables for MSBuild to look for our additional SDK Resolvers and or our resolver to find our source-built SDKs. -->
<EnvironmentVariables Include="MSBUILDADDITIONALSDKRESOLVERSFOLDER=$(VSMSBuildSdkResolversDir)" />
<EnvironmentVariables Include="SOURCE_BUILT_SDK_ID_%(SourceBuiltSdkOverride.Group)=%(SourceBuiltSdkOverride.Identity)" />
<EnvironmentVariables Include="SOURCE_BUILT_SDK_VERSION_%(SourceBuiltSdkOverride.Group)=%(SourceBuiltSdkOverride.Version)" />
<EnvironmentVariables Condition="'%(SourceBuiltSdkOverride.Location)' != ''" Include="SOURCE_BUILT_SDK_DIR_%(SourceBuiltSdkOverride.Group)=%(SourceBuiltSdkOverride.Location)/" />
<EnvironmentVariables Condition="'%(SourceBuiltSdkOverride.Location)' == ''" Include="SOURCE_BUILT_SDK_DIR_%(SourceBuiltSdkOverride.Group)=$(SourceBuiltSdksDir)%(SourceBuiltSdkOverride.Identity)/" />
</ItemGroup>
</Target>
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 target just got moved up to the build stage.

Comment on lines -339 to -348
<Target Name="Build" DependsOnTargets="BuildRepoReferences;RepoBuild" />

<Target Name="Package"
AfterTargets="Build"
DependsOnTargets="
CopyRepoArtifacts;
CopyInnerBuildRestoredPackages;
EnsurePackagesCreated;
ExtractToolPackage;
CleanupRepo" />

Copy link
Member Author

Choose a reason for hiding this comment

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

These targets got combined and moved down.

1. Shipping and non-shipping artifacts to Arcade default locations:
-> artifacts/packages/<config>/Shipping
-> artifacts/packages/<config>/NonShipping

Add the local feed for the addition package directory to NuGet.config
Remove the non-shipping packages list generation now that artifacts
are grouped.

2. Genereated the intermediate PackageVersions* files in
-> artifacts/obj/PackageVersions

3. Extract the repo symbol archives intermediates to
-> artifacts/obj/Symbols/

4. Sequence targets better and apply correct DependsOnTargets

5. Small clean-up

Fixes dotnet/source-build#4104
Fixes dotnet/source-build#3696
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 9, 2024
<SourceBuiltBlobFeedDir>$([MSBuild]::NormalizeDirectory('$(SharedIntermediateOutputPath)', 'blob-feed'))</SourceBuiltBlobFeedDir>
<SourceBuiltPackagesPath>$([MSBuild]::NormalizeDirectory('$(SourceBuiltBlobFeedDir)', 'packages'))</SourceBuiltPackagesPath>
<SourceBuiltAssetsDir>$([MSBuild]::NormalizeDirectory('$(SourceBuiltBlobFeedDir)', 'assets'))</SourceBuiltAssetsDir>
<IntermediateSymbolsRootDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsObjDir)', 'Symbols'))</IntermediateSymbolsRootDir>
Copy link
Member

Choose a reason for hiding this comment

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

How does the Microsoft build gather/publish symbols? Does it want the individual symbol packages? I ask wondering if the symbols should be places next to the packages they are for?

Copy link
Member Author

Choose a reason for hiding this comment

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

How does the Microsoft build gather/publish symbols? Does it want the individual symbol packages?

Correct. Symbol packages are placed next to the non symbol packages in MSFT builds. We want symbol packages here as well but they are currently filtered out: dotnet/source-build#4110

@MichaelSimons
Copy link
Member

Because of the scope of these changes please run a full CI run.

@ViktorHofer ViktorHofer marked this pull request as ready for review February 10, 2024 11:51
Comment on lines 59 to 61
scope: full
${{ else }}:
scope: lite
scope: full
Copy link
Member Author

Choose a reason for hiding this comment

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

Temp change to queue a full build

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 12, 2024

The failures are due to dotnet/source-build#4111. I validated that the Private.* source-build artifacts are produced correctly and with the expected content. Also verified the other pipeline artifacts (packages & logs). Also did offline builds with these changes.

@MichaelSimons
Copy link
Member

MichaelSimons commented Feb 12, 2024

The failures are due to dotnet/source-build#4111. Validate that the Private.* source-build artifacts are produced correctly and with the expected content. Also verified the other pipeline artifacts (packages & logs). Also did offline builds with these changes.

Unfortunately the Alpine w/previous SDK leg is the one that has the poison test enabled. Would you mind temporarily enabling poison on the alpine leg with the MSFT Sdk to validate there are no regressions in the poison infra?

@MichaelSimons
Copy link
Member

Looks like there are numerous poison leaks. @NikolaMilosavljevic can you please review the changes to help RCA the Poison infra regressions.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 13, 2024

@MichaelSimons @NikolaMilosavljevic I fixed the leak detection issue and updated the branch with latest main. The remaining failure exists in main as well. This PR is now ready. PTAL.

In comparison, here's a full build from main: https://dnceng.visualstudio.com/internal/_build/results?buildId=2377522&view=results

@@ -111,21 +83,22 @@
Inputs="$(MSBuildProjectFullPath)"
Outputs="$(BaseIntermediateOutputPath)ReportPoisonUsage.complete" >
<ItemGroup>
<FinalCliTarball Include="$(SharedOutputPath)**/*$(ArchiveExtension)" />
<!-- Exclude the Private.SourceBuilt.Artifacts archive from poison usage scan. -->
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move this comment to be directly above the line that is doing the removal?

@MichaelSimons
Copy link
Member

Merging ahead of PR validation since the last change was to revert the changes to run additional legs for validation purposes.

@MichaelSimons MichaelSimons merged commit 94def2e into main Feb 13, 2024
3 of 21 checks passed
@MichaelSimons MichaelSimons deleted the ArtifactsLocationRevamp branch February 13, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants