tweag / HaskellR

The full power of R in Haskell.
https://tweag.github.io/HaskellR
Other
584 stars 47 forks source link

Fix up the build plan for ghcs 8.10 to 9.2 #381

Closed andreabedini closed 1 year ago

andreabedini commented 1 year ago

This fixes up the build plan for GHCs 8.10, 9.0, and 9.2. GHC 9.4 would work for inline-r [1] but it requires revisions in other packages.

Unfortunately my cabal tooling reformats everything but it looks like we haven't lost much.

[1] This works

cabal build -w ghc-9.4 inline-r \
     --allow-newer=ihaskell-inline-r:template-haskell \
     --allow-newer=ihaskell:base \
     --allow-newer=ihaskell:ghc \
     --allow-newer=ghc-parser:ghc
michaelpj commented 1 year ago
     --allow-newer=ihaskell-inline-r:template-haskell \

Couldn't you include the relaxed bound in this PR, since ihaskell-inline-r is defined here?

andreabedini commented 1 year ago

Couldn't you include the relaxed bound in this PR, since ihaskell-inline-r is defined here?

Oh, wow, this is tricky.

tl;dr: I need --allow-newer=ihaskell-inline-r:template-haskell to come up with a plan, which will work for inline-r and fail for ihaskell-inline-r.

Let me explain.

ihaskell-inline-r does not build with ghc-9.4 so I don't think relaxing its bounds on template-haskell is warranted.

❯ cabal build -w ghc-9.4 ihaskell-inline-r --allow-newer=ihaskell-inline-r:template-haskell --allow-newer=ihaskell:base --allow-newer=ihaskell:ghc --allow-newer=ghc-parser:ghc 
Build profile: -w ghc-9.4.2 -O1
In order, the following will be built (use -v for more details):
 - ghc-parser-0.2.4.0 (lib) (requires build)
 - ihaskell-0.10.3.0 (lib) (requires build)
 - ihaskell-blaze-0.3.0.1 (lib) (requires build)
 - ihaskell-inline-r-0.1.1.1 (lib) (first run)
Starting     ghc-parser-0.2.4.0 (lib)
Building     ghc-parser-0.2.4.0 (lib)

Failed to build ghc-parser-0.2.4.0.
Build log (
/home/andrea/.cabal/logs/ghc-9.4.2/ghc-parser-0.2.4.0-07a4c82b051a3203c0227f37803fa11c05aa8d12dc918e449e981e4fe3e409c1.log
):
Configuring library for ghc-parser-0.2.4.0..
Preprocessing library for ghc-parser-0.2.4.0..
Error: cabal-3.8.1.0: can't find source for Language/Haskell/GHC/Parser in .,
dist/build/autogen, dist/build/global-autogen

Error: cabal: Failed to build ghc-parser-0.2.4.0 (which is required by
ihaskell-inline-r-0.1.1.1). See the build log above for details.

As the above shows, it's ghc-parser (depedency of ihaskell and of ihaskell-blaze, both depedencies of ihaskell-inline-r) that does not compile with ghc-9.4 (for whatever reason, I have yet to dedicate any attention to the failure itself).

But building inline-r on ghc-9.4 still works! Indeed inline-r does not depend on ihaskell-inline-r, and the reason I needed --allow-newer=ihaskell-inline-r:template-haskell is because a cabal project has a single install plan for all included packages.

Now, we could relax the bounds on template-haskell for ihaskell-inline-r if we could argue the breaking changes in template-haskell-2.19.0.0 do not affect our code. But 1) I don't have a quick way to do that 2) users are going to be stuck on ghc-parser anyway.

mboes commented 1 year ago

I'm fine with skipping testing with GHC 9.4 until it lands either in a Stackage nightly or better yet in an LTS. More generally, this PR proposes a matrix build workflow using cabal-install in addition to the existing workflow using Stack. I'd rather avoid using two build tools in CI when testing older compilers can just as easily be done with the current build tool.

The current CI workflow is fully reproducible, whereas this alternative one is not. It could be made to be reproducible with a freeze file, but cabal-install makes it awkward to have several freeze files for several compilers, which is what we need here. The project currently only aims to stay current with nightly and recent LTS snapshots. Keeping to that avoids the busywork visible in this PR to repeatedly futz with dependency version bounds when testing with multiple compiler versions.

See 31ef684432e8865e307cecdb42ebdd5ecda295cf for what testing GHC 8.10 through 9.2 by targeting a few LTS's and the nightly looks like. Small change to the existing workflow file.

andreabedini commented 1 year ago

The current CI workflow is fully reproducible, whereas this alternative one is not.

I have added index-state to cabal.project for reproducibility (at least at "hackage level").

I have also added explicit testing with the package sets from LTS-18, LTS-19 and nightly-2022-01-23.

Please note that reproducibility at the project level helps the developers but not the consumers. Those who consume these package cannot use the package developers reproducible environment (unless nix is involved).

The project currently only aims to stay current with nightly and recent LTS snapshots. Keeping to that avoids the busywork visible in this PR to repeatedly futz with dependency version bounds when testing with multiple compiler versions.

I volunteer to maintain with the version bounds so that the packages as published on Hackage are buildable.

mboes commented 1 year ago

I merged #387 before noticing you had updated this PR. Nonetheless, I'd be happier with sticking with Stack for now, even if we could well switch to cabal-install at a future date. Either way, it's a goal of the project to stay in nightly and ship as part of as many LTS's as possible. This PR tests against LTS's using stackage.org/lts/cabal.config (clever!), but that's an unofficial feature of stackage.org (see https://github.com/fpco/stackage-server/issues/232).

I volunteer to maintain with the version bounds so that the packages as published on Hackage are buildable.

Sounds good to me. Could you therefore refocus this PR on just the version bounds?

andreabedini commented 1 year ago

Sounds good to me. Could you therefore refocus this PR on just the version bounds?

:+1: I was going to suggest that myself. I'll open a new PR so I can keep this branch in my fork as it is.

This PR tests against LTS's using stackage.org/lts/cabal.config (clever!), but that's an unofficial feature of stackage.org (see fpco/stackage-server#232).

Yeah, I know, although I think that issue is a bit outdated (maybe the discussion happened before index-state?). See https://github.com/fpco/stackage-server/pull/313 for a fresh assessment (or at least from my POV).