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.

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

Jump to bottom

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

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

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

wants to merge 8 commits into from
+137 −9

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 16, 2025 •
edited
Loading

Blocked on #46021

Contributes to dotnet/source-build#4648

@jkoritzinsky jkoritzinsky requested review from a team as code owners January 16, 2025 21:37
@dotnet-issue-labeler dotnet-issue-labeler bot 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

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

ViktorHofer
ViktorHofer approved these changes Jan 16, 2025
View reviewed changes
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.

mmitche
mmitche reviewed Jan 16, 2025
View reviewed changes
src/SourceBuild/content/repo-projects/Directory.Build.props
@@ -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 jkoritzinsky mentioned this pull request Jan 16, 2025
Merged
@jkoritzinsky jkoritzinsky added Blocked Issue is blocked on an external dependency Area-VMR and removed Area-Infrastructure untriaged Request triage from a team member labels Jan 17, 2025
This was referenced Jan 17, 2025
Open
Open
Open
Open
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

@azure-pipelines Azure Pipelines
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

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member 

    /__w/1/vmr/src/runtime/src/mono/monoaotcross.proj(42,7): error MSB4184: The expression """.Substring(0, -1)" cannot be evaluated. length ('-1') must be a non-negative value. (Parameter 'length')
    /__w/1/vmr/src/runtime/src/mono/monoaotcross.proj(42,7): error MSB4184: Actual value was -1.

I think I saw this error in #45932 as well but am not 100% sure.

@jkoritzinsky
Copy link
Member Author

I just fixed that error in the PGO leg there earlier today.

@build-analysis build-analysis bot mentioned this pull request Jan 21, 2025
Open
3 tasks
@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Jan 21, 2025 •
edited
Loading

Looking at CI, there's 3 remaining issues:

  • arcade won't respect the default artifact visibility until we re-bootstrap with an Arcade that supports specifying the default visibility.
  • nuget-client isn't respecting the default artifact visibility
  • sdk needs to be updated to not publish the toolset zips when PgoInstrument is true.

jkoritzinsky added a commit to jkoritzinsky/NuGet.Client that referenced this pull request Jan 22, 2025
@jkoritzinsky
Use the documented Artifact extension point to add artifacts
62c4b5f 
The Artifact extension point handles symbol packages automatically for source-build, and it automatically gets support for new features like Artifact Visibility (required for dotnet/sdk#46063)
@jkoritzinsky jkoritzinsky mentioned this pull request Jan 22, 2025
Open
3 tasks
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jan 22, 2025
Open
@jkoritzinsky
Copy link
Member Author

/azp run sdk-unified-build-full

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jan 22, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmitche mmitche mmitche approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

Assignees
No one assigned
Labels
Area-VMR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jkoritzinsky @ViktorHofer @mmitche