dotnet icon indicating copy to clipboard operation
dotnet copied to clipboard

Improve Perf for Azure Pipelines build

Open Nirmal4G opened this issue 3 years ago • 8 comments

Changes

  • Use 'windows-latest' image.
  • Narrow Test Results path pattern.
  • Do Restore separately from Build step.
  • Don't do previous steps like restore/build.
  • Use 'BuildConfiguration' for 'Release' build.
  • Remove tools that are already present in the image.
  • Use Multi-Level lookup to use already installed runtime(s).

This reduces build times by 20%-40% approx. If we can cache .NET SDKs and NuGet packages, we could further reduce build times by another 30%!

PR Checklist

  • [x] Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • [x] Based off latest main branch of toolkit
  • [x] PR doesn't include merge commits (always rebase on top of our main, if needed)
  • [x] Tested code with current supported SDKs
  • [x] Contains NO breaking changes
  • [x] Code follows all style conventions

Other information

Rebase or Squash merge the PR and set its message to title and description.

Nirmal4G avatar Sep 20 '22 20:09 Nirmal4G

Azure Pipeline references:

Nirmal4G avatar Sep 20 '22 21:09 Nirmal4G

Almost done. Roslyn compiler supports file_header_template. We could use that instead of Update-Headers.ps1 script. Remove the invocation from the pipeline.

See the changes here: f520a84c8e2a6e0df95c90373ae228b6eab6cc74. If you are okay with that, I'll either open a new PR or add it here.

Nirmal4G avatar Sep 21 '22 15:09 Nirmal4G

There's a couple different bugs in Roslyn 4.3, one is that issue with S.C.I you linked, and another is one that causes ForAttributeWithMetadataName to crash at runtime...We'd still need the .NET 7 SDK anyway because I'll be adding a .NET 7 target to the HighPerformance shortly...

Both of them seem to be compiler issues and fix was already provided the next day. I have fixed them in my fork and it also doesn't need .NET 7 SDK. By "shortly" if you mean in next few days then this patch might not be worth merging. But if you see the changes, you might know whether you need it or not.

See the changes here: 39ea1521c20c94edec2d4e057b7e5064d75bbf73. If you're okay with the changes, we can also drop the "Cache .NET SDKs and Tools" commit.

Nirmal4G avatar Sep 22 '22 13:09 Nirmal4G

"fix was already provided the next day."

We can't rely on that unfortunately. Even if the latest VS 17.3.x build is fixed, that still means that anyone using a previous version (including in CIs) would get random crashes from the generators without a clear explanation as to why. It's just safer to skip to 4.4 directly, which is what I've done. People on 17.3 would still just use the previous generator anyway, which still works, it's just a bit less efficient. 17.4 goes GA in November which is when I plan to release 8.1, so that's fine 🙂

Sergio0694 avatar Sep 22 '22 13:09 Sergio0694

@Nirmal4G I love that change, but we need files without the header to also produce an error, is that the case?

NOTE: that should be in a separate PR either way.

Sergio0694 avatar Sep 22 '22 14:09 Sergio0694

we need files without the header to also produce an error, is that the case?

Yes, it does produce error and also suggest a fix for both header less and non-matching scenarios.

Nirmal4G avatar Sep 22 '22 15:09 Nirmal4G

Awesome, then yeah let's do that too (in another PR) 😄

While at it, can we also normalize the whitespace of all files in the solution, given the header will also have them? All files should just be using \n. Right now I'm not sure there's a line terminator set, and I've seen plenty of files using CLRF instead.

Sergio0694 avatar Sep 22 '22 15:09 Sergio0694

While at it, can we also normalize the whitespace of all files in the solution, given the header will also have them?

All files are already normalized. I already did this in my previous PRs in Windows repo and recently updated Git Files in here #304.

All files should just be using \n. Right now, I'm not sure there's a line terminator set, and I've seen plenty of files using CLRF instead.

Git always stores text files with LF (\n) terminator. Git by default checks out and normalizes the line-endings to match each platform's native line-ending (Mac: CR, Unix/Linux: LF, Windows: CRLF).

Nirmal4G avatar Sep 22 '22 15:09 Nirmal4G

@Nirmal4G Getting this CI failure in https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=79569&view=logs&j=4a47a688-b50e-58f6-4d2c-6d1b829fc8e2&t=2bf27cc2-dcab-5f34-f3ae-ecb203c09e81:

C:\hostedtoolcache\windows\dotnet\sdk\7.0.100-rc.1.22431.12\NuGet.targets(396,5): error MSB3202: The project file "D:\a\1\s\tests\CommunityToolkit.Mvvm.Roslyn431.UnitTests\CommunityToolkit.Mvvm.Roslyn431.UnitTests.csproj" was not found. [D:\a\1\s\dotnet Community Toolkit.sln]

Is this a regression caused by this PR?

Branch is https://github.com/CommunityToolkit/dotnet/tree/dev/roslyn-431.

Sergio0694 avatar Oct 03 '22 19:10 Sergio0694

@Sergio0694 The error says Project file not found. I don't think it is related to this PR. Maybe you forgot to commit this CommunityToolkit.Mvvm.Roslyn431.UnitTests renamed file?

Nirmal4G avatar Oct 04 '22 03:10 Nirmal4G