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

Official Builds With Custom OptProf 'Just Work' #6411

Merged
merged 3 commits into from
May 29, 2021
Merged

Official Builds With Custom OptProf 'Just Work' #6411

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7cd569
Ideal scenario
benvillalobos May 4, 2021
eafdd44
Use optprofdrop var when building.
benvillalobos May 4, 2021
4ffeee7
Pass OptProfDropName as a pipeline parameter to use at compile-time. …
benvillalobos May 5, 2021
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
20 changes: 18 additions & 2 deletions .vsts-dotnet.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,28 @@ trigger:
# SignType: real
# SkipApplyOptimizationData: false
Comment on lines 10 to 11
Copy link
Member

Choose a reason for hiding this comment

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

🙌🏻 we can promote these now too! I'll send the PR after this one goes in.

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 considered promoting the other vars but realized I only ever changed the value of SkipApplyOptimizationData once ever 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't appear to have worked so looks like you were totally right. https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4728100&view=logs&j=bb592630-4b9d-53ad-3960-d954a70a95cf&t=bb592630-4b9d-53ad-3960-d954a70a95cf

🙇🏻‍♂️

That one is helpful to bootstrap from nothing to get a build from which you can collect updated optprof data. But in no other circumstance. And SignType is basically never helpful. So I'm going to stop bothering.

Copy link
Member Author

@benvillalobos benvillalobos May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For whoever decides to do this in the future:

I think it's because the call to CIBuild.cmd needs to be updated to take the pipeline parameter directly. Or some script variable needs be created to take in that value?

Copy link
Member

Choose a reason for hiding this comment

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

the call already takes the pipeline parameter, right? My untested pet theory was that it was because it was defined twice, once in the YAML and once in the AzDO metadata.

Copy link
Member Author

@benvillalobos benvillalobos May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I want to say that's runtime variable syntax and that syntax doesn't work with compile time variables and blarg 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Bingo bango! #6415


parameters:
- name: OptProfDropName
displayName: Optional OptProfDrop Override
type: string
default: 'default'

variables:
# if OptProfDrop is not set, string '$(OptProfDrop)' will be passed to the build script.
- name: OptProfDrop
value: ''
- name: SourceBranch
value: $(IbcSourceBranchName)
- ${{ if startsWith(variables['Build.SourceBranch'], 'refs/heads/exp/') }}:
# If we're not on a vs* branch, use main as our optprof collection branch
- ${{ if not(startsWith(variables['Build.SourceBranch'], 'refs/heads/vs')) }}:
- name: SourceBranch
value: main
# if OptProfDropName is set as a parameter, set OptProfDrop to the parameter and unset SourceBranch
- ${{ if ne(parameters.OptProfDropName, 'default') }}:
- name: OptProfDrop
value: ${{parameters.OptProfDropName}}
- name: SourceBranch
value: ''
Comment on lines +30 to +34
Copy link
Member

Choose a reason for hiding this comment

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

I know pretty much nothing about yml. Curious, why this can't be done with OptProfDropName alone, i.e. without introducing OptProfDrop. Assuming that '' is the default value of the parameter, can we do the following and omit the rest of the changes except for the refs/heads/exp/ -> !refs/heads/vs tweak?

Suggested change
- ${{ if ne(parameters.OptProfDropName, 'default') }}:
- name: OptProfDrop
value: ${{parameters.OptProfDropName}}
- name: SourceBranch
value: ''
- ${{ if ne(parameters.OptProfDropName, '') }}:
- name: SourceBranch
value: ''

Copy link
Member Author

Choose a reason for hiding this comment

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

The main thing to solve with OptProfDropName is making sure that further in the script p:VisualStudioIbcDrop is passed an empty string if we don't care to override it. Because OptProfDropName is a pipeline parameter now, it can't have an empty value 😞 (maybe it can be set to empty during the script compile time?).

So I have that OptProfDrop variable because:

  1. I know how to work with OptProfDropas a variable at this point (ie. I know I can set it equal to an empty string)
  2. It took ~28 commits to get to this point and I really don't want to keep testing to get a cleaner yml for minimal benefit

Copy link
Member

Choose a reason for hiding this comment

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

Makes perfect sense, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Note that this isn't so much about "YAML" as about "Azure DevOps Pipelines YAML" which is similar to "GitHub Actions YAML" but (maybe?) not exactly the same.

Having been in Ben's position I completely empathize with

It took ~28 commits to get to this point and I really don't want to keep testing to get a cleaner yml for minimal benefit

!

- name: _DotNetArtifactsCategory
value: .NETCore
- name: _DotNetValidationArtifactsCategory
Expand Down Expand Up @@ -100,7 +116,7 @@ stages:
/p:DotNetSymbolServerTokenSymWeb=$(symweb-symbol-server-pat)
/p:TeamName=MSBuild
/p:DotNetPublishUsingPipelines=true
/p:VisualStudioIbcDrop=$(OptProfDropName)
/p:VisualStudioIbcDrop=$(OptProfDrop)
displayName: Build
condition: succeeded()

Expand Down