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 build and tests with GHC-9.4.3 and 9.2.5 #3364

Closed
wants to merge 16 commits into from
Closed

Fix build and tests with GHC-9.4.3 and 9.2.5 #3364

wants to merge 16 commits into from

Conversation

mouse07410
Copy link

@mouse07410 mouse07410 commented Nov 24, 2022

Fixes build with GHC-9.4.3.
Includes workaround for apparently-broken implicit-hie 0.1.3.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 501 to 502
if impl(ghc >= 9.4)
build-depends: ghc-exactprint >= 1.6.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Author

@mouse07410 mouse07410 Nov 24, 2022

Choose a reason for hiding this comment

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

Ah, a good question. Without this seemingly unnecessary constraint, Cabal somehow manages to pull ghc-exactprint-1.6.0 - presumably because there's a dependency of dependency that's asking for it. And CI tests kept failing.

With this addition, all the relevant tests passed (that is - at least, until I started applying changes to address suggestions ;).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really think this needs to go in the right place. It shouldn't be that hard to figure out. At the very least it needs a clear comment explaining this.

Copy link
Author

@mouse07410 mouse07410 Nov 25, 2022

Choose a reason for hiding this comment

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

I really think this needs to go in the right place

I attempted to find the right place - starting with attempts to put it in plugins/hls-refactor-plugin and ghcide. Failed miserably in both cases - tests kept failing. Please feel free to pitch a hand and locate that right place - I don't know nearly enough to dig deeper. I tried to search/grep for exactprint, and did not come up with anything helpful (besides the above, which did not help either).

If you do know that right place, please share. Or, perhaps, create a new PR - I'm already up to my ears in the bog of CI that I don't understand.

The current workaround was prompted by observation that

$ ghcup compile hls -g master --ghc 9.4.3 -- --index-state=@<current_date> --constraint="ghc-exactprint >= 1.6.1"

works.

How would you want me to explain it? Just copy-paste this post into the .cabal file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not necessary, afaict, this worked for me out of the box:

> cabal update
> cabal build -w ghc-9.4.3 exe:haskell-language-server

Copy link
Author

Choose a reason for hiding this comment

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

Interesting - perhaps, index-state changed since...? Should I update index-state in this PR too?

Also, have you tried just cabal build -w ghc-9.4.3 (to build whatever's there)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you updated the index-state in cabal.project, you locally need to make sure your global index-state is newer than the one in cabal.project (which you added). So just running cabal update in the project or outside of it should do the trick.

Yeah, works just the same. It'd be weird if cabal didn't choose the most recent version of 'ghc-exactprint'.

build-depends: fourmolu ^>= 0.8.2
if impl(ghc >= 9.4)
build-depends: fourmolu ^>= 0.9

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused by exactly why this is done this way (same as #3364 (comment), basically). But if it passes CI I'm fine with it.

I'll revisit all these bounds next time I come along to bump the version.

Copy link
Author

Choose a reason for hiding this comment

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

I'm a bit confused by exactly why this is done this way

Mainly, because I don't know any better. It seemed to be a crude but correct way to get tests pass.

I'll be happy if somebody with more knowledge fixes this the right way.

@mouse07410
Copy link
Author

I don't know what to do with this test failure (GHC-9.4.3, ubuntu-latest):

doc
      local define:                                                                   OK (1.50s)
      local empty doc:                                                                OK (1.52s)
      local single line doc without '\n':                                             FAIL (expected: Completion doc doesn't support ghc9) (1.51s)
        test/exe/Main.hs:2044:
        expected: ["*Defined at line 3, column 1 in this module*\n* * *\ndocdoc\n"]
         but got: ["*Defined at line 3, column 1 in this module*\n"] (expected failure)
      local multi line doc with '\n':                                                 FAIL (expected: Completion doc doesn't support ghc9) (1.51s)
        test/exe/Main.hs:[204](https://github.com/haskell/haskell-language-server/actions/runs/3545320619/jobs/5953373235#step:8:205)4:
        expected: ["*Defined at line 4, column 1 in this module*\n* * *\n abcabc\n"]
         but got: ["*Defined at line 4, column 1 in this module*\n"] (expected failure)
      local multi line doc without '\n':                                              FAIL (expected: Completion doc doesn't support ghc9) (1.51s)

@fendor
Copy link
Collaborator

fendor commented Nov 25, 2022

The testcases you mention are expected failures and require no action.
However,

   addDependentFile
    file-changed
      test:                                                                           FAIL
        Exception: Timed out waiting to receive a message from the server.
        Last message received:
        {
            "jsonrpc": "2.0",
            "method": "window/logMessage",
            "params": {
                "message": "Live bytes: 29.27MB Heap size: 331.35MB",
                "type": 3
            }
        }
        Use -p '/file-changed.test/' to rerun this test only.

might be a genuine bug.

@mouse07410
Copy link
Author

Very likely so. But I don't see how this PR could be related, or how to fix it. Or even whether the fix should belong to this PR.

@fendor
Copy link
Collaborator

fendor commented Nov 25, 2022

Well, if the PR that adds support for GHC 9.4.3, and the test-suite for GHC 9.4.3 breaks, then it is related because GHC changed something, somehow and now some assumption we previously had does not hold anymore. In other words, HLS is not compatible with GHC 9.4.3 out of the box.

A fix may very well belong into this PR, and the fix is likely to be rather minor, but potentially very hard to track down.
It is in theory possible to disable this specific test if you have a good reason, but I think fixing it is the right course of action.

@mouse07410
Copy link
Author

mouse07410 commented Nov 25, 2022

. . . HLS is not compatible with GHC 9.4.3 out of the box.

I call it "limited support". ;-)

It is in theory possible to disable this specific test if you have a good reason, but I think fixing it is the right course of action.

My only/main reason to ignore this test is that I don't have the slightest clue how to go about fixing, let alone actually fixing it.

The change to make HLS build and pass the majority of the tests seems fairly small and simple. The choice appears to be between providing some support for GHC-9.4.3 now, or waiting for an unknown length of time until somebody with enough knowledge figures out what's wrong and makes all the tests pass.

Also, what should we do with GHC-9.2.5? Stackage released LTS for it, but there's no official HLS support yet.

@mouse07410 mouse07410 changed the title Fix build and tests with GHC-9.4.3. Fix build and tests with GHC-9.4.3 and 9.2.5 Nov 25, 2022
@mouse07410
Copy link
Author

Is there any way to make tests continue with other GHC versions rather than cancel, if one fails?

@fendor
Copy link
Collaborator

fendor commented Nov 26, 2022

Is there any way to make tests continue with other GHC versions rather than cancel, if one fails?

No, this is by design to spare some CI resources.

@mouse07410
Copy link
Author

Is there any way to make tests continue with other GHC versions rather than cancel, if one fails?

No, this is by design to spare some CI resources.

I understand that. But here it looks like we need to know which GHC versions do and which do not fail the tests, with the given changes.

Remove forcing `ghc-exactprint >= 1.6.1`, checking if @fendor 's suggestion is correct
@pepeiborra
Copy link
Collaborator

@mouse07410 you could mark the test as an expected failure in ghc 9.4.5. This will allow CI to continue past the error.

@mouse07410
Copy link
Author

@mouse07410 you could mark the test as an expected failure in ghc 9.4.5. This will allow CI to continue past the error.

Please excuse my ignorance. How to do that?

@fendor
Copy link
Collaborator

fendor commented Nov 28, 2022

Use the knownBrokenFor function in front of the test-case, e.g. knownBrokenFor [GHC GHC94] "Not supported on GHC 9.4.3" $ testSession .... Another example at https://github.com/haskell/haskell-language-server/blob/master/ghcide/test/exe/Main.hs#L1989

@mouse07410
Copy link
Author

Use the knownBrokenFor function in front of the test-case, e.g. knownBrokenFor [GHC GHC94] "Not supported on GHC 9.4.3" $ testSession .... Another example at https://github.com/haskell/haskell-language-server/blob/master/ghcide/test/exe/Main.hs#L1989

I see, thanks. Frankly, I think it is beyond my competency to start messing with the code. Perhaps, one of the maintainers would care to tweak these?

@wavewave
Copy link
Contributor

The currently broken master CI makes us hard to work on. (We are currently trying to land some changes for GHC 9.4 such as #3317 but could not make progress related to CI failures)
Is there any blocker for this to be merged and make CI succeed again? Thanks.
cc: @pepeiborra @michaelpj

@pepeiborra
Copy link
Collaborator

The currently broken master CI makes us hard to work on. (We are currently trying to land some changes for GHC 9.4 such as #3317 but could not make progress related to CI failures)
Is there any blocker for this to be merged and make CI succeed again? Thanks.
cc: @pepeiborra @michaelpj

@wavewave what is broken in master CI and what parts of this PR would help unblock you?

@9999years
Copy link
Contributor

9999years commented Dec 13, 2022

@wavewave what is broken in master CI and what parts of this PR would help unblock you?

C'mon, just look at the most recent master commit. It tells you exactly which checks failed:

924b932

You can see that most of the checks in #3317 are failing. We need this PR to fix those checks. I believe this PR will be necessary but perhaps not sufficient to fix those checks.

@pepeiborra
Copy link
Collaborator

C'mon, just look at the most recent master commit. It tells you exactly which checks failed:

924b932

Can you be more explicit? I don't see any checks failing other than the Nix ones.

You can see that most of the checks in #3317 are failing. We need this PR to fix those checks. I believe this PR will be necessary but perhaps not sufficient to fix those checks.

I see that there is a build error in a dependency:

Building library for apply-refact-0.10.0.0..
[508](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:509)
[1 of 5] Compiling Refact.Compat    ( src/Refact/Compat.hs, dist/build/Refact/Compat.o, dist/build/Refact/Compat.dyn_o )
[509](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:510)

[510](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:511)
src/Refact/Compat.hs:183:15: error:
[511](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:512)
Error:     Not in scope: type constructor or class ‘ErrorMessages’
[512](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:513)
    |
[513](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:514)
183 | type Errors = ErrorMessages
[514](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:515)
    |               ^^^^^^^^^^^^^
[515](https://github.com/haskell/haskell-language-server/actions/runs/3681170875/jobs/6227583497#step:4:516)
Error: Process completed with exit code 1.

Can you clarify how this PR fixes that?

@wavewave
Copy link
Contributor

@pepeiborra
I probably do not follow the current status about CI correctly. My question was at more high level, and the CI badge on master commits is shown red cross mark, and the PR title says "it fixes build", so was asking whether we can merge and fix the CI. Thanks for trying to clarify the actual issue further.
The above issue you linked is another upstream issue for apply-refact, which should be solved by updating hackage with GHC-9.4 compatible version (the master of apply-refact is already updated) (note that this couldn't be solved locally here by source-repository-package since the newer version of apply-refact is breaking GHC 9.2 build.

@pepeiborra
Copy link
Collaborator

Superseded by #3396

@pepeiborra pepeiborra closed this Dec 14, 2022
@michaelpj
Copy link
Collaborator

It is certainly a problem that our CI checks are flaky, yeah. But I don't believe that master or anything else is actually broken.

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.

7 participants