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

Make it build with ghc-9.6 #5286

Merged
merged 13 commits into from
May 18, 2023
Merged

Make it build with ghc-9.6 #5286

merged 13 commits into from
May 18, 2023

Conversation

erikd
Copy link
Contributor

@erikd erikd commented May 3, 2023

No description provided.

@erikd erikd force-pushed the erikd/ghc-9.6 branch 5 times, most recently from 39846a1 to 5c2dab1 Compare May 3, 2023 03:51
cabal.project Outdated Show resolved Hide resolved
cabal.project Show resolved Hide resolved
@@ -70,7 +70,7 @@ executable doc-doctests
, base >=4.9 && <5
, bytestring
, containers
, flat <0.5
, flat ^>=0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful with this, we have to check carefully that upstream hasn't broken anything. Is this change necessary for 9.6?

Copy link
Contributor Author

@erikd erikd May 3, 2023

Choose a reason for hiding this comment

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

The old version does not compile with 9.6 (probably due to base bounds nope, much worse than that).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can get away with allow-newering the base bounds then I think I'd mildly prefer this, since this is a sensitive dependency and this will change the bound for all GHC versions atm.

Copy link
Contributor Author

@erikd erikd May 3, 2023

Choose a reason for hiding this comment

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

Would it not make sense to do one or both of the following?

  • Write tests for flat and submit them upstream.
  • Write tests for flat and keep them in the Plutus repo.

Copy link
Contributor Author

@erikd erikd May 3, 2023

Choose a reason for hiding this comment

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

I tried removing the version bump for flat and added allow-newer: flat:*. The result was:

src/Flat/Decoder/Strict.hs:240:19: error: [GHC-31891]
    • Illegal term-level use of the type constructor or class ‘TA.Array’
    • imported qualified from ‘Data.Text.Array’ at src/Flat/Decoder/Strict.hs:45:1-53
    • Perhaps use variable ‘array’ (line 239)
    • In the first argument of ‘T.Text’, namely ‘(TA.Array array)’
      In the first argument of ‘return’, namely
        ‘(T.Text (TA.Array array) 0 (lengthInBytes `div` 2))’
      In a stmt of a 'do' block:
        return (T.Text (TA.Array array) 0 (lengthInBytes `div` 2))
    |
240 |   return (T.Text (TA.Array array) 0 (lengthInBytes `div` 2))
    |                   ^^^^^^^^

and

src/Flat/Encoder/Prim.hs:243:25: error: [GHC-76037]
    Not in scope: data constructor ‘TA.Array’
    NB: the module ‘Data.Text.Array’ does not export ‘Array’.
    |
243 |     eUTF16F_ !(TI.Text (TA.Array array) w16Off w16Len) s =
    |                         ^^^^^^^^

It supposedly is possible to constrain text < 2.0 but that may cause issues further up the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have tests. Nonetheless I'm cautious :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bezirg could you look into upgrading to flat >= 0.6 on master and check that the script dump test continues to pass?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I am on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test infrastructure this needs got broken a while ago so it might be a little while until we can test this. I am tentatively in favour of merging since I think it's very likely not broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests are finished locally.
Both plutus-master and plutus-master+flat0.6.patch pass the current mainnet script tests

@erikd erikd force-pushed the erikd/ghc-9.6 branch 2 times, most recently from c40ea93 to 7c27753 Compare May 5, 2023 01:16
@angerman angerman linked an issue May 5, 2023 that may be closed by this pull request
@michaelpj
Copy link
Contributor

Now it appears to fail to build with 8.10 and fail to even plan with 9.2

@erikd
Copy link
Contributor Author

erikd commented May 8, 2023

@michaelpj I rebased this against master, then made sure it (other than plutus-tx-plugin) builds here with ghc-8.10, ghc-9.2 and ghc-9.6.

I think the failure to create a build plan was due to the line:

    build-depends: ghc >=9.6 && <9.8

I have been adjusting those bounds for each compiler.

Would be happy to hear any solutions to that issue.

@michaelpj
Copy link
Contributor

I think we discussed this a bit on Slack, but:

  • plutus-tx-plugin depends on ghc, and is finicky.
  • So we're only supporting a single version of GHC and it's non-trivial to switch which one.
  • That version is 9.2, so the bounds on ghc should remain the same.
  • We prevent plutus-tx-plugin and its downstream packages from building on other GHCs with buildable: False conditionals. That prevents cabal from trying to put it in the plan for other GHC versions and getting confused.
  • Those buildable conditionals need to be updated to exclude GHCs newer than 9.2.
  • That should leave us with all those packages building only on 9.2, which is what we want for now.

@erikd
Copy link
Contributor Author

erikd commented May 9, 2023

Ah, this is what I need:

diff --git a/plutus-tx-plugin/plutus-tx-plugin.cabal b/plutus-tx-plugin/plutus-tx-plugin.cabal
index a40d9ccde..6635d44b6 100644
--- a/plutus-tx-plugin/plutus-tx-plugin.cabal
+++ b/plutus-tx-plugin/plutus-tx-plugin.cabal
@@ -49,7 +49,7 @@ flag use-ghc-stub
 library
   import:          lang
 
-  if impl(ghc <9.0)
+  if impl(ghc < 9.2) || impl(ghc >= 9.3)
     buildable: False

and in a number of other places.

@erikd erikd force-pushed the erikd/ghc-9.6 branch 6 times, most recently from b30089f to fd2e4e6 Compare May 10, 2023 07:06
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks pretty much there. We need to update the GHC versions section of CONTRIBUTING, but I can do that when I add 9.6 CI.

cabal.project Outdated
, th-compat:*

constraints:
, microlens >= 0.2.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining these (I would expect us cabal to pick the compatible versions of these libraries automatically) and when we can remove them.

cabal.project Outdated
, dependent-map:*
, hedgehog:*
, protolude:*
, th-compat:*
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming we have upstream issues for everything that's jailbroken by this?

@@ -70,7 +70,7 @@ executable doc-doctests
, base >=4.9 && <5
, bytestring
, containers
, flat <0.5
, flat ^>=0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

The test infrastructure this needs got broken a while ago so it might be a little while until we can test this. I am tentatively in favour of merging since I think it's very likely not broken.

deriving anyclass (NFData)
deriving stock (Functor, Generic)

deriving stock instance (Show tyname, Show name, GShow uni, Everywhere uni Show, Show fun, Show ann, Closed uni)
Copy link
Contributor

Choose a reason for hiding this comment

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

😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it really sucks. But perhaps we could introduce some aliases in order not to write this whole thing our every time. Maybe some generic one that works for every constraint (we'll only need to special-case things like GShow). Not sure if it's worth it, though.

@michaelpj michaelpj added the No Changelog Required Add this to skip the Changelog Check label May 11, 2023
@michaelpj
Copy link
Contributor

I pushed a commit attempting to make the 9.6 CI work, and also merged master to get a haskell.nix update.

@michaelpj
Copy link
Contributor

Sigh: input-output-hk/haskell.nix#1943

@michaelpj
Copy link
Contributor

Okay, it seems to find a build plan now, let's see how far it gets.

@michaelpj
Copy link
Contributor

src/Codec/CBOR/Magic.hs:260:20: error: [GHC-83865]
    • Couldn't match expected type ‘Word64#’ with actual type ‘Word#’
    • In the first argument of ‘W64#’, namely ‘(toWord w#)’
      In the expression: W64# (toWord w#)
      In an equation for ‘w64’: w64 w# = W64# (toWord w#)
    |
260 |     w64 w# = W64# (toWord w#)
    |                    ^^^^^^^^^

maybe well-typed/cborg#311 ?

Looks like we need to poke our WT colleagues or something.

@michaelpj
Copy link
Contributor

Non-darwin fails because of cardano-crypto-class using -Werror :| I think they fixed that on master but we need a release.


src/Cardano/Crypto/Hash/Short.hs:23:35: error: [GHC-58520] [-Wtype-equality-requires-operators, Werror=type-equality-requires-operators]
    The use of ‘~’ without TypeOperators
    will become an error in a future GHC release.
    Suggested fix: Perhaps you intended to use TypeOperators
   |
23 | instance (KnownNat n, CmpNat n 33 ~ 'LT) => HashAlgorithm (Blake2bPrefix n) where
   |                                   ^

@michaelpj
Copy link
Contributor

  • There was a cardano-crypto-class release that should have 9.6 support, bumped our lower bound
  • I've enabled the 9.6 CI on linux only for now, to get around the cborg issue. That'll be a problem in the long run but apparently WT are aware of it and we can probably just ignore it for now.
  • I simplified the allow-newers a lot
  • I added a s-r-p for inline-r, since it actually needs some code changes to build :(

I still seem to be getting some warnings locally, I'll fix those next, and then I think this might be ready.

@michaelpj
Copy link
Contributor

An actual test failure! And an interesting one.

deriving anyclass (NFData, NoThunks)
deriving anyclass (NFData)

-- For some reason the generic instance gives incorrect nothunk errors,
Copy link
Contributor

Choose a reason for hiding this comment

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

@angerman
Copy link
Contributor

force-pushed to re-excite ci.

@erikd erikd force-pushed the erikd/ghc-9.6 branch 3 times, most recently from a7bcba9 to d3d6088 Compare May 17, 2023 07:44
@erikd erikd changed the title WIP Make it build with ghc-9.6 Make it build with ghc-9.6 May 17, 2023
@erikd
Copy link
Contributor Author

erikd commented May 18, 2023

I think all that is missing is #5309

@michaelpj michaelpj merged commit ab4d2cc into master May 18, 2023
@michaelpj michaelpj deleted the erikd/ghc-9.6 branch May 18, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade plutus to build with ghc-9.6
5 participants