Enable 2.1 official builds in AzDO #28829

wtgodbe · 2020-12-23T18:05:33Z

Migrates our TeamCity build infrastructure for release/2.1 official builds to Azure DevOps.

Notes:

Pipebuild PR to point the new definition: here
Pipebuild PR I'll need to merge once I'm done testing end-to-end builds: here
Aspnet/Buildtools PR to allow customization of inputs to the SignFiles target: here, here
I removed usage of the aspnet-internal-tools repo entirely, opting instead to use microbuild to do signing at the end of the Windows-SharedFx job (instead of a separate code signing job that checked out and built aspnet-internal-tools)
Green test build ran in isolation: here
Test build with pipebuild inputs, still running: here

Also called out some relevant things in comments on the file changes, and I still have some general cleanup to do, so this is still a WIP.

CC @Pilchie

wtgodbe · 2020-12-23T18:06:39Z

.azure/pipelines/ci.yml

+      inputs:
+        artifactName: artifacts-Windows-Installers
+        downloadPath: $(Build.StagingDirectory)/downloaded_artifacts/
+    - task: CopyFiles@2


This giant block of CopyFile tasks is really ugly, but I couldn't get it to work with fewer - @riarenas @mmitche is there a cleaner way to copy all of the inputs without doing them one-by-one?

wtgodbe · 2020-12-23T18:09:12Z

src/Installers/Windows/AspNetCoreModule-Setup/build/settings/common.props

    <CharacterSet>Unicode</CharacterSet>
-    <OutDir>bin\$(Configuration)\$(PlatformShortname)\</OutDir>
-    <IntDir>obj\$(Configuration)\$(PlatformShortname)\</IntDir>
+    <OutDir Condition=" '$(OutDir)' == '' ">bin\$(Configuration)\$(PlatformShortname)\</OutDir>


@JunTaoLuo @dougbu do you remember why we need this setting here at all? This file is imported very late, and up until this moment outdir points to bin\Release\Installers. Installer signing fails without this change because the files to sign must be in outdir, and this setting changes where outdir points to. My change didn't affect where installers got built to, but I'm wondering if I'm missing something.

Maybe @jkotalik knows something about this? Looks like the change was added in 8356baf but that commit is huge.

wtgodbe · 2020-12-23T18:09:46Z

src/Installers/Windows/Wix.props


  <PropertyGroup>
-    <Version>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).$(AspNetCorePatchVersion).$(BuildNumber)</Version>
+    <!-- Ugly hack - Wix doesn't accept build numbers larger than 65535, and AzDo's BuildIds are larger than that.


This hack kinda sucks, but I couldn't think of anything better - @joeloff @dougbu @JunTaoLuo any ideas?

I think in TC the build number was literally just a counter. If BuildNumber is used for MSIs, then it doesn't really matter that much because Windows Installer only cares about the first 3 parts. For standalone bundles (EXEs), the 4 part version is considered when evaluating upgrades so having an increasing number is desired.

Are the AzDo numbers incremental as well? Unless it hits something large, e.g. 262140 and the modulus wraps, I guess this would be fine.

Yeah, AzDO numbers are also incremental. We feed the version into GuidInputs as of #27079. Since the max for any part of the 4-part version is 65535, we'd run into possible issues as soon as any build number got higher than that - it just might happen sooner in AzDO since builds are more frequent there than in TeamCity. But theoretically, the odds of a collision should be the same as the previous odds of an overflow

And we'll always be incrementing the 3rd part of the 4-part version anyways (each patch)

I think this is probably the best we can do for now but I'm a little concerned about the modulo logic since it can eventually silently cause issues. Instead of modulo maybe just use subtraction and error out if our build numbers are too large so we would be notified and figure something else out at that time? Though I don't know if that will ever happen, I feel it's safer since it'll fail loudly.

I think the only risk is if we produce two versions that are exactly the same, which would happen if 2 builds of the same patch were exactly 34,172 AzDO builds apart (which seems unlikely). I believe 2.1.25.1 would still be considered "higher" than 2.1.24.65535 by WiX - @joeloff does that sound right? That is, it isn't strictly an issue if the buildNumbers aren't always increasing?

This would be an issue if we were producing previews of this branch, because there wouldn't be a guarantee that the versions would always be increasing (e.g. preview1's BuildNumber could be higher than preview2's, and they'd have the same patch number)

Pilchie · 2020-12-23T19:56:53Z

Not going to claim I'm capable of reviewing this, but very happy to see it.

dougbu

Could you explain why the run.* scripts and build.ps1 needed to change❔

Separately, can we now delete .azure/pipelines/pr-validation-temp.ymlvalidation-temp.yml)❔

.azure/pipelines/jobs/default-build.yml

wtgodbe · 2020-12-28T21:37:27Z

Could you explain why the run.* scripts and build.ps1 needed to change?

I was duplicating what we have in extensions where we read PB_ vars from the environment: https://github.com/dotnet/extensions/blob/3a9ef3fa2c14721f1803f7c89d82384438358330/build.ps1#L208-L216. However, this doesn't work for PB_ vars which the pipeline defines as secrets, so I have another commit I'm testing now that goes back to having those vars be passed in from the command line. I'll have to do the same for extensions - it turns out that in that repo, we haven't been restoring from PB_AdditionalRestoreSources or importing PB_PackageVersionProps at all, since those variables are secrets and therefore don't actually get read from the environment.

As for installers\build.ps1, we don't know the full package version at queue time any more (teamCity did have that information at queue time), so we can't pass it in to build.ps1 - so I moved that logic to the .proj file that gets built by the powershell script.

wtgodbe · 2020-12-29T00:46:41Z

This seems to be working now. Here's a build that was kicked by pipebuild: https://dnceng.visualstudio.com/internal/_build/results?buildId=933230&view=results

It pushed assets to this blob location, which looks the same as previous aspnetcore 2.1 blobs:
https://dotnetfeed.blob.core.windows.net/orchestrated-release-2-1/20201223-10/aspnet/index.json

dougbu · 2020-12-29T01:40:03Z

However, this doesn't work for PB_ vars which the pipeline defines as secrets, so I have another commit I'm testing now that goes back to having those vars be passed in from the command line.

This isn't completely necessary. Adding env: settings in the YAML file, copying the secrets, would be more secure. (AzDO sometimes messes up masking secrets when logging command lines.)

wtgodbe · 2020-12-29T17:13:46Z

This isn't completely necessary. Adding env: settings in the YAML file, copying the secrets, would be more secure. (AzDO sometimes messes up masking secrets when logging command lines.)

Oh, I didn't know that was more secure. I'll give that a try now.

wtgodbe · 2020-12-29T17:37:18Z

https://dnceng.visualstudio.com/internal/_build/results?buildId=933842&view=results

wtgodbe · 2020-12-29T21:01:42Z

Looks like that worked: https://dotnetfeed.blob.core.windows.net/orchestrated-release-2-1/20201229-10/aspnet/index.json

JunTaoLuo

First batch of comments

.azure/pipelines/jobs/default-build.yml

JunTaoLuo · 2020-12-30T18:42:55Z

.azure/pipelines/jobs/default-build.yml

-    BuildScriptArgs: ${{ parameters.buildArgs }}
    BuildConfiguration: ${{ parameters.configuration }}
    BuildDirectory: ${{ parameters.buildDirectory }}
-    TeamName: AspNetCore


Did we want to start capturing binlogs all the time? Wouldn't this use up a lot of storage?

I didn't think it was too impactful for 2.1 since we rarely build that branch - but, I could turn it off for public builds like we do in other branches

JunTaoLuo

2nd batch

JunTaoLuo · 2020-12-30T19:19:09Z

src/Installers/Windows/AspNetCoreModule-Setup/build/settings/common.props

    <CharacterSet>Unicode</CharacterSet>
-    <OutDir>bin\$(Configuration)\$(PlatformShortname)\</OutDir>
-    <IntDir>obj\$(Configuration)\$(PlatformShortname)\</IntDir>
+    <OutDir Condition=" '$(OutDir)' == '' ">bin\$(Configuration)\$(PlatformShortname)\</OutDir>


Maybe @jkotalik knows something about this? Looks like the change was added in 8356baf but that commit is huge.

JunTaoLuo · 2020-12-30T19:30:48Z

src/Installers/Windows/Wix.props


  <PropertyGroup>
-    <Version>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).$(AspNetCorePatchVersion).$(BuildNumber)</Version>
+    <!-- Ugly hack - Wix doesn't accept build numbers larger than 65535, and AzDo's BuildIds are larger than that.


I think this is probably the best we can do for now but I'm a little concerned about the modulo logic since it can eventually silently cause issues. Instead of modulo maybe just use subtraction and error out if our build numbers are too large so we would be notified and figure something else out at that time? Though I don't know if that will ever happen, I feel it's safer since it'll fail loudly.

JunTaoLuo · 2020-12-30T19:33:13Z

build/repo.targets

+      <FilesToExcludeFromSigning Remove="@(FilesToExcludeFromSigning)" />
+
+      <!-- Make sure 3rd party binaries get 3rd party certificate -->
+      <CustomFileSignInfo Include="MsgPack.dll" CertificateName="3PartySHA2" />


How was this list generated? I just want to double check but it looks fine.

I manually looked at the contents of all of the .nupkg's/.zips that we're signing, and cross-referenced with what's in master: https://github.com/dotnet/aspnetcore/blob/master/eng/Signing.props

JunTaoLuo

Last batch, so far so good.

.azure/pipelines/ci.yml

Try some internal props

0f0a564

wtgodbe requested review from a team, JunTaoLuo, dougbu and mmitche December 23, 2020 18:05

wtgodbe commented Dec 23, 2020

View reviewed changes

Pass envs to dockerbuild

67dddb1

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 26, 2020

dougbu reviewed Dec 28, 2020

View reviewed changes

.azure/pipelines/jobs/default-build.yml Outdated Show resolved Hide resolved

.azure/pipelines/jobs/default-build.yml Show resolved Hide resolved

wtgodbe force-pushed the wtgodbe/21Pipeline branch from 2894cbe to 0328995 Compare December 29, 2020 00:45

wtgodbe marked this pull request as ready for review December 29, 2020 00:46

wtgodbe added 3 commits December 29, 2020 09:22

Fix job name

546d476

Re-pin

9bca0a5

Set env vars

7844ef6

wtgodbe force-pushed the wtgodbe/21Pipeline branch from 0328995 to 7844ef6 Compare December 30, 2020 17:34

wtgodbe mentioned this pull request Dec 30, 2020

Set PB_ variables in the environment dotnet/extensions#3762

Merged

Set defaults

84ba407

JunTaoLuo reviewed Dec 30, 2020

View reviewed changes

.azure/pipelines/ci.yml Show resolved Hide resolved

.azure/pipelines/ci.yml Show resolved Hide resolved

JunTaoLuo approved these changes Jan 12, 2021

View reviewed changes

wtgodbe merged commit 0b598a9 into release/2.1 Jan 12, 2021

wtgodbe deleted the wtgodbe/21Pipeline branch January 12, 2021 22:06

Enable 2.1 official builds in AzDO #28829

Enable 2.1 official builds in AzDO #28829

Uh oh!

Conversation

wtgodbe commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgodbe Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgodbe Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgodbe Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pilchie commented Dec 23, 2020

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wtgodbe commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtgodbe commented Dec 29, 2020

Uh oh!

dougbu commented Dec 29, 2020

Uh oh!

wtgodbe commented Dec 29, 2020

Uh oh!

wtgodbe commented Dec 29, 2020

Uh oh!

wtgodbe commented Dec 29, 2020

Uh oh!

JunTaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

wtgodbe commented Dec 23, 2020 •

edited

Loading

wtgodbe Dec 23, 2020 •

edited

Loading

wtgodbe Dec 23, 2020 •

edited

Loading

wtgodbe Dec 30, 2020 •

edited

Loading

wtgodbe commented Dec 28, 2020 •

edited

Loading

JunTaoLuo left a comment •

edited

Loading