GitHub Actions pr_ci.yml improvements #12589

lahma · 2022-10-08T08:07:36Z

Relates to #12583 , checking how to make PR CI build faster on GH actions side.

NuGet packages probably come faster from local data center than public download interface
Don't install .NET 6 (comes OOB), don't install Node.JS 15 (v16 comes OOB)
Add hard-linking to improve build speeds on Windows, some discussion about the feature (not working inside VS but for CI build seems like a good choice)

Probably not a common thing to see Windows build completing faster than the Linux one?

I'd so much love to have NUKE used in this project, but I guess that's way too far-fetched...

Skrypt · 2022-10-08T16:33:22Z

It is the first time I hear about https://nuke.build/
Looks nice. Maybe a small POC to see the build times and compare first.

.github/workflows/pr_ci.yml

lahma · 2022-10-09T03:00:18Z

It is the first time I hear about nuke.build Looks nice. Maybe a small POC to see the build times and compare first.

I'll create a PR to show the thing. Just yesterday I just wanted to delete all binaries in OC build folders (netcoreapp3.1 and net5.0) leftovers, dotnet clean etc didn't help much so had to do manual search and delete. When using NUKE I would have just said .\build.cmd clean on command line to get it cleaned. Having common tasks and automation in c# really helps, but a PR will follow!

Skrypt · 2022-10-09T16:00:59Z

I generally do git clean -xdf for that matter but sometimes it's a little too much. 😄

hishamco · 2022-10-09T16:29:44Z

I suffered a lot in the paste, but I did what Jasmin suggested with clean everyting which is too much ;)

hishamco · 2022-10-09T16:30:51Z

I think we need to discuss to support NUKE or not, or we can use programmable MSBuild tasks like what I started long time ago

hishamco · 2022-10-09T16:31:54Z

For reference #9247 and #12364

lahma · 2022-10-09T17:05:08Z

NUKE has the advantage that you can debug it too when needed using the IDE.

hishamco · 2022-10-09T18:06:04Z

Sounds good, can we open a discussion for this one, so others can add thier comments and ideas

Piedone · 2022-10-11T20:36:29Z

.github/workflows/pr_ci.yml

+         path: ~/.nuget/packages
+         key: ${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }}
+         restore-keys: |
+            ${{ runner.os }}-nuget-


Could you do some repeated measurements before/after (just with the caching change) so we can see an average for the total workflow runtime (or cache+build)? Two GHA builds can vary wildly between two runs, so any two builds are not really comparable.

We've experimented around for NuGet and NPM caching with our OSOCE solution, which uses OrchardCore.Application.Cms.Targets (so basically every Orchard package) and some, so it should be comparable in terms of NuGet restore requirements. It turned out that restoring the cache under Windows is so slow that it cancels out the savings.

I'll check with some verbose output...

The comment I made at the end shows the results, with no restore it's faster to have the cache in use. Restore is I/O intensive even when nothing needs to be restored (NuGet cache contains items).

If you mean this comment then I only see a single measurement. Sorry for being a pain here, it's just that a +-10% jitter in the performance of GitHub Actions builds is usual, with particularly (un)lucky ones even hitting outside of that, so while this is promising, I'd like to see with multiple runs whether the speedup persists.

.github/workflows/pr_ci.yml

Piedone

Interesting things, thank you! I'll check out the hard link build in our workflows too, I'm curious about the impact.

Piedone · 2022-10-11T20:47:54Z

.github/workflows/pr_ci.yml

-    - uses: actions/setup-node@v2
+    - uses: actions/cache@v3
      with:
-        node-version: "15"
-    - name: Setup .NET 6.0
-      uses: actions/setup-dotnet@v1
-      with:


I'm not sure about these. While it's great to save on having to do these, but we also take a dependency on ubuntu-latest and windows-latest containing the version we want, and if the images are updated, our build can break without us touching it. Perhaps these actions could instead be optimized not to do the installation if it's already available?

Could you reply, please?

Definitely no need to install Node 15 when 16 is LTS, seems that for Windows image every unnecessary step will be slow. Using -latest already makes building unpredictable - it can be Windows 2032 some day.

We definitely shouldn't use v15 of Node, I agree. However, that doesn't necessarily mean we should use the version provided by the image, which is not really something contributors would keep in mind. While using the "latest" images indeed poses a similar issue, I'd say that the chance that a Node or .NET SDK version update brings issues is at least an order of magnitude larger, than that of other image updates, especially OS ones. It's anecdotal, but we even had minor .NET SDK versions breaking the build here (and I've seen that many times with our other apps at Lombiq too), while I can't remember OS updates breaking anything like this.

Similar to setup-dotnet, setup-node
won't install anything if the version already exists locally. So if we target v16 (since the 3 latest LTS versions are pre-installed) then this shouldn't be significantly slower than not doing anything, while retaining that the version we use for the build is known.

.github/workflows/pr_ci.yml

Piedone · 2022-10-12T14:35:55Z

.github/workflows/pr_ci.yml

-    - name: Setup .NET 6.0
-      uses: actions/setup-dotnet@v1
-      with:
-        dotnet-version: 6.0.x
-        include-prerelease: true


v3 of actions/setup-dotnet already includes a feature not to install a given version if it already exists on the VM. So, using it should be enough for us to use the version we want if it exists on the image but still get the speedup: https://github.com/Lombiq/GitHub-Actions/pull/67/files#diff-7978921ed7153ba992ebf38332bdd64fe6f3acb10a452b2978859c5a835ea74bR30 (note the comment above, I initially understood the behavior differently).

Ideally build system would handle this. For example NUKE checks solution's global.json for required SDK version and downloads if needed. Works also outside of GitHub. But probably upgrading actions is the way to go when we can't use the best tools..

Well, we can have something fancy, but in the short term, just pointing this action to v3 will optimize this :).

Just had a run using v3 and it took 1m 29s, so it still comes with a price...

That's supposed to be a lot faster... In practice I've seen <20s for this (vs ~3 minutes for a full install). So, I don't know.

The build you're showing was perhaps a one-time fluke. I again checked our builds, and these restores are ~15s when using a preinstalled SDK. See e.g.:

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3257120788/jobs/5348182484#step:3:1

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3257120788/jobs/5348182424#step:3:1

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3256910188/jobs/5347817613#step:3:1

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3256910188/jobs/5347817671#step:3:1

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3257569220/jobs/5348910654#step:3:1

https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/3257569220/jobs/5348910715#step:3:1

15 seconds is quite long time for no-op?

That's what I initially thought as well, but as someone kindly explained in the mentioned thread it's not a no-op :). It's also checking the latest released 6.0.x version online, and if it's newer than the pre-installed one, installs that instead.

sebastienros · 2022-10-13T18:11:43Z

I'd so much love to have NUKE

Go to hell Marko

Piedone · 2022-10-13T18:18:09Z

Don't merge, see my comments.

lahma · 2022-10-15T05:55:04Z

So because changes requested status I can't get builds to run? Not so convenient...

Piedone

I dont know why the build is not running, I think it should (I certainly don't see any way to let it run). I'll add an approval to see if it helps but please note that still, I don't think this should yet be merged.

Piedone · 2022-10-15T19:09:47Z

Still nothing. I don't know... Please try to push again and let's see if the build kicks off.

lahma · 2022-10-16T06:17:47Z

Sorry, by bad. The YAML had indent problem, this is why tools like NUKE generate the YAML for you...

Piedone · 2024-01-10T01:38:45Z

Is this something you'd like to revisit @lahma or should we close?

lahma force-pushed the add-cache-to-actions branch 2 times, most recently from f3dade7 to 28404a1 Compare October 8, 2022 08:33

lahma changed the title ~~Add cache action for NuGet packages to pr_ci.yml~~ GitHub Actions pr_ci.yml improvements Oct 8, 2022

lahma marked this pull request as ready for review October 8, 2022 09:24

Skrypt reviewed Oct 8, 2022

View reviewed changes

.github/workflows/pr_ci.yml Outdated Show resolved Hide resolved

Piedone reviewed Oct 11, 2022

View reviewed changes

.github/workflows/pr_ci.yml Outdated Show resolved Hide resolved

Piedone requested changes Oct 11, 2022

View reviewed changes

Piedone mentioned this pull request Oct 11, 2022

Test performance of hard link .NET builds and use them if suitable (OSOE-395) Lombiq/GitHub-Actions#64

Closed

Piedone reviewed Oct 11, 2022

View reviewed changes

.github/workflows/pr_ci.yml Outdated Show resolved Hide resolved

Piedone reviewed Oct 12, 2022

View reviewed changes

sebastienros approved these changes Oct 13, 2022

View reviewed changes

lahma force-pushed the add-cache-to-actions branch from 8cc1361 to 4ab6ce4 Compare October 15, 2022 05:51

Piedone approved these changes Oct 15, 2022

View reviewed changes

Piedone added the don't merge label Oct 15, 2022

lahma force-pushed the add-cache-to-actions branch 2 times, most recently from 460a297 to bc5b166 Compare October 16, 2022 06:17

lahma added 15 commits October 20, 2022 07:59

actual lock file usage, disable implicit sources

992f14f

cannot use lock files for cache key as they are not in the repo..

b9dc6cf

better specificity for hashing

f6fded4

setup .net can take over minute even with v3 on windows

a565a02

trigger

b7d0be5

try caching csproj.nuget.g.targets

bb202fc

skip restore on cache hit

94ceec5

friendly name for cache

87e5306

move implicit skip to correct task

70b99b7

cleanup

1d77dfe

workaround for source root problem

9557fa1

test cache invalidation

81fbd26

remove restore verbosity

c5bb2aa

no need for symbols, bring back .NET and Node setup tasks

b2ec3f9

use public project references to reduce disk I/O from dependencies

227b396

lahma force-pushed the add-cache-to-actions branch from dd5f008 to 227b396 Compare October 20, 2022 05:37

freaking YAML

c77ac62

lahma force-pushed the add-cache-to-actions branch from 144e584 to c77ac62 Compare October 20, 2022 05:43

lahma added 5 commits October 20, 2022 08:45

cleanup

369190f

fix syntax

d948438

more specific cache key

9af8521

force evaluate when cache hit fails

07eb291

change to use solution for restore again

f9ac261

lahma force-pushed the add-cache-to-actions branch from 9b8f83c to 7129dfc Compare October 20, 2022 06:23

tweak cached items

5a82c18

lahma force-pushed the add-cache-to-actions branch from 7129dfc to d2619c0 Compare October 20, 2022 06:32

test non-deterministic

233f62a

lahma force-pushed the add-cache-to-actions branch from d2619c0 to 233f62a Compare October 20, 2022 06:43

lahma closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Actions pr_ci.yml improvements #12589

GitHub Actions pr_ci.yml improvements #12589

lahma commented Oct 8, 2022 •

edited

Loading

Skrypt commented Oct 8, 2022

lahma commented Oct 9, 2022

Skrypt commented Oct 9, 2022 •

edited

Loading

hishamco commented Oct 9, 2022

hishamco commented Oct 9, 2022

hishamco commented Oct 9, 2022

lahma commented Oct 9, 2022

hishamco commented Oct 9, 2022

Piedone Oct 11, 2022 •

edited

Loading

lahma Oct 15, 2022

lahma Oct 16, 2022

Piedone Oct 16, 2022

Piedone left a comment

Piedone Oct 11, 2022

Piedone Oct 16, 2022

lahma Oct 16, 2022

Piedone Oct 16, 2022

Piedone Oct 12, 2022

lahma Oct 15, 2022

Piedone Oct 15, 2022

lahma Oct 16, 2022

Piedone Oct 16, 2022

Piedone Oct 16, 2022

lahma Oct 16, 2022

Piedone Oct 16, 2022

sebastienros commented Oct 13, 2022

Piedone commented Oct 13, 2022

lahma commented Oct 15, 2022

Piedone left a comment

Piedone commented Oct 15, 2022

lahma commented Oct 16, 2022

Piedone commented Jan 10, 2024

GitHub Actions pr_ci.yml improvements #12589

GitHub Actions pr_ci.yml improvements #12589

Conversation

lahma commented Oct 8, 2022 • edited Loading

Skrypt commented Oct 8, 2022

lahma commented Oct 9, 2022

Skrypt commented Oct 9, 2022 • edited Loading

hishamco commented Oct 9, 2022

hishamco commented Oct 9, 2022

hishamco commented Oct 9, 2022

lahma commented Oct 9, 2022

hishamco commented Oct 9, 2022

Piedone Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Oct 13, 2022

Piedone commented Oct 13, 2022

lahma commented Oct 15, 2022

Piedone left a comment

Choose a reason for hiding this comment

Piedone commented Oct 15, 2022

lahma commented Oct 16, 2022

Piedone commented Jan 10, 2024

lahma commented Oct 8, 2022 •

edited

Loading

Skrypt commented Oct 9, 2022 •

edited

Loading

Piedone Oct 11, 2022 •

edited

Loading