Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitHub action workflow #1

Merged
merged 12 commits into from
Feb 6, 2021
Merged

Add GitHub action workflow #1

merged 12 commits into from
Feb 6, 2021

Conversation

shonfeder
Copy link
Owner

No description provided.

Comment on lines 28 to 32
- name: Cache opam repository
uses: actions/cache@v2
with:
path: ~/.opam
key: ${{ runner.os }}-${{ hashFiles('*.opam') }}-${{ matrix.ocaml-version }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching the entire switch is not a good idea. By default, setup-ocaml caches everything it can safely cache. (For example: ~/.opam/download-cache)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the warning! Any pointers towards something that explains the risks? It seems like it can significantly speed up the cycle, so I'd be interested to know what the tradeoffs are.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is the action/cache itself problem, but it breaks the cache in some cases. Also, caching the entire switch causes the problem of not pulling the latest version of packages unless you explicitly specify the version or use opam lock, which can sometimes delay the discovery of a problem you want to find.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dune-cache makes the opam install process a few times faster for many projects, so it's a better option to use dune-cache instead of caching the whole. (Perhaps triggering the build again will make the process a little faster in this PR from the next job.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! All clear now. Thanks so much for the tips here!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can check the consistency of the installed opam packages by calling opam upgrade --fixup, but I don't think it's ideal for end-users to do that in all their workflows. So I don't really want to recommend that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anytime!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this makes sense. This is another thing I'll use at my own risk ;)

@smorimoto
Copy link

I was just feeling particularly pedantic tonight, so I posted a few comments, but I think I should definitely have written these in the documentation. Sorry about that...!

@shonfeder
Copy link
Owner Author

Your tips are entirely welcome and greatly appreciated, @smorimoto! It is a help to me, and if helps flag areas that a semi-comprehending user might need documented too, that's even better 🙂

@shonfeder shonfeder merged commit 94d3a82 into master Feb 6, 2021
@shonfeder shonfeder deleted the add-ci branch February 6, 2021 16:49
@smorimoto
Copy link

Thank you for your kind words! My experience here has been quite helpful since I was going to write documentation and explanation next week. (I think it will be released officially after that.)

@shonfeder
Copy link
Owner Author

shonfeder commented Feb 6, 2021

I'm very glad to hear that! I'd hoped I might be of some use by being an early adopter. I'm excited for pending release 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants