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

9.6 support for HLS #3480

Merged
merged 9 commits into from
Mar 22, 2023
Merged

9.6 support for HLS #3480

merged 9 commits into from
Mar 22, 2023

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Feb 8, 2023

This is a WIP branch for 9.6 support. It currently compiles and works with GHC 9.6.1-alpha2

TODO

  • make downstream pull requests for hiedb and hie-bios
  • Fix compilation with older GHCs
  • Clean up CPP
  • Fix cabal.project
  • Use the fat interface file support in GHC 9.6 instead of relying on HLS core files (to be done in seperate PR)

@mpickering
Copy link
Contributor

I have tested the current state of this branch to load ghc library into HLS when using 9.6 as the boot compiler and things seem to be working quite smoothly.

@ozkutuk
Copy link
Collaborator

ozkutuk commented Feb 16, 2023

hls-explicit-record-fields-plugin LGTM

Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

LGTM as for (disabled) Splice Plugin.

Just curious, what prevented Splice Plugin from compat with GHC 9.6?

cabal.project Outdated
fourmolu -fixity-th
fourmolu -fixity-th,
setup.happy == 1.20.1.1,
happy == 1.20.1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just build this branch, and found that hiedb's use of dfs (in HieDb.Query) is incompatible with algebraic-graphs-0.7.

I successfully compiled this branch with the following command:

ghcup compile hls -g wip/9.6 --git-describe-version --ghc 9.6.0.20230210 --cabal-update -- --allow-newer -j4 --constraint="algebraic-graphs==0.6.1"

Perhaps, it might be good to add algebraic-graphs == 0.6.1 here?

cabal.project Outdated Show resolved Hide resolved
cabal.project Outdated Show resolved Hide resolved
Copy link
Collaborator

@konn konn left a comment

Choose a reason for hiding this comment

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

LGTM as for Splice Plugin.

Sorry for messing up reviewing state by reviewing too early, and thank you for the great work!

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.

Looks fine, I haven't reviewed all the CPP but I trust you.

cabal.project Outdated Show resolved Hide resolved
sudo mkdir -p /usr/local/.ghcup
sudo chown -R $USER /usr/local/.ghcup
shell: bash

- uses: haskell/actions/setup@v2.3.3
- uses: FinleyMcIlwaine/actions/setup@4a3a6eafc25bf77dff3e127437ac51e63b5d812b
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it would be nice to not have to use this. Is there an upstream PR?

# set to the "last merge commit on the GITHUB_REF branch" (see
# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request).
# But we want to check out the latest commit on the branch whether or
# not it is a merge commit, so this is how we do that.
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 confused by this. Is this pervasively wrong with everyone using these workflows? Do you have a reference?

.github/workflows/test.yml Outdated Show resolved Hide resolved

-- mfsolve has duplicate instances in its test suite
-- See: https://github.com/kuribas/mfsolve/issues/8
package mfsolve
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 confused by this. tests: True shouldn't mean that cabal builds the tests for library dependencies. Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure, maybe a cabal bug?

cabal.project Outdated Show resolved Hide resolved
@@ -66,7 +115,9 @@ constraints:
ghc-check -ghc-check-use-package-abis,
ghc-lib-parser-ex -auto,
stylish-haskell +ghc-lib,
fourmolu -fixity-th
fourmolu -fixity-th,
setup.happy == 1.20.1.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 aren't these constraints in the conditional block? comments? can we put the 9.4 constraints in a conditional block also?

cabal.project Outdated Show resolved Hide resolved
@@ -1242,6 +1248,7 @@ pluginSimpleTests =

-- Error: cabal: Failed to build ghc-typelits-natnormalise-0.7.7 (which is
-- required by plugin-1.0.0). See the build log above for details.
ignoreFor (BrokenForGHC [GHC96]) "fragile, frequently times out" $
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its fragile for all ghc versions, I think due to #3423

@@ -2116,7 +2123,7 @@ highlightTests = testGroup "highlight"
, DocumentHighlight (R 6 10 6 13) (Just HkRead)
, DocumentHighlight (R 7 12 7 15) (Just HkRead)
]
, knownBrokenForGhcVersions [GHC90, GHC92, GHC94] "Ghc9 highlights the constructor and not just this field" $
, knownBrokenForGhcVersions [GHC90, GHC92, GHC94, GHC96] "Ghc9 highlights the constructor and not just this field" $
Copy link
Collaborator

Choose a reason for hiding this comment

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

man, there are lots of broken tests in here :(

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Pretty much impossible to review due to hundreds of little non-trivial changes that each would take me hours to properly review.

I think we could have better discipline to factor out CPP into the GHC.Core module, but it also seems slightly pointless to do that since we do have CPP statements everywhere anyway...

Thus, looks good to me, thanks for the great work! Just marked a couple of nitpicks.

test/functional/FunctionalCodeAction.hs Show resolved Hide resolved
test/functional/FunctionalCodeAction.hs Show resolved Hide resolved
Comment on lines +2 to +3

allow-newer: base
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment that we just need this until async support ghc 9.6

@sidkshatriya
Copy link

sidkshatriya commented Mar 14, 2023

I get a puzzling error [ cabal ] fatal: Could not parse object '721bd0ce7392b8eff8d7643cf132a6448466fd52'. when I try to build HLS using ghcup compile hls on the wip/9.6 branch.

Details below:

$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 9.2.7
$ cabal --version
cabal-install version 3.8.1.0
compiled using version 3.8.1.0 of the Cabal library 
$ ghcup compile hls -g wip/9.6 --ghc 9.6.1 --git-describe-version
...
[ Info  ] Fetching git repo https://github.com/haskell/haskell-language-server.git at ref wip/9.6 (this may take a while)
[ Info  ] Examining git ref wip/9.6
[ ...   ]   HLS version (from cabal file): 1.9.1.0
[ ...   ]   'git describe' output: 1.9.0.0-81-g589871da
[ ...   ]   commit hash: 589871dad1adef4376ce2057c3f9961e6244f4e2
[ Info  ] Building HLS 1.9.0.0-81-g589871da for GHC version 9.6.1
[ cabal ] fatal: Could not parse object '721bd0ce7392b8eff8d7643cf132a6448466fd52'.





[ Error ] [GHCup-08841] BuildFailed failed in dir /home/abcxyz/.ghcup/tmp/ghcup-f7e12ac2ec321a74:
...

What does this error mean and how can I resolve it?

P.S. I get the same error when I switch my cabal and ghc.

$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 9.6.1
$ cabal --version
cabal-install version 3.10.1.0
compiled using version 3.10.1.0 of the Cabal library

@guibou
Copy link
Collaborator

guibou commented Mar 17, 2023

I pushed a few changes related to nix. I'm now able to build haskell-language-server with GHC 9.6 and nix, with only a few disabled plugins (mostly because retrie does not build).

I had to change a few of the haskell code to take into account changes in hie-bios, see https://github.com/haskell/hie-bios/pull/375/files#diff-58ae540d147057c94682a01b61de564caf1eb641fac8d2a3f29ea1f29d673ac4, please tell me if I'm breaking your cabal only build because it uses a different version of hie-bios. It introduces logging to a few of the function, for now (in order to build), I've just set an empty logger, this must be fixed.

@sidkshatriya
Copy link

sidkshatriya commented Mar 17, 2023

@wz1000 Regarding the error reported above #3480 (comment) , I tried narrowing it down further.

The issue seems to be in cabal.project in wip/9.6. Specifically:

if impl(ghc >= 9.5)
  source-repository-package
    type:git
    location: https://github.com/wz1000/HieDb
    tag: 721bd0ce7392b8eff8d7643cf132a6448466fd52

(This also has your git-blame)

However 721bd0ce7392b8eff8d7643cf132a6448466fd52 does not exist as as revision on the HieDb github repo. Have you by any chance not pushed it up from your local machine?

@guibou
Copy link
Collaborator

guibou commented Mar 17, 2023

I pushed additionnal changes which enables all the remaining plugin. Mostly it fixed retrie (with a custom patch of retrie, see facebookincubator/retrie#54 (comment)), and propagated the fix (to add TransformT everywhere lift was used for TransformT).

@wz1000
Copy link
Collaborator Author

wz1000 commented Mar 22, 2023

@guibou I dropped a couple of your commits due to merge conflicts which I had no way of testing/resolving, I suspect they might need to be reworked any way.

@wz1000 wz1000 force-pushed the wip/9.6 branch 2 times, most recently from 6377cd0 to c54d7ea Compare March 22, 2023 09:52
Fixes

hls-refactor-plugin 9.6 support

hls-gadt-plugin

Fix 9.4 build

Fixes

hls-gadt-plugin fixes

WIP 9.6 patches

fixes

fixes

fixes

fixes

fixes

Fixes and add CI

CI

CI fixes

patch haskell/actions for haskell/ghcup-hs#783

CI fixes

CI fixes

CI fixes

CI

CI

CI

CI

CI

Fix build on 9.0

Fix build on 9.0

hls-splice-plugin 9.6 compat

fixes

fixes

fixes

fixes

Fix benchmark build errors

9.2.5 and 8.10.7 had build errors when running benchmarks due to `mfsolve`
test suite having duplicate instances, so stop building tests for
mfsolve (see: kuribas/mfsolve#8). Also,
`http2-4.0.0` has a parse error due to a misplaced haddock comment that
causes build failure with `-haddock`. It is fixed in the latest commit
of the source repo, so use that in the `cabal.project` for now.

Checkout correct commit on `pull_request` in CI

By default, the `pull_request` event has a `GITHUB_SHA` env variable set to the
"last merge commit on the GITHUB_REF branch"
(see https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request).
But we want to check out the latest commit on the branch whether or not it is a
merge commit. This commit changes the CI actions to do just that.

fixes

Use head.hackage for 9.4

Only use head.hackage for 9.5 and up

Reverts the change that caused head.hackage to be used for 9.4 as
well

Reintroduce source-repo-package for ekg-json

Fix refactor plugin tests

Fix missing constraint detection in refactor plugin

ghc 9.6+ allow newer unordered-containers:template-haskell

Some refactor tests no longer broken for 9.2

Fix simple-multi-test on 9.6

Mark simple-plugin as broken on 9.6

func-test fixes

Disable unsupported plugins on 9.6

Eval plugin fixes

Eval plugin test fixes, debug output in CI script

Restore 'working' setup/actions

WIP Fix GHC prerelease windows install

Fix eval plugin T11

fixes

Eval plugin fixes

Fix splice plugin test

Mark `simple plugin` ghcide test broken on 9.6

fixes

fixes

Use GHC 9.6-rc1 in CI

Try using 9.6.1 for CI
@wz1000 wz1000 added the merge me Label to trigger pull request merge label Mar 22, 2023
@guibou
Copy link
Collaborator

guibou commented Mar 22, 2023

@wz1000 I'm fine with the removed changes. I've tested and nix run .#haskell-language-server-961 works, so that's perfect ;)

@mergify mergify bot merged commit 191bda6 into master Mar 22, 2023
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.

None yet

10 participants