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 nix cache workflow #1247

Merged
merged 5 commits into from
Jul 24, 2023
Merged

add nix cache workflow #1247

merged 5 commits into from
Jul 24, 2023

Conversation

rsoeldner
Copy link
Member

This PR adds nix caching of artefacts

Copy link
Contributor

@enobayram enobayram left a comment

Choose a reason for hiding this comment

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

LGTM, but let's see the build pass once we have the credentials set.

.github/workflows/nix.yml Outdated Show resolved Hide resolved
@rsoeldner rsoeldner marked this pull request as ready for review July 7, 2023 17:15
@enobayram enobayram self-requested a review July 10, 2023 06:42
@enobayram
Copy link
Contributor

Since you were able to get the new github action to run without merging the PR, I suggest we extend the build to cache the outputs needed by nix develop as well. But I suggest we employ the mkCheck trick from chainweb-node so that the github action doesn't end up downloading the full devShell closure even when it's not necessary.

Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

I don't have much knowledge of the nixcache stuff, so I defer to Enis as the final say

@emilypi
Copy link
Member

emilypi commented Jul 13, 2023

@enobayram ?

@enobayram
Copy link
Contributor

@emilypi sorry for leaving this PR hanging. As I've mentioned above, it would be nicer to cache the devShell as well before we merge this PR. I'll coordinate with @rsoeldner to push it over the finish line.

@enobayram
Copy link
Contributor

@rsoeldner I've pushed a few commits here in order to add the devShell into the list of packages being built using the mkCheck trick. However, the existing devShell using haskell4nix was not building, so I've switched to the devShell exposed by haskell.nix. Do you know why we used to define the devShell manually using haskell4nix, is there anything wrong with haskell.nix's devShell?

@emilypi
Copy link
Member

emilypi commented Jul 20, 2023

Is there something inherent in devShell that we need, or is it just "nicer"? Afaik, only Greg uses Nix on this project, so it's largely for CI and deployments. It seems like having a nicer shell is diminishing returns at this point, when the broader win is just the caching itself.

@enobayram
Copy link
Contributor

@emilypi

Is there something inherent in devShell that we need, or is it just "nicer"?

Well, we probably want to have some devShell for this flake either way, because that's the environment you get when you run nix develop in this repo. Before my commit that switches to haskell.nix's devShell (which does that by stopping it from getting overriden), we were preparing (more like attempting to prepare, since it didn't work) a shell environment that's independent of the pact cabal project, so it was just a shell with cabal and haskell-language-server in it. haskell.nix's devShell is nicer, because the environment you get from it also contains all the compiled Haskell packages available to cabal, so if you switch to a new branch and run nix develop, you can immediately start building pact (particularly if we have the Nix cache populated with the devShell output). That also means if you start hoogle in that shell, it will contain all the haddocks from all the dependencies.

Afaik, only Greg uses Nix on this project, so it's largely for CI and deployments.

I remember @jwiegley also wanting to have caching for pact's nix develop and I assumed that @rsoeldner is also using Nix on Pact since he opened this PR.

It seems like having a nicer shell is diminishing returns at this point, when the broader win is just the caching itself.

haskell.nix already gives us a devShell for the pact project that we're wrapping with it, so it shouldn't need much maintenance on our part. By adding the devShell to the Nix build GitHub action, we're introducing some CI overhead, but I think most builds shouldn't end up rebuilding the devShell, because the devShell only changes when the Haskell dependency tree changes and I imagine most pact commits must be changing the pact package itself.

All of that said, I'd be happy to remove devShell from this Nix build action if you're not convinced about the case for it.

@emilypi
Copy link
Member

emilypi commented Jul 20, 2023

@imalsogreg @rsoeldner @jwiegley are you guys convinced this is something you want?

@enobayram enobayram mentioned this pull request Jul 20, 2023
8 tasks
@enobayram
Copy link
Contributor

Now that the Nix build action is passing on all platforms, this PR should be ready to merge unless we decide to remove devShell from check.

@jwiegley
Copy link
Contributor

Caching of artefacts is something that I want, and @emilypi I also use this for working on Pact, and I hope that more people will be doing so in future.

@rsoeldner
Copy link
Member Author

rsoeldner commented Jul 22, 2023

@emilypi I'm satisfied with this. Its a big step forward on my current workflow:-)

@enobayram thank you for the entire support, it was an excellent experience!

Copy link
Contributor

@imalsogreg imalsogreg left a comment

Choose a reason for hiding this comment

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

This works on my mac! At least, most of the deps came from chainweb's nix cache. (I did have to compile trifecta and pact, not sure why).

nix build .#

@rsoeldner rsoeldner merged commit 1d7b4e5 into master Jul 24, 2023
7 checks passed
@imalsogreg imalsogreg deleted the rsoeldner/nix-cache branch July 24, 2023 22:01
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.

5 participants