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

Ghc 9.0.1 support for ghcide #1649

Merged
merged 93 commits into from
Jun 6, 2021

Conversation

anka-213
Copy link
Contributor

@anka-213 anka-213 commented Apr 2, 2021

This builds on top of #1635

Significant progress on #297

It includes a bunch of upstream fixes via source-repository-package in cabal.project:

It also sets allow-newer: * in cabal.project, since that was the fastest way to get things going and the process to figure out the exact bounds is tedious.

I've added a whole bunch of compatibility functions to Development.IDE.GHC.Compat, many of which are probably useful for ghc9 compatibility outside ghcide as well. Maybe they should be extracted?

Currently, some of the ghcide tests fail for ghc-9.0.1 and I haven't fully figured out why yet.

@jneira
Copy link
Member

jneira commented Apr 12, 2021

Still a WIP but just in case but it seems default project formatting style is being changed: see https://github.com/haskell/haskell-language-server/blob/master/CONTRIBUTING.md

Nvm, the formatting is not applied for hiecompat

@anka-213 anka-213 marked this pull request as ready for review April 12, 2021 09:09
@anka-213 anka-213 changed the title WIP: Ghc 9.0.1 support for ghcide Ghc 9.0.1 support for ghcide Apr 12, 2021
@anka-213 anka-213 marked this pull request as draft April 12, 2021 09:09
@anka-213 anka-213 marked this pull request as ready for review April 12, 2021 09:10
@anka-213
Copy link
Contributor Author

There are still a few test cases that fail on ghc-9.0.1 for me, most notably the benchmark tests. I don't really understand how they work.

I'm also considering adding ghc-9.0.1 to the CI, but in that case, we would only want to test ghcide, since that is the only thing that is currently supported.

@anka-213
Copy link
Contributor Author

@jneira This is ghcide and not just hiecompat, so it's probably still relevant.

@anka-213
Copy link
Contributor Author

Does the CI machines have too little disk space?

> /tmp/ghc5331_0/ghc_48.s: hPutBuf: resource exhausted (No space left on device)

@jneira
Copy link
Member

jneira commented Apr 12, 2021

There are still a few test cases that fail on ghc-9.0.1 for me, most notably the benchmark tests. I don't really understand how they work.

Sure @pepeiborra could help us in that side

I'm also considering adding ghc-9.0.1 to the CI, but in that case, we would only want to test ghcide, since that is the only thing that is currently supported.

github job steps can be skipped adding conditions to if: field, using the ghc version

@pepeiborra
Copy link
Collaborator

There are still a few test cases that fail on ghc-9.0.1 for me, most notably the benchmark tests. I don't really understand how they work.

Sure @pepeiborra could help us in that side

I'm also considering adding ghc-9.0.1 to the CI, but in that case, we would only want to test ghcide, since that is the only thing that is currently supported.

github job steps can be skipped adding conditions to if: field, using the ghc version

Ignore local failures in benchmark tests - they can time out fairly easily. As long as they work in CI...

@anka-213
Copy link
Contributor Author

@pepeiborra It seemed like they didn't time out, but rather didn't run at all. Every single benchmark test failed in less than 10 seconds with did not successfully complete 5 repetitions. It took longer time and more succeeded on older ghc versions. But maybe I'm misinterpreting the error message?

@pepeiborra
Copy link
Collaborator

Run them with LSP_TEST_LOG_MESSAGES=1 LSP_TEST_LOG_STDERR=1 to find out what's going on

@anka-213
Copy link
Contributor Author

The CI failure Package index change detected, that's pretty unusual does indeed seem unusual. The only mention I can find of it on the internet is from the source code of stack:
https://github.com/commercialhaskell/pantry/blob/0a04a97561b3f292e66b285b41f75f082294ac67/src/Pantry/Hackage.hs#L242

@anka-213
Copy link
Contributor Author

@pepeiborra Ah, I see, thanks. The problem was that the benchmark suite is trying to build Cabal-3.2.0.0, which doesn't support base-4.15.

@pepeiborra
Copy link
Collaborator

You can change that in ghcide/bench/config.yaml

@anka-213
Copy link
Contributor Author

The tests reloading-th-test-unboxed and th-linking-test-unboxed are unreliable for me. They randomly fail or succeed depending on (I assume) timing.
Sometimes it gets back an empty list of diagnostics first and then the real list, sometimes it gets the correct diagnostics right away.

@anka-213
Copy link
Contributor Author

anka-213 commented May 2, 2021

How do I make the github actions CI run again? Do I need to resolve the merge conflicts first? Currently only the Circle CI seems to run.

The lazy solution to making things compile
I tried to limit the use of CPP to the Compat module as much as possible
by re-exporting the new functions under the old names,
but there is still plenty of pragmas all over the code.

I'm using ghc-api-compat so the imports doesn't need to be changed as much.
@berberman
Copy link
Collaborator

Yeah, you need resolve conflicts to trigger github actions.

"This code is only concerned with extracting argument names, so I don't see how multiplicity would be relevant here"
haskell#1649 (comment)
@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

I may have accidentally created a bunch of "duplicate" jobs in the github CI by pushing many times in quick succession. We can probably cancel the older ones so the newest ones get in front of the queue more quickly.

Circle CI does this automatically, but that doesn't seem to happen on github.

@berberman
Copy link
Collaborator

We can probably cancel the older ones so the newest ones get in front of the queue more quickly.

I've cancelled them

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

I think the build failed on ghc9 because I rebased to master on the PR for LSP. Let me update the reference

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

Future work: (Could be included in this pr though)

  • Update the nix files to use the patched versions of all the upstream packages when building on ghc9.

The sad thing is that now we would have three copies of all those upstream patches, one for each method for building hls.

Edit: Which reminds me that I forgot to update the commit hash in the stack.yaml file

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

Oh, wait, why did stack successfully build it with the old commit hash for lsp, while cabal failed? It must be in a cache, so it didn't notice that the commit had disappeared.

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

Oh, no! Test failures again. I thought I have fixed those, but maybe there's a regression after upgrading a package or something.

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

Here are the four that are currently consistently failing on my local machine:

haskell-language-server
  code actions
    hlint suggestions
      hlint diagnostics ignore hints honouring .hlint.yaml: FAIL (3.59s)
        src/Test/Hls/Util.hs:389:
        Got unexpected diagnostics for Uri {getUri = "file:///Users/anka/projekt/not-mine/haskell/haskell-language-server/test/testdata/hlint/ignore/ApplyRefact.hs"} got [Diagnostic {_range = Range {_start = Position {_line = 2, _character = 4}, _end = Position {_line = 2, _character = 7}}, _severity = Just DsInfo, _code = Just (InR "refact:Redundant bracket"), _source = Just "hlint", _message = "Redundant bracket\nFound:\n  (1)\nWhy not:\n  1\n", _tags = Nothing, _relatedInformation = Nothing},Diagnostic {_range = Range {_start = Position {_line = 4, _character = 0}, _end = Position {_line = 4, _character = 22}}, _severity = Just DsInfo, _code = Just (InR "Use camelCase"), _source = Just "hlint", _message = "Use camelCase\nFound:\n  camel_case = ...\nWhy not:\n  camelCase = ...\n", _tags = Nothing, _relatedInformation = Nothing}]
    redundant import code actions
      remove solitary redundant imports:                    FAIL
        Exception: Timed out waiting to receive a message from the server.
        Last message received:
        {
            "method": "$/progress",
            "params": {
                "token": "5",
                "value": {
                    "kind": "end"
                }
            },
            "jsonrpc": "2.0"
        }
      doesn't touch other imports:                          FAIL
        Exception: Timed out waiting to receive a message from the server.
        Last message received:
        {
            "method": "textDocument/publishDiagnostics",
            "params": {
                "diagnostics": [
                    {
                        "range": {
                            "end": {
                                "character": 0,
                                "line": 1
                            },
                            "start": {
                                "character": 0,
                                "line": 0
                            }
                        },
                        "message": "target â is not a module name or a source file",
                        "severity": 1,
                        "source": "compiler"
                    }
                ],
                "uri": "file:///Users/anka/projekt/not-mine/haskell/haskell-language-server/test/testdata/redundantImportTest/src/MultipleImports.hs"
            },
            "jsonrpc": "2.0"
        }
  window/workDoneProgress
    sends indefinite progress notifications:                FAIL
      Exception: Timed out waiting to receive a message from the server.
      Last message received:
      {
          "method": "$/progress",
          "params": {
              "token": "19",
              "value": {
                  "message": "Finished indexing 1 files",
                  "kind": "end"
              }
          },
          "jsonrpc": "2.0"
      }

4 out of 4 tests failed (61.11s)

It might be something silly like a currently disabled plugin being required for the test. I'll look a bit more at it.

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

Ok, more debugging: It looks like it's one of the upstream changes is what triggers these errors, since they also occur on ghc-8.10.4, even if I enable all plugins, when I run it with --project-file=cabal-ghc9.project

After testing some more: More specifically, it's with the newest version of lsp that it's broken. Removing just that patch and using the version from hackage fixes these four errors (on ghc-8.10.4). Not at all obvious why.

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

A git bisect points me to this PR: haskell/lsp#326

This might also be an answer to the question posed in that PR:

Why is lsp changing the current directory? I could not find any good reason.

@pepeiborra
Copy link
Collaborator

A git bisect points me to this PR: haskell/lsp#326

This might also be an answer to the question posed in that PR:

Why is lsp changing the current directory? I could not find any good reason.

Lsp changing the current directory was causing other problems and had to be removed. It's unfortunate that this is blocking the 9.0 upgrade, but it will need to be fixed on the hls plugins side. It should be a simple matter of making sure all relative paths to config files are made absolute using the workspace root

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

A git bisect points me to this PR: haskell/lsp#326
This might also be an answer to the question posed in that PR:

Why is lsp changing the current directory? I could not find any good reason.

Lsp changing the current directory was causing other problems and had to be removed. It's unfortunate that this is blocking the 9.0 upgrade, but it will need to be fixed on the hls plugins side. It should be a simple matter of making sure all relative paths to config files are made absolute using the workspace root

Ah, I see. It's not really blocking the 9.0 upgrade, it's blocking the lsp package upgrade in general, which the 9.0 upgrade happens to do for 9.0 specifically. The test failures appear on all ghc versions, not just ghc-9 if I use that lsp version. There are 3 paths forward I can think of:

  1. Temporarily revert that change in the branch of lsp that 9.0 uses. This shouldn't cause any regressions, since hls currently still uses the old version without that patch
  2. Ignore those test failures on ghc-9 and mark them as broken on ghc-9 for now. I don't know how serious those problems are.
  3. Try to fix the problems in the plugins. I don't know how to do that or how to figure out what caused the failures.

I tried building hls/master with the current lsp/master to verify if these test failures are caused by a combination of my changes and that patch or if that patch alone is sufficient to cause these test failures. And it did give the same test failures.

That patch was causing test failures, but the issues
should be fixed for real at some point, so that patch can be incluede
@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

I opted for option 1 for now, but if someone knows how to fix the test failures or judges that those test failures are a smaller problem than those caused by lsp changing the current directory, we can change that.

@anka-213
Copy link
Contributor Author

anka-213 commented Jun 5, 2021

I've created issue #1895 for the problems related to haskell/lsp#326 and a draft PR #1894 for testing it.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Good to merge!

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

6 participants