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

Properly condition onelocbuild template #6561

Merged
merged 7 commits into from
Jun 16, 2021

Conversation

benvillalobos
Copy link
Member

Context

Migrates us "fully" onto the OneLocBuild.yml template. We were missing a condition and parameter in the template import.

Thought: This doesn't need to be backported to <=vs16.11 right? Those builds should be having the onelocbuild template imported regardless. On second thought, they may need the MirrorRepo parameter.

Changes Made

Add MirrorRepo parameter
Add condition on importing the template.

Testing

Will create exp/ branch to test this.

Notes

.vsts-dotnet.yml Outdated
CreatePr: false
LclSource: lclFilesfromPackage
LclPackageId: 'LCL-JUNO-PROD-MSBUILD'
- {{ if eq(variables['Build.SourceBranch'], 'refs/heads/main') }}:
Copy link
Member

Choose a reason for hiding this comment

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

Don't we almost always want this in our release branches? The loc handback is almost always after we've branched for release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Loc handback occurs continuously now. When you branch for release, you can change this to continue receiving loc handback from those branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I wasn't considering future release branches. We can modify this to include an or(XXX, startsWith(refs/heads/vs))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, would love to filter on the pattern. Don't love the idea of adding a step to branching for release, which is currently git-only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't love the idea of adding a step to branching for release

I also prefer just adding the or instead of having us manually push stuff each release. Will add the condition

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recommend this as it will cause problems with OneLocBuild, which expects to receive artifacts from only one branch. There's a whole official process for branching for release which you can read here.

If you want to run two branches at once, you'll need two separate package repositories on the loc side (the LCL-JUNO bit). You'll need to file a ticket with the loc team about doing that.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, but how would this fix that? You're saying that we need to change the build definition in both the release branch and the main branch for every release?

Copy link
Member

Choose a reason for hiding this comment

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

Talked offline. The answer is evidently "yes, this is what we must do".

.vsts-dotnet.yml Outdated Show resolved Hide resolved
.vsts-dotnet.yml Outdated Show resolved Hide resolved
.vsts-dotnet.yml Outdated Show resolved Hide resolved
@benvillalobos benvillalobos force-pushed the onelocbuild-update branch 3 times, most recently from 5bfd681 to 20aa42b Compare June 14, 2021 21:36
@benvillalobos
Copy link
Member Author

@rainersigwald
Copy link
Member

Is it possible to test the release-branch condition as well?

@rainersigwald
Copy link
Member

Oh, and: this is going to main. But we need it for 16.11, no?

@benvillalobos
Copy link
Member Author

Is it possible to test the release-branch condition as well?

https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4870284&view=results

Oh, and: this is going to main. But we need it for 16.11, no?

Yeah I believe so. I was wondering if we should target 16.10 but it isn't LTS so I assume we pass on that even though it has the template import.

Looks like there's a onelocbuild failure with our internal repo (/cc: @jonfortescue): https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4870284&view=logs&j=6aa31087-d1b8-5ee1-af29-f0fe63353381&t=f5f0ecb8-e09a-5b29-99cb-311e048f2c1c

OneLocBuildClient.exe Error: 0 : System.ArgumentException: Invalid repo name: 'DotNet-msbuild-Trusted'.

@benvillalobos benvillalobos changed the base branch from main to vs16.11 June 14, 2021 22:51
…ecause we have an internal mirror. Note that MirrorRepo is somewhat confusing. The GH repo is set as MirrorRepo because, in the context of an official build, the build runs FROM the internal mirror and GH is the mirror.
@benvillalobos
Copy link
Member Author

Rebased onto vs16.11. This is currently blocked on the linked failing official build (see above).

@jonfortescue
Copy link
Contributor

@benvillalobos investigating.

@jonfortescue
Copy link
Contributor

@benvillalobos looks like msbuild hasn't taken arcade latest which is where these changes to the template were introduced. This PR will need to be merged and put onto this branch before these changes work.

@rainersigwald
Copy link
Member

We don't plan to ever take that into this branch. It's a 5.x servicing branch. Is there an equivalent Arcade 5 update?

@jonfortescue
Copy link
Contributor

@rainersigwald ah, right, I forgot about that. There will be one soon!

@jonfortescue
Copy link
Contributor

jonfortescue commented Jun 16, 2021

@rainersigwald @benvillalobos next release/5.0 update from arcade will contain the necessary changes!

@rainersigwald
Copy link
Member

I merged the Arcade update and changed the condition to be current-release-branch only. A build on not-that-branch shows the OneLocBuild step missing: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4878158

@jonfortescue can you give this a once-over again please?

@rainersigwald rainersigwald added this to the MSBuild 16.11 milestone Jun 16, 2021
@rainersigwald rainersigwald added Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. Localization labels Jun 16, 2021
@jonfortescue
Copy link
Contributor

@rainersigwald @benvillalobos this all looks good to me!

@rainersigwald
Copy link
Member

Build passed, bypassing policy due to reporting outage.

https://dev.azure.com/dnceng/public/_build/results?buildId=1190370&view=results

@rainersigwald rainersigwald merged commit a2ac856 into dotnet:vs16.11 Jun 16, 2021
@rainersigwald
Copy link
Member

Ok, so the loc portion of the build passed: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=4878749&view=logs&j=6aa31087-d1b8-5ee1-af29-f0fe63353381&t=f5f0ecb8-e09a-5b29-99cb-311e048f2c1c

But the PR generated #6571 went to main rather than vs16.11. And it appears to actually be based off of main. @jonfortescue do we need to specify mirrorBranch?

@jonfortescue
Copy link
Contributor

shoot. I should have caught that, I'm sorry. Yes, you'll need to specify mirrorBranch.

@rainersigwald
Copy link
Member

#6572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. Localization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants