search

cdepillabout / pretty-simple

pretty-printer for Haskell data types that have a Show instance
https://hackage.haskell.org/package/pretty-simple
BSD 3-Clause "New" or "Revised" License
243 stars 29 forks source link

Only build pretty-simple package in cabal CI. Build web with Nix in CI. #117

Closed cdepillabout closed 2 years ago

cdepillabout commented 2 years ago

This PR just modifies CI to only build web with Nix, and have the cabal CI jobs only build the pretty-simple package, not web.

It also updates the GHC versions used in CI.

This PR should hopefull be able to be directly merged into https://github.com/cdepillabout/pretty-simple/pull/116.

georgefst commented 2 years ago

Awesome, thanks!

georgefst commented 2 years ago

CI failure looks related to https://github.com/ghcjs/jsaddle/pull/133. But cabal.project is pointing to that branch, so I don't know why the looser bound isn't being used.

cdepillabout commented 2 years ago

@georgefst I came up with a hacky workaround of just removing the ./cabal.project file before running cabal build in CI. That way, cabal won't see the web package, and won't take it into account when running its solver.

This is definitely hacky, but hopefully it works.

cdepillabout commented 2 years ago

@georgefst Looks like the Nix build is now failing: https://github.com/cdepillabout/pretty-simple/actions/runs/3123785920/jobs/5066666069

Maybe just a temporary network problem with GitHub?

georgefst commented 2 years ago

@georgefst Looks like the Nix build is now failing: https://github.com/cdepillabout/pretty-simple/actions/runs/3123785920/jobs/5066666069

Maybe just a temporary network problem with GitHub?

Hopefully. I've kicked it to try again.

georgefst commented 2 years ago

Hopefully. I've kicked it to try again.

Hmm, it happened again. No idea what that's about.

cdepillabout commented 2 years ago

@georgefst As far as I can tell, this wasn't caused by this PR, so what about just merging in this PR and then trying to figure out a fix in https://github.com/cdepillabout/pretty-simple/pull/116?

georgefst commented 2 years ago

I think we really need to do something about caching first. It looks to me like CI might have been about to go and build GHCJS. And if we get that from Cachix instead, the current failure (whatever its cause) looks like it might be avoided.

cdepillabout commented 2 years ago

In my opinion, there are really two problems here:

  1. The Nix build doesn't seem to build as-is. We probably don't want a build that can only succeed when you're getting artifacts from a cache, since people who don't have that cache setup will not be able to build. I'm also not able to build locally, because I'm getting the same error as on CI.
  2. Even if we were able to build, without setting up an additional cache, the Nix CI would take a really long time. I think our two options would be to use some sort of GHCJS or Miso Nix cache (if it is available), or setup our own cachix cache. Do you know if there is a GHCJS or Miso Nix cache we could use? It should be relatively easy to setup our Nix action to use an additional cache: https://github.com/marketplace/actions/install-nix#how-do-i-add-a-binary-cache

But I don't think either of these should prevent us from at least merging this PR in, since this PR should still be a strict improvement on https://github.com/cdepillabout/pretty-simple/pull/116

georgefst commented 2 years ago

Do you know if there is a GHCJS or Miso Nix cache we could use?

Yes. I mentioned this in https://github.com/cdepillabout/pretty-simple/pull/117#discussion_r979475784. It's the reason I can build this locally.

The Nix build doesn't seem to build as-is. We probably don't want a build that can only succeed when you're getting artifacts from a cache, since people who don't have that cache setup will not be able to build. I'm also not able to build locally, because I'm getting the same error as on CI.

In principle, I totally agree with you. But the whole GHCJS ecosystem is very fragile (for now, there's exciting progress around it being merged to GHC proper), and I think caches are all-but-necessary. Even though, Nix being what it is, in theory it should be possible to reproduce.

cdepillabout commented 2 years ago

But the whole GHCJS ecosystem is very fragile (for now, there's exciting progress around it being merged to GHC proper), and I think caches are all-but-necessary.

Ah, that's unfortunate, but makes sense.

I added the cache in ceaaa96 so hopefully this should be good to merge, if you're happy with it.

The Nix build now only takes a few minutes.