-
Notifications
You must be signed in to change notification settings - Fork 90
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
Introduce 'haskell.toolchain' setting #562
Conversation
a89c6d5
to
687e9b1
Compare
@@ -291,7 +316,7 @@ export async function findHaskellLanguageServer( | |||
latestHLS, | |||
...(latestCabal ? ['--cabal', latestCabal] : []), | |||
...(latestStack ? ['--stack', latestStack] : []), | |||
...(recGHC ? ['--ghc', 'recommended'] : []), | |||
...(recGHC ? ['--ghc', recGHC] : []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bugfix regardless of this PR.
687e9b1
to
383fb8f
Compare
src/hlsBinaries.ts
Outdated
@@ -266,21 +270,42 @@ export async function findHaskellLanguageServer( | |||
} else { | |||
// we manage HLS, make sure ghcup is installed/available | |||
await upgradeGHCup(context, logger); | |||
|
|||
const toolchainConfig: ToolConfig = new Map(Object.entries(workspace.getConfiguration('haskell').get('toolchain') as any)) as ToolConfig; | |||
let latestHLS: string | undefined | null = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I wasn't aware undefined
and null
were different values in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm abusing this fact to force different codepath. Great engineering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look sensible to me from a quick glance. I will trust you on the details.
Thanks for implementing this and all the other work too!
"description": "Whether to also install/manage GHC when 'manageHLS' is set to 'GHCup'." | ||
"type": "object", | ||
"default": {}, | ||
"description": "When manageHLS is set to GHCup, this can overwrite the automatic toolchain configuration with a more specific one. When a tool is omitted, the extension will manage the version (for 'ghc' we try to figure out the version the project requires). The format is '{\"tool\": \"version\", ...}'. 'version' accepts all identifiers that 'ghcup' accepts." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a link in the GHCup documentation we could point to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather vscode specific and described in the readme.
6d0269f
to
9deed49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! My only complaint is regarding usability, we need to add more explanations why certain messages happen and what users can do to understand the pop-up and why it happens.
Example:
For me, hls-wrapper failed to load, but nothing helped me to understand what I could do to reproduce it or figuring out what is going wrong. Just this:
Found "/home/hugin/Documents/haskell/fpex-eval/hie.yaml" for "/home/hugin/Documents/haskell/fpex-eval/a"
Run entered for haskell-language-server-wrapper(haskell-language-server-wrapper) Version 1.6.1.0, Git revision f4022c5bb8530cd306c53b941878244bf27a5d41 (dirty) x86_64 ghc-8.10.7
Current directory: /home/hugin/Documents/haskell/fpex-eval
Operating system: linux
Arguments: ["--lsp","--debug"]
Cradle directory: /home/hugin/Documents/haskell/fpex-eval
Cradle type: Cabal
Tool versions found on the $PATH
cabal: 3.6.2.0
stack: 2.7.5
ghc: 8.8.4
Consulting the cradle to get project GHC version...
Failed to get project GHC version:CradleError {cradleErrorDependencies = [], cradleErrorExitCode = ExitFailure 1, cradleErrorStderr = ["Error when calling cabal --builddir=/home/hugin/.cache/hie-bios/dist-fpex-eval-22f998bc1b8ed0a562686ad76442afff v2-exec --with-compiler /home/hugin/.cache/hie-bios/wrapper-13a09b18ea883dd377d59db5e821a86b ghc -v0 -- --numeric-version","","cabal: Could not resolve dependencies:\n[__0] trying: fpex-0.0.0 (user goal)\n[__1] next goal: base (dependency of fpex)\n[__1] rejecting: base-4.13.0.0/installed-4.13.0.0 (conflict: fpex =>\nbase>=4.14.0.0 && <4.17.0.0)\n[__1] rejecting: base-4.16.1.0, base-4.16.0.0, base-4.15.1.0, base-4.15.0.0,\nbase-4.14.3.0, base-4.14.2.0, base-4.14.1.0, base-4.14.0.0, base-4.13.0.0,\nbase-4.12.0.0, base-4.11.1.0, base-4.11.0.0, base-4.10.1.0, base-4.10.0.0,\nbase-4.9.1.0, base-4.9.0.0, base-4.8.2.0, base-4.8.1.0, base-4.8.0.0,\nbase-4.7.0.2, base-4.7.0.1, base-4.7.0.0, base-4.6.0.1, base-4.6.0.0,\nbase-4.5.1.0, base-4.5.0.0, base-4.4.1.0, base-4.4.0.0, base-4.3.1.0,\nbase-4.3.0.0, base-4.2.0.2, base-4.2.0.1, base-4.2.0.0, base-4.1.0.0,\nbase-4.0.0.0, base-3.0.3.2, base-3.0.3.1 (constraint from non-upgradeable\npackage requires installed instance)\n[__1] fail (backjumping, conflict set: base, fpex)\nAfter searching the rest of the dependency tree exhaustively, these were the\ngoals I've had most trouble fulfilling: base, fpex\n\n"]}
while the error message itself is coming from HLS (and is objectively bad), we ought to print the exact command a user can run to reproduce this error. For example:
PATH="pathstuff" hls-wrapper --project-ghc-version
or something similar.
Doesn't seem like a regression. |
It's not, but it is something that I think needs improvement. |
I don't think this needs to block the release. Otherwise it will cause another 2 weeks of roundtrip time and then someone discovers yet something else they don't like. |
No, we can start with the pre-release today and ask on discourse / reddit / discord for volunteers and feedback |
Wrt #561
Example config in current workspace:
This means:
ghc
version corresponding to the project (default, because it's omitted)Another config: