Add 'cache: true' option to enable caching #11

omelkonian · 2020-12-18T11:10:06Z

Caches output.cabal-store and ./dist-newstyle
Currently hashFiles is not exposed in the actions/cache API,
hence the use of folder-hash.hashElements

hazelweakly · 2020-12-19T20:22:34Z

I like it so far! If we're going to have a generic cache: true option, it should work for stack as well as cabal out of the box.

For stack, output.stack-root would be what was cached (which I just realized I never properly put inside the action.yml file... sigh).

Then I suppose there's the question of what sort of API to expose to allow people to customize things. Something that will likely be important is doing the minimal amount of work while not being useless about it. (So, for example, running cabal freeze in CI is something I see people do a lot. I don't understand it because it seems to completely defeats the purpose of a freeze file; does this not break repositories where freeze files are checked in?).

I'll comment on the PR with a few more specific thoughts. Thanks for doing a lot of work on this!

hazelweakly · 2020-12-19T20:23:56Z

setup/action.yml

@@ -23,6 +23,9 @@ inputs:
  stack-setup-ghc:
    required: false
    description: 'If specified, enable-stack must be set. Will run stack setup to install the specified GHC'
+  cache:
+    required: false
+    description: 'If specified, automatically caches the cabal-related folders.'


A catch-all cache option should work for all the build tools the action supports, so having the description reflect that would be less confusing.

hazelweakly · 2020-12-19T20:37:12Z

setup/src/setup-haskell.ts

@@ -4,6 +4,8 @@ import {getOpts, getDefaults, Tool} from './opts';
 import {installTool} from './installer';
 import type {OS} from './opts';
 import {exec} from '@actions/exec';
+import * as c from '@actions/cache';
+import {hashElement} from 'folder-hash';


Do you think it would be better to use folder-hash or to copy in the exact code that the github runner uses for its hashFiles function? I'm leaning towards the latter since it would (in theory) preserve hash compatibility if that code ever gets migrated/exposed in actions/toolkit.

Right, that sounds more robust, I have copied hashFiles, but had to change it slightly to take normal arguments.

hazelweakly · 2020-12-19T20:46:24Z

setup/src/setup-haskell.ts

-        if (!opts.stack.enable) await exec('cabal update');
-      });
+    if (opts.cabal.enable) core.info('Loading cache...');
+    await exec('cabal', ['freeze'], {silent: true});


I'm hesitant to want to run cabal freeze by default. Perhaps we should check for a cabal.project.freeze and a stack.yaml.lock by default, but allow users to override this by specifying an option like cache-keys that works exactly as key + restore-keys does on actions/cache now.

Then we can just split on newlines for the cache-keys and keep the standard os-$ghc-$hash-$sha logic as the default. This also has the benefit of being.

We will also probably want to support something like cache-paths in case some people don't want to cache dist-newstyle in addition to cabal-root (or don't want to cache stack-root, or...)

hazelweakly · 2020-12-19T20:48:26Z

setup/src/setup-haskell.ts

+      if (!opts.stack.enable) await exec('cabal update');
+    });
+
+    if (cacheHit && cacheHit != keys[0]) {


is != intentional here? (Just curious since I've got strict equality burned into my brain at this point)

* Support both cabal and stack * Copy `hashFiles` until exposed exposed in the actions/cache API * Saving cache as a post-script * Additional action inputs: `cache-paths` and `cache-keys` * Add `output.stack-root` in `action.yml`

hazelweakly · 2020-12-22T19:37:57Z

setup/src/opts.ts

-  const stackSetupGhc = (inputs['stack-setup-ghc'] || '') !== '';
-  const stackEnable = (inputs['enable-stack'] || '') !== '';
+  const isEnabled = (s: string): boolean => (inputs[s] || '') !== '';
+  const readList = (s: string): string[] => (inputs[s] || '').split('\n');


Should we filter empty lines out here? I think (but haven't verified) that

key: | a b

would result in ['', 'a', 'b'] otherwise.

hazelweakly · 2020-12-22T19:43:16Z

setup/src/setup-haskell.ts

+      core.setOutput('stack-root', stackRoot);
+      if (os === 'win32') core.exportVariable('STACK_ROOT', 'C:\\sr');
+
+      if (opts.cache) {


We will probably want to be fairly careful here. Is there a plan for how the caching behavior should work if the defaults aren't overridden vs if the defaults are overridden?

In particular, I'm not sure it's completely intuitive to have "enabling the cache" result in running commands or otherwise creating files/directories you might not otherwise expect.

I also don't actually know how globbing and then accessing the first item of the array works if there's no stack.yaml.lock file. Does it throw an error? Would the **/stack.*.lock potentially grab way too many files and/or the wrong file(s) if you had git submodules checked out and one of your submodules had a stack.yaml.lock file but your stack project did not?

hazelweakly · 2020-12-22T19:56:11Z

setup/src/setup-haskell.ts

+
+      if (opts.cache) {
+        const matches = await glob
+          .create('**/cabal.*.freeze')


Odd question: How does this work if

A haskell project uses cabal and the project is both an exe and a library.

Said project has a cabal.project.freeze file as per exe recommendations, but names it something different by default so that building with cabal build can be tested to work if the project is used as a library.

What if the project does not want to cache the dist-newstyle directory and only wants to cache the cabal store? It's a reasonable want for some projects. Examples include: for a while, macOS was suffering dylib issues when caching dist-newstyle and so I had to turn off caching dist-newstyle until that was resolved; it can take up a lot of extra space with not much extra benefit, especially if you're using submodules for dependencies instead of "vendoring" them with a cabal.project file (you pay the penalty of having the submodules bloat dist-newstyle as well as not having mtime work well so you frequently have to rebuild all of them anyway even if it is cached; but if you use cabal.project to vendor them, they get built and stored in the cabal.store, making caching dist-newstyle more effective).

dist-newstyle is also not guaranteed to remain the same name forever. So there's that...

larskuhtz · 2021-01-05T14:27:20Z

(So, for example, running cabal freeze in CI is something I see people do a lot. I don't understand it because it seems to completely defeats the purpose of a freeze file; does this not break repositories where freeze files are checked in?).

One reason to run cabal freeze in CI is to include the freeze file in the artifacts for reproducible builds and documentation purposes. So, one builds and tests against the most recent versions from Hackage, but one can roll back to the latest working state if something breaks.

lisanna-dettwyler · 2021-06-12T10:06:49Z

(So, for example, running cabal freeze in CI is something I see people do a lot. I don't understand it because it seems to completely defeats the purpose of a freeze file; does this not break repositories where freeze files are checked in?).

One reason to run cabal freeze in CI is to include the freeze file in the artifacts for reproducible builds and documentation purposes. So, one builds and tests against the most recent versions from Hackage, but one can roll back to the latest working state if something breaks.

@larskuhtz That does sound really useful! It could also allow for freeze files to not be used by default unless the build fails, then it's retried with the last known good one. If it's a pull request it could offer to update the file in the branch too if building from a fresh one works. And it's easy to check if there's a freeze file present already and not mess with it.

brandonchinn178 · 2022-11-09T02:23:11Z

setup/src/setup-haskell.ts

+        keys: opts.cacheKeys || [
+          keys.join('-'),
+          keys.slice(0, 4).join('-') + '-',
+          keys.slice(0, 3).join('-') + '-',
+          keys.slice(0, 2).join('-') + '-'
+        ]


You should be careful about the caching growing unbounded as time goes on. More info here: https://markkarpov.com/post/github-actions-for-haskell-ci.html#comment-6035135802

The cache is designed to grow unbounded because github actions automatically limits it to the last 5G in size. It might theoretically make sense to let people override the key but if they want a very specific set of behavior, they'll probably end up writing their own usage of caching anyway.

omelkonian marked this pull request as draft December 18, 2020 11:11

hazelweakly reviewed Dec 19, 2020

View reviewed changes

omelkonian force-pushed the cache branch from 5587534 to 025c274 Compare December 22, 2020 16:28

omelkonian marked this pull request as ready for review December 22, 2020 16:28

omelkonian force-pushed the cache branch from 025c274 to 7861316 Compare December 22, 2020 16:32

Add 'cabal: true' option to enable caching

2306df7

* Support both cabal and stack * Copy `hashFiles` until exposed exposed in the actions/cache API * Saving cache as a post-script * Additional action inputs: `cache-paths` and `cache-keys` * Add `output.stack-root` in `action.yml`

omelkonian force-pushed the cache branch from 7861316 to 2306df7 Compare December 22, 2020 19:08

hazelweakly reviewed Dec 22, 2020

View reviewed changes

andreasabel mentioned this pull request Sep 21, 2021

Neutral in the choice of build tools #12

Open

georgefst mentioned this pull request Dec 3, 2021

CI improvements georgefst/evdev#21

Closed

gelisam mentioned this pull request Apr 2, 2022

GHC 9.2 support gelisam/hawk#272

Open

brandonchinn178 reviewed Nov 9, 2022

View reviewed changes

andreasabel changed the title ~~Add 'cabal: true' option to enable caching~~ Add 'cache: true' option to enable caching Dec 22, 2022

andreasabel added the re: cache Concerning caching label Dec 22, 2022

andreasabel mentioned this pull request Mar 16, 2023

Add cache action? #216

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'cache: true' option to enable caching #11

Add 'cache: true' option to enable caching #11

omelkonian commented Dec 18, 2020

hazelweakly commented Dec 19, 2020

hazelweakly Dec 19, 2020

hazelweakly Dec 19, 2020

omelkonian Dec 22, 2020

hazelweakly Dec 19, 2020

hazelweakly Dec 19, 2020

hazelweakly Dec 22, 2020

hazelweakly Dec 22, 2020

hazelweakly Dec 22, 2020

larskuhtz commented Jan 5, 2021

lisanna-dettwyler commented Jun 12, 2021

brandonchinn178 Nov 9, 2022

hazelweakly Nov 27, 2022

Add 'cache: true' option to enable caching #11

Are you sure you want to change the base?

Add 'cache: true' option to enable caching #11

Conversation

omelkonian commented Dec 18, 2020

hazelweakly commented Dec 19, 2020

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

larskuhtz commented Jan 5, 2021

lisanna-dettwyler commented Jun 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment