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

Fix building against GHC 9 / Lens 5 #1694

Merged
merged 2 commits into from Mar 7, 2023
Merged

Fix building against GHC 9 / Lens 5 #1694

merged 2 commits into from Mar 7, 2023

Conversation

apoikos
Copy link
Member

@apoikos apoikos commented Mar 6, 2023

It looks like something in the GHC 9 / Lens 5 combination yields unexpected results when using view on Lens', with GHC complaining that it's passed a Lens' instead of a Getting (which should still be compatible). Commenting out the signatures of Ganeti.Network.poolLens, Ganeti.Network.poolArrayLens and Ganeti.Utils.MultiMap.multiMapL makes Ganeti build again, so I'd keep this ugly hack until we figure out what's actually wrong.

It looks like something in the GHC 9 / Lens 5 combination yields
unexpected results when using view on Lens', with GHC complaining that
it's passed a Lens' instead of a Getting (which should still be
compatible).  Commenting out the signatures of Ganeti.Network.poolLens,
Ganeti.Network.poolArrayLens and Ganeti.Utils.MultiMap.multiMapL makes
Ganeti build again, so I'd keep this ugly hack until we figure out
what's actually wrong.

Signed-off-by: Apollon Oikonomopoulos <apoikos@dmesg.gr>
Copy link
Member

@saschalucas saschalucas left a comment

Choose a reason for hiding this comment

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

Dear @apoikos,

does this PR address compile errors like this one?

src/Ganeti/Utils/MultiMap.hs:103:17: error:
    • Couldn't match type: forall (f :: * -> *).
                           Functor f =>
                           (S.Set v0 -> f (S.Set v0)) -> MultiMap k v0 -> f (MultiMap k v0)
                     with: (S.Set v -> Const (S.Set v) (S.Set v))
                           -> MultiMap k v -> Const (S.Set v) (MultiMap k v)
      Expected: k -> Getting (S.Set v) (MultiMap k v) (S.Set v)
        Actual: k -> Lens' (MultiMap k v0) (S.Set v0)

and what will happen if the type signature is removed? Will Haskell compute the "right one" implicitly? If so, what are the implications of automatic type signatures?

TIA, Sascha.

@apoikos
Copy link
Member Author

apoikos commented Mar 6, 2023

@saschalucas yes, it fixes these errors. In general, type signatures are optional as long as the compiler can figure them out. For most simple functions, the compiler can rely on type inference to guess the function's signature. If the compiler can infer a function's type and a signature has been explicitly defined, then these two must match, otherwise - as in this case - the compilation errors out.

Now, it looks like with have an additional error with the tests, one that I didn't catch in Debian because I'm not running the Haskell tests there during build. I will address this shortly.

Copy link
Member

@saschalucas saschalucas left a comment

Choose a reason for hiding this comment

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

Thanks @apoikos for your explanation. Let's go ahead an use automatic type signatures if that helps to run on newer Haskell stacks.

As you mentioned, github CI hs-tests fails ATM. Fixing this would be nice, too.

Citing the GHC 9.0.1 changelog[1]:

  Breaking change: Template Haskell splices now act as separation points
  between constraint solving passes. It is no longer possible to use an
  instance of a class before a splice and define that instance after a
  splice. For example, this code now reports a missing instance for C
  Bool:

    ```
    class C a where foo :: a
    bar :: Bool
    bar = foo
    $(return [])
    instance C Bool where foo = True
    ```

In other words, code order matters in Template Haskell files, which
breaks the Haskell test build with GHC >= 9.0.1. Fix this by reordering
various Arbitrary instances so that they are defined before being used.

Also the Arbitrary instance for IAllocatorParams was silently acting as
a generic instance for (Container JSValue), providing instances for
DiskParams, HypervisorState and DiskState as well. Change the Arbitrary
instance declaration to Arbitrary (Container JSValue) to better reflect
its generic purpose.

[1] https://downloads.haskell.org/ghc/9.0.1/docs/html/users_guide/9.0.1-notes.html#highlights

Signed-off-by: Apollon Oikonomopoulos <apoikos@dmesg.gr>
@apoikos
Copy link
Member Author

apoikos commented Mar 7, 2023

@saschalucas, I fixed the other build issues as well.

@apoikos apoikos merged commit feab8fa into master Mar 7, 2023
@apoikos apoikos deleted the fix/ghc_9_lens_5 branch March 8, 2023 10:25
@anarcat
Copy link
Contributor

anarcat commented Jul 6, 2023

is this commit planned to be backported into Debian bookworm somehow? worried about upgrading our clusters to bookworm right now...

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.

None yet

3 participants