Skip to content

Bump Ormolu and Fourmolu to GHC-9.2-compatible versions #2579

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

Merged
merged 9 commits into from
Jan 13, 2022

Conversation

georgefst
Copy link
Collaborator

@georgefst georgefst commented Jan 10, 2022

Related: #2503.

I haven't tested the Stack or Nix builds.

@georgefst georgefst mentioned this pull request Jan 10, 2022
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

many thanks for updating both plugins
looks good to me, @pepeiborra what do you think about merge to master and then update the ghc-9.2.1 branch?

@michaelpj
Copy link
Collaborator

We could also merge the 9.2 branch (which it seems like we're about to do) and do this right afterwards.

@Ailrun
Copy link
Member

Ailrun commented Jan 11, 2022

Or merging this to ghc 9.2 branch is also an option

@jneira
Copy link
Member

jneira commented Jan 11, 2022

I would go for merging 9.2.1 first, there is lot of work already invested there and it almost has a working executable as is

@georgefst
Copy link
Collaborator Author

georgefst commented Jan 11, 2022

Yeah, I considered all of those options. I did it this way because it looked like #2503 was going to be merged imminently.

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jan 12, 2022
@jneira jneira removed the merge me Label to trigger pull request merge label Jan 13, 2022
@jneira
Copy link
Member

jneira commented Jan 13, 2022

tests are disabled for ghc-9.2.1 but should be executed, no?
removing merge me just in case

@jneira
Copy link
Member

jneira commented Jan 13, 2022

plugins were disabled in the cabal-ghc921.project so they were no being even built, it seems ormolu does not support ghc-lib-parser-9.2.1.*:

 [__4] rejecting: ghc-lib-parser-9.2.1.20220109 (conflict: ormolu =>
ghc-lib-parser>=9.0 && <9.1)

@jneira
Copy link
Member

jneira commented Jan 13, 2022

sorry i am starting to wonder if the goal of the pr was built both with ghc-9.2 or only bump versions to an intermmediate version 🤔

@jneira
Copy link
Member

jneira commented Jan 13, 2022

After some cabal.project tweaks we reach a build error:

Building library for ormolu-0.3.1.0..
[ 1 of 47] Compiling GHC.DynFlags     ( src/GHC/DynFlags.hs, dist/build/GHC/DynFlags.o, dist/build/GHC/DynFlags.dyn_o )

src/GHC/DynFlags.hs:28:13: error:
Error:     Not in scope: ‘platformMini’
    Perhaps you meant one of these:
      ‘platformMisc’ (imported from GHC.Driver.Session),
      data constructor ‘PlatformMisc’ (imported from GHC.Settings),
      ‘platformMinInt’ (imported from GHC.Platform)
   |
28 |             platformMini =
   |             ^^^^^^^^^^^^

src/GHC/DynFlags.hs:29:15: error:
Error:     Not in scope: data constructor ‘PlatformMini’
    Perhaps you meant one of these:
      ‘PlatformMisc’ (imported from GHC.Settings),
      variable ‘sPlatformMisc’ (imported from GHC.Settings),
      variable ‘platformMisc’ (imported from GHC.Driver.Session)
   |
29 |               PlatformMini
   |               ^^^^^^^^^^^^

src/GHC/DynFlags.hs:30:19: error: Not in scope: ‘platformMini_arch’
   |
30 |                 { platformMini_arch = ArchUnknown,
   |                   ^^^^^^^^^^^^^^^^^

src/GHC/DynFlags.hs:31:19: error:
Error:     Not in scope: ‘platformMini_os’
    Perhaps you meant ‘platformMinInt’ (imported from GHC.Platform)
   |
31 |                   platformMini_os = OSUnknown
   |                   ^^^^^^^^^^^^^^^

src/GHC/DynFlags.hs:43:7: error:
Error:     Not in scope: ‘sPlatformConstants’
    Perhaps you meant one of these:
      data constructor ‘PlatformConstants’ (imported from GHC.Platform),
      ‘platformConstants’ (imported from GHC.Platform),
      ‘platform_constants’ (imported from GHC.Settings)
   |
43 |       sPlatformConstants =
   |       ^^^^^^^^^^^^^^^^^^

src/GHC/DynFlags.hs:44:28: error:
Error:     Not in scope: ‘pc_DYNAMIC_BY_DEFAULT’
   |
44 |         PlatformConstants {pc_DYNAMIC_BY_DEFAULT = False, pc_WORD_SIZE = 8},
   |                            ^^^^^^^^^^^^^^^^^^^^^

@georgefst
Copy link
Collaborator Author

There was just a bad merge from master, in a way that was very easy to miss. We didn't want to inherit the change in cabal-ghc921.project that ignored the local hls-ormolu-plugin. And we really don't want allow-newer - all the relevant bounds should already be correct.

Copy link
Collaborator

@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.

Nice work!

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jan 13, 2022
@michaelpj
Copy link
Collaborator

Popping merge-me back on.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

thanks!

@mergify mergify bot merged commit 4ffdf45 into haskell:master Jan 13, 2022
@@ -288,7 +288,7 @@ common floskell
cpp-options: -Dfloskell

common fourmolu
if flag(fourmolu) && (impl(ghc < 9.2.1) || flag(ignore-plugins-ghc-bounds))
if flag(fourmolu) && flag(ignore-plugins-ghc-bounds)
Copy link
Member

Choose a reason for hiding this comment

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

this should have been

if flag(fourmolu)

corrected in #2567 51db994

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants