Skip to content

PGO artifacts are vertical visibility by default except for dotnet-sdk-pgo-* #46063

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

Merged
merged 19 commits into from
Jan 27, 2025

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 16, 2025

Blocked on #46021

Contributes to dotnet/source-build#4648

@jkoritzinsky jkoritzinsky requested review from a team as code owners January 16, 2025 21:37
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Jan 16, 2025
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Out of curiosity, who consumes these?

@jkoritzinsky
Copy link
Member Author

They're consumed by dotnet-optimization to generate the next set of PGO data to feed back into dotnet/runtime.

@@ -95,6 +95,8 @@
<BuildArgs>$(BuildArgs) /p:SourceBuiltAssetManifestsDir=$(RepoAssetManifestsDir)</BuildArgs>
<BuildArgs Condition="'$(OfficialBuildId)' != ''">$(BuildArgs) /p:OfficialBuildId=$(OfficialBuildId)</BuildArgs>
<BuildArgs Condition="'$(ForceDryRunSigning)' != ''">$(BuildArgs) /p:ForceDryRunSigning=$(ForceDryRunSigning)</BuildArgs>
<!-- PGO assets by default are "Vertical" visibilty. Each repo will enable the specific artifacts it must publish externally -->
<BuildArgs Condition="'$(PgoInstrument)' == 'true'">$(BuildArgs) /p:DefaultArtifactVisibility=Vertical</BuildArgs>
Copy link
Member

Choose a reason for hiding this comment

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

@ViktorHofer @jkoritzinsky Is your thinking that default artifact visibility is something that the VMR will be responsible for passing, rather than runtime setting this under certain conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the default visibility should be a property of the VMR for PGO as we need to tell every repo (not just runtime) that its assets are Vertical-only.

For other scenarios (like filtering out RID-agnostic packages if we wanted to get rid of the concept of a main vertical entirely) I could see runtime controlling the default rules itself.

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

All of the sdk-unified-build-full failures will be fixed by #45932 (they're all corner cases that the new tooling missed for maintaining the build experience of current main).

@jkoritzinsky jkoritzinsky removed the Blocked Issue is blocked on an external dependency label Jan 21, 2025
@jkoritzinsky jkoritzinsky force-pushed the pgo-visibility-default branch from 8fdfa88 to 59cca10 Compare January 21, 2025 19:19
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 23, 2025

I don't think we are yet at the point of merging this PR but before doing that, we should queue an official build as only that executes the BuildPass=final stage.

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 24, 2025

This PR should now be unblocked as we just merged the SDK re-bootstrap.

@jkoritzinsky
Copy link
Member Author

Here's an internal official build with the final join from 98a908a: https://dev.azure.com/dnceng/internal/_build/results?buildId=2627132&view=results

It still has the toolset packages on Windows and Arcade's packages, but this job shows that we've already handled all of the cases that were causing bad duplicates (duplicates for assets not produced in win-x64 jobs). The rest we can validate by checking the asset manifests of public CI.

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…ard ways (and it was picking up installers in the linux PGO legs)
@jkoritzinsky jkoritzinsky force-pushed the pgo-visibility-default branch from 55dc4b3 to fe24e02 Compare January 24, 2025 22:01
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

Success! Only the PGO assets are published from the PGO jobs! I'll merge this in

@jkoritzinsky jkoritzinsky merged commit e176707 into dotnet:main Jan 27, 2025
70 of 78 checks passed
@jkoritzinsky jkoritzinsky deleted the pgo-visibility-default branch January 27, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants