Use Appveyor matrix for faster PR builds #6945

bergmeister · 2018-05-27T20:09:03Z

PR Summary

Related: #6944

Reduce PR build time by 5 minutes by:

Having Packaging as a separate build job in a matrix -> runs in parallel in PR builds because the Microsoft account is a paid account that allows that (at no additional costs)
Not caching the dotnet folder any more, which is too large and the overhead of zip/unzip/upload/download does not pay off (and fails in forked builds that are on a free AppVeyor account due to the size).
Setting the environment variable DOTNET_SKIP_FIRST_TIME_EXPERIENCE to 1 because the initialization of the dotnet cli cache (1 minute) does not pay off for the whole build.

The total build time of builds on a fork that is on a free AppVeyor account and therefore do not have parallelism, remains the same due to the time saving of redundant caching.

This is just a simple example of what we can easily achieve, we could continue this pattern and split the test runs as per the referenced issue to bring PR builds down to 10 minutes (but this will incur an increase for fork builds on free AppVeyor accounts)

PR Checklist

…that can run in parallel.

… does not exist for the packaging build)

iSazonov · 2018-05-28T04:06:53Z

Seems we don't want publish packages if tests failed.
And

matrix:
  fast_finish: true

doesn't help.

iSazonov · 2018-05-28T05:19:03Z

Restoring build cache take 4 minutes! Unzipping - can we cache unzipped data?

… forks for a fail-fast approach and to get deliverables early) -Remove caching dotnet, which does not seem to pay off due to high zipping and unzipping costs -Set DOTNET_SKIP_FIRST_TIME_EXPERIENCE to 1 to save 1 minute for unnecessary cache initialization since now we always install the sdk from fresh

bergmeister · 2018-05-28T09:11:56Z

@iSazonov fast_finish: true would still be helpful in the sense that running tests don't continue to run if the packaging build is broken. I don't see a reason for not publishing the packages even if some tests fail later on because if it is something that is just a prototype/experiment then I would not want to be blocked on that, in fact, the packages are the first thing that I want from a build.
I do think that trying to cache dotnet does not pay off the zip/unzip/upload/download costs, instead of just installing the required dotnet SDK anyway (which takes only 12 seconds to download, extract and install). Especially in forks, the cache is too big for the free version, therefore that time is wasted because it fails at the end.
I now set an environment variable DOTNET_SKIP_FIRST_TIME_EXPERIENCE to 1 to avoid unnecessary cache initialisation in this case, which saves around one minute

iSazonov · 2018-05-28T11:38:43Z

Make sense set DOTNET_SKIP_FIRST_TIME_EXPERIENCE in second job too?

… it makes the 2nd build slower (it did before but maybe it was a bad run

iSazonov · 2018-05-28T14:35:30Z

Seems we could decouple still two job - for admin tests and xUnit tests. If first we start CI-tagged tests and then packaging, admin-tagged tests and xUnit tests we get minimal time about 14 minutes.

bergmeister · 2018-05-28T15:13:50Z

Yes, that's why this PR is not supposed to close the issue but yes, this is the way to go, I have outlined ideas like that in the issue. This PR is just the first step to show the direction without disadvantages for builds on a fork with a free account. Bear in mind that for every additional build job, people on non-free AppVeyor accounts incur a 2 minute penalty on their fork builds (i.e. their own builds before they have submitted a PR) due to it having to bootstrap the environment.
In general, I'd prefer to drip-feed those improvements one by one into master instead of having one big PR. This way we can also split the work should it get more technical with XUnit/Admin tests.
From my point of view this PR is complete as it leads to an overall improvement on both sides (PR and fork builds) without any disadvantages.

TravisEz13 · 2018-05-31T19:27:04Z

I'm not sure this will speed things up. It may actually slow things down as we often hit our maximum number of jobs. We would need more budget to make this actually work. @SteveL-MSFT What do you think?

bergmeister · 2018-05-31T19:39:07Z

I ran this also on my fork (which has only a concurrency of 1 because I am cheap 😛 ), the time was the same (due to time savings of dumping the dotnet caching and skipping the dotnet initialization). Therefore at least, when job pipelines are available (which probably applies more to me due due to my difference in time-zone and more frequent weekend work), then there will be a speedup. Of course a more efficient split-up would be to do something more expensive than packaging in the 2nd build but this is just the first step to show the future path

TravisEz13 · 2018-05-31T20:04:51Z

appveyor.yml


-# cache version - netcoreapp.2.1-sdk.2.1.300-rc1
 cache:
-  - '%LocalAppData%\Microsoft\dotnet -> appveyor.yml'


This is here for reliability. Please don't remove

Are you saying the download of the SDK sometimes fails? Usually cache makes things less reliable due to leftovers. The reason for removal was the amount of time it was taking and the fact that it often also fails due to its size.

Any download sometimes fails.

Ok, but then shouldn't we rather cache just what we have downloaded instead of caching the appdata folder of dotnet?

TravisEz13 · 2018-05-31T20:07:07Z

Does DOTNET_SKIP_FIRST_TIME_EXPERIENCE mean dotnet cli doesn't download the files? We weren't caching the dotnet cache for perf but for reliability.

bergmeister · 2018-05-31T20:15:39Z

@TravisEz13
DOTNET_SKIP_FIRST_TIME_EXPERIENCE skips the dotnet internal initial cache performance optimisation (that is only useful for long term performance but not when one uses dotnet only a couple of times in a build). It only happens on the very first dotnet command after a fresh install. This process takes about a minute, which is time that we never get back.
For details see e.g. here:
http://donovanbrown.com/post/Stop-wasting-time-during-NET-Core-builds

TravisEz13 · 2018-06-04T20:20:16Z

appveyor.yml

 # version is set in tools\appveyor.psm1 - Invoke-AppVeyorInstall
+environment:
+  POWERSHELL_TELEMETRY_OPTOUT: 1
+  DOTNET_SKIP_FIRST_TIME_EXPERIENCE: 1


can we add this link somewhere near here
http://donovanbrown.com/post/Stop-wasting-time-during-NET-Core-builds

cc @DarqueWarrior 😄

TravisEz13

looks good if we add a comment about what the skip first-time experience variable means.

…o AppVeyorMatrix and add requested comment # Conflicts: # appveyor.yml

…Shell into AppVeyorMatrix # Conflicts: # appveyor.yml

bergmeister · 2018-06-04T20:40:07Z

@TravisEz13 Ok, done. What are your thoughts on the concern about reliability due to removing the CLI cache and instead always just installing it from scratch. PSScriptAnalyzer does this as well and I have not seen a sporadic failure due to that yet.

TravisEz13 · 2018-06-05T00:53:55Z

@bergmeister I think we can try it and see what happens.

daxian-dbw · 2018-06-05T01:00:50Z

appveyor.yml

+matrix:
+  fast_finish: true

-# cache version - netcoreapp.2.1-sdk.2.1.300


Can you please leave this comment in place? It's needed to flush the nuget package cache when we update the sdk or runtime for the build.

sorry, I didn't notice this. Yes, please add this back

Ok, added back

iSazonov · 2018-06-05T03:04:29Z

appveyor.yml

 # version is set in tools\appveyor.psm1 - Invoke-AppVeyorInstall
+environment:
+  POWERSHELL_TELEMETRY_OPTOUT: 1
+  DOTNET_SKIP_FIRST_TIME_EXPERIENCE: 1  # avoid expensive initialization of dotnet cli, see: http://donovanbrown.com/post/Stop-wasting-time-during-NET-Core-builds


I believe it is better to place comments on new line.

iSazonov · 2018-06-05T03:05:17Z

appveyor.yml

-image: Visual Studio 2017
+# Stop all jobs in the matrix if any one them fail
+matrix:
+  fast_finish: true


If we split jobs should we fast stop?

The idea was: if the build itself is broken, then both jobs will fail at the same stage anyway because they both need to build. The only undesired case would be when adding/updating new 3rd party packages and packaging is broken then the tests would abort early. I am happy to revert this if you wish

Thanks!

Closed.

iSazonov · 2018-06-06T03:42:37Z

@bergmeister Thanks for the contribution!

… time by another 5 minutes (#7021) Closes #6944 Following PR #6945, further, reduce the total build time (without any disadvantages) by around 5 minutes by making sure there is a more even split between the 2 build jobs (the 2nd build job used to be much shorter). Therefore this PR moves also the xUnit and Pester-Admin tests into the 2nd build job. If it is a daily/feature test commit, then the feature tests will also happen (only) in the 2nd build job. Because both jobs now run tests, the failfast option was removed. The final question from my side is whether running tests in 2 build jobs is OK for the daily build, which uploads code coverage results? The time to wait for the AppVeyor build results is now 15 +/- 2 minutes, which is a huge improvement to what used to be around 28 minutes before the build matrix was introduced.

bergmeister added 3 commits May 27, 2018 18:49

Use AppVeyor build matrix to produce artefacts in separate build job …

f85eda6

…that can run in parallel.

fix yaml syntax

85e363b

fix ps syntax

c60e4bf

bergmeister requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners May 27, 2018 20:09

bergmeister added 2 commits May 27, 2018 21:55

Make Invoke-AppVeyorCleanCache check if the folder exists (because it…

f998055

… does not exist for the packaging build)

Fix env variable check

5106825

bergmeister added 2 commits May 28, 2018 10:48

Skip dotnet initialization for packaging only

f48c172

remove trailing whitespace

7db9dd0

bergmeister changed the title ~~WIP PoC of using Appveyor matrix for faster builds~~ Use Appveyor matrix for faster PR builds May 28, 2018

Try DOTNET_SKIP_FIRST_TIME_EXPERIENCE for both jobs (again) to see if…

332bcd2

… it makes the 2nd build slower (it did before but maybe it was a bad run

iSazonov assigned TravisEz13 May 28, 2018

TravisEz13 reviewed May 31, 2018

View reviewed changes

TravisEz13 reviewed Jun 4, 2018

View reviewed changes

bergmeister added 3 commits June 4, 2018 21:32

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

57e536f

…o AppVeyorMatrix and add requested comment # Conflicts: # appveyor.yml

Merge branch 'AppVeyorMatrix' of https://github.com/bergmeister/Power…

262ee4e

…Shell into AppVeyorMatrix # Conflicts: # appveyor.yml

fix merge error

46fcd9d

daxian-dbw reviewed Jun 5, 2018

View reviewed changes

daxian-dbw approved these changes Jun 5, 2018

View reviewed changes

iSazonov reviewed Jun 5, 2018

View reviewed changes

bergmeister added 2 commits June 5, 2018 07:36

address pr feedback about comments (different line and cache)

37ca273

fix yaml

42a9809

iSazonov approved these changes Jun 5, 2018

View reviewed changes

TravisEz13 merged commit 2ec3056 into PowerShell:master Jun 5, 2018

daxian-dbw mentioned this pull request Jun 5, 2018

Skip dotnet-cli initialization and stop caching the 'dotnet' folder for Travis CI #7007

Merged

11 tasks

bergmeister mentioned this pull request Jun 7, 2018

AppVeyor build matrix: more efficient build job split to reduce total time by another 5 minutes #7021

Merged

11 tasks

Use Appveyor matrix for faster PR builds #6945

Use Appveyor matrix for faster PR builds #6945

Uh oh!

Conversation

bergmeister commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented May 28, 2018

Uh oh!

bergmeister commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented May 28, 2018

Uh oh!

iSazonov commented May 28, 2018

Uh oh!

bergmeister commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented May 31, 2018

Uh oh!

bergmeister commented May 31, 2018 • 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!

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!

TravisEz13 commented May 31, 2018

Uh oh!

bergmeister commented May 31, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Jun 5, 2018

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!

Choose a reason for hiding this comment

Uh oh!

bergmeister Jun 5, 2018 • 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!

iSazonov commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bergmeister commented May 27, 2018 •

edited

Loading

iSazonov commented May 28, 2018 •

edited

Loading

bergmeister commented May 28, 2018 •

edited

Loading

bergmeister commented May 28, 2018 •

edited

Loading

bergmeister commented May 31, 2018 •

edited

Loading

bergmeister commented May 31, 2018 •

edited

Loading

bergmeister commented Jun 4, 2018 •

edited

Loading

bergmeister Jun 5, 2018 •

edited

Loading