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

Official Builds With Custom OptProf 'Just Work' #6411

Merged
merged 3 commits into from
May 29, 2021

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented May 4, 2021

Fixes #6085

Context

Fixes two issues:

  • If an official build is kicked off that had stale optprof, no optprof, or is a branch that never would have optptrof (think exp/), it would fail. We'd then have to re-run using a specific optprofdropname
  • When overriding the optprofdropname, we need to delete the IbcSourceBranchName. This is one less thing for the kitten (or any other dev launching pipeline builds of specific branches) to worry about!

Changes Made

Our official builds now take OptProfDropName as a pipeline parameter. We should leave the default value (which is default) if we don't plan to use a custom drop. When we do, paste your optprofdrop there and the script should do the rest.

No more manually clearing the IbcSourceBranch pipeline variable!

Testing

See https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4724888&view=results
I launched a pipeline from branch bevillal/ci/quality-of-life.

  • I did not clear the IbcSourceBranchName pipeline variable, but did set the OptProfDrop pipeline parameter and it properly cleared SourceBranch. This fixes the issue with setting OptProfDrop and needing to clear that field manually.

Now see https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4724898&view=results
I launched a build from the same branch and did NOT set optprofdrop. The build correctly pulls optprof data from main.

Notes

This one was very annoying to figure out.

@benvillalobos benvillalobos marked this pull request as ready for review May 5, 2021 00:38
@benvillalobos benvillalobos changed the title Update Official Builds To 'Just Work' Official Builds With Custom OptProf 'Just Work' May 5, 2021
Comment on lines +30 to +34
- ${{ if ne(parameters.OptProfDropName, 'default') }}:
- name: OptProfDrop
value: ${{parameters.OptProfDropName}}
- name: SourceBranch
value: ''
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

!

Comment on lines 10 to 11
# SignType: real
# SkipApplyOptimizationData: false
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

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

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

Comment on lines +30 to +34
- ${{ if ne(parameters.OptProfDropName, 'default') }}:
- name: OptProfDrop
value: ${{parameters.OptProfDropName}}
- name: SourceBranch
value: ''
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

!

@benvillalobos benvillalobos added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 5, 2021
@Forgind Forgind merged commit ea93ae1 into dotnet:main May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-detect when OptProfDropName is set in our yaml and set SourceBranch
4 participants