Consider user installed HLSes (e.g. via ghcup compile) #543

hasufell · 2022-03-13T17:32:05Z

Fixes #542

We don't do random fallbacks anymore. If serverExecutablePath is set, it must exist. We only check PATH if no other setting is enabled.

package.json

hasufell · 2022-03-13T20:24:32Z

src/extension.ts

+    if (e instanceof MissingToolError) {
+      const link = e.installLink();
+      if (link) {
+        if (await window.showErrorMessage(e.message, `Install ${e.tool}`)) {
+          env.openExternal(link);
+        }
+      } else {
+        await window.showErrorMessage(e.message);
+      }
+    } else if (e instanceof Error) {


Here at the top-level of extension init we catch all promise errors and also restore the MissingToolError with link popup.

hasufell · 2022-03-13T20:25:50Z

src/hlsBinaries.ts

 */
-function findHLSinPATH(context: ExtensionContext, logger: Logger, folder?: WorkspaceFolder): string | null {
-  // try 'serverExecutablePath' setting
+function findServerExecutable(context: ExtensionContext, logger: Logger, folder?: WorkspaceFolder): string {


So we separate findHLSinPATH and findServerExecutable...

hasufell · 2022-03-13T20:27:45Z

src/hlsBinaries.ts

-        // permissively check if we have HLS installed
-        // this is just to avoid a popup
-        let wrapper = await callGHCup(context, logger,
-            ['whereis', 'hls'],
-            undefined,
-            false,
-            (err, stdout, _stderr, resolve, _reject) => { err ? resolve('') : resolve(stdout?.trim()); }
-        );
-        if (wrapper === '') {
-            // install recommended HLS... even if this doesn't support the project GHC, because
-            // we need a HLS to find the correct project GHC in the first place
-            await callGHCup(context, logger,
+        // we manage HLS, make sure ghcup is installed/available
+        await getGHCup(context, logger);
+
+        // get a preliminary hls wrapper for finding project GHC version,
+        // later we may install a different HLS that supports the given GHC
+        let wrapper = await getLatestHLSfromGHCup(context, storagePath, logger).then(e =>
+          (e === null)
+          ?  callGHCup(context, logger,


Here's some considerable cleanup... now we pick a proper hls-wrapper executable (e.g. a compiled one) for the first pass (the one that just discovers project GHC), not just the recommended... then the next pass will possibly downgrade the wrapper to match the project GHC.

hasufell · 2022-03-13T20:29:52Z

src/hlsBinaries.ts

@@ -346,10 +384,15 @@ export async function getProjectGHCVersion(
                    logger.error(`stdout: ${stdout}`);
                }
                // Error message emitted by HLS-wrapper
-                const regex = /Cradle requires (.+) but couldn't find it/;
+                const regex = /Cradle requires (.+) but couldn't find it|The program \'(.+)\' version .* is required but the version of.*could.*not be determined|Cannot find the program \'(.+)\'\. User-specified/;


This is the regex that will cause MissingToolError and matches on various outputs from the wrapper.

Ok, this is a work-around until haskell/haskell-language-server#2591 lands?

hasufell · 2022-03-13T20:30:35Z

src/hlsBinaries.ts

+  context: ExtensionContext,
+  storagePath: string,
+  logger: Logger,
+  targetGhc?: string


targetGhc is undefined for the first pass, where we just want the hls wrapper and don't care about hls exe.

hasufell · 2022-03-13T20:31:37Z

src/hlsBinaries.ts

-        throw new Error(noMatchingHLS);
-    } else {
+    const latestMetadataHls = await getLatestHLSfromMetadata(context, storagePath, projectGhc, logger);
+    const latestGhcupHls = await getLatestHLSfromGHCup(context, storagePath, logger, projectGhc).then(e => e === null ? null : e[0]);


Here's where we get a possibly self-compiled already installed HLS.

Worthy not: we only consider the latest self-compiled...

There is another edge case: if you compile HLS 1.6.1.0 manually instead of via bindist, the supported GHC versions differ from the metadata.

I'm not sure that's worth to support explicitly.

Worthy not: we only consider the latest self-compiled...

What does that mean? If I have compiled version 1.6.1.0, and then I compile version 1.5.0.0, does that mean version 1.5.0.0 is used (since it is the most recent compiled HLS version), or that no matter what, we only use 1.6.1.0 and if there is no version 1.6.1.0, we would not use the self compiled version 1.5.0.0 either? Does this take into account what the latest supported HLS version relative to a GHC version is? E.g. when 8.10.4 supports only up to 1.4.0.0, and I compile the version 1.5.0.0 for it (because that's possible in theory) would the extension pick that up correctly?

If 1.6.1.0 is the latest properly released one and you have 1.7.0.0 and 1.8.0.0 compiled, the extension will only bother looking at 1.8.0.0.

There are a number of complicated interactions here, e.g. it's possible that you compiled 1.6.1.0 as well and it doesn't support the project GHC, because it's only built for one GHC (unlike the bindist).

So we have a Map of HLS ver -> GHC versions.

A way out here would be this:

Map A: gather all ghcup-installed HLSes and their corresponding supported GHC version, regardless of the HLS metadata file

Map B: gather HLS metadata

merge both this way:

if a HLS version exists both in A and B, only consider GHC versions from A (because ghcup install hls <ver> would not overwrite the already existing one... and the existing one might have less supported GHC versions)

if a HLS version exists only in A, pick A

if a HLS version exists only in B, pick B

I'm starting to think this functionality belongs into ghcup. It is non-trivial.

It is non-trivial. Adding it to ghcup directly sounds like a nice idea!

src/hlsBinaries.ts

fendor

Looking good, haven't had a chance to test everything out, though

fendor · 2022-03-14T19:41:25Z

src/hlsBinaries.ts

@@ -346,10 +384,15 @@ export async function getProjectGHCVersion(
                    logger.error(`stdout: ${stdout}`);
                }
                // Error message emitted by HLS-wrapper
-                const regex = /Cradle requires (.+) but couldn't find it/;
+                const regex = /Cradle requires (.+) but couldn't find it|The program \'(.+)\' version .* is required but the version of.*could.*not be determined|Cannot find the program \'(.+)\'\. User-specified/;


Ok, this is a work-around until haskell/haskell-language-server#2591 lands?

fendor · 2022-03-14T19:43:56Z

src/hlsBinaries.ts

-        throw new Error(noMatchingHLS);
-    } else {
+    const latestMetadataHls = await getLatestHLSfromMetadata(context, storagePath, projectGhc, logger);
+    const latestGhcupHls = await getLatestHLSfromGHCup(context, storagePath, logger, projectGhc).then(e => e === null ? null : e[0]);


Worthy not: we only consider the latest self-compiled...

What does that mean? If I have compiled version 1.6.1.0, and then I compile version 1.5.0.0, does that mean version 1.5.0.0 is used (since it is the most recent compiled HLS version), or that no matter what, we only use 1.6.1.0 and if there is no version 1.6.1.0, we would not use the self compiled version 1.5.0.0 either? Does this take into account what the latest supported HLS version relative to a GHC version is? E.g. when 8.10.4 supports only up to 1.4.0.0, and I compile the version 1.5.0.0 for it (because that's possible in theory) would the extension pick that up correctly?

hasufell · 2022-03-14T23:17:51Z

src/hlsBinaries.ts

+    // first we get supported GHC versions from available HLS bindists (whether installed or not)
+    const metadataMap = await getHLSesfromMetadata(context, storagePath, logger);
+    // then we get supported GHC versions from currently installed HLS versions
+    const ghcupMap = await getHLSesFromGHCup(context, storagePath, logger);
+    // since installed HLS versions may support a different set of GHC versions than the bindists
+    // (e.g. because the user ran 'ghcup compile hls'), we need to merge both maps, preferring
+    // values from already installed HLSes
+    const merged = (metadataMap === null)
+        ? ghcupMap
+        : ((ghcupMap === null)
+            ? null
+            : (new Map<string, string[]>([...metadataMap, ...ghcupMap]))); // right-biased
+
+    if (!merged) {
+      window.showErrorMessage(noMatchingHLS);
+      throw new Error(noMatchingHLS);
    } else {
-        return latestMetadataHls;
+        // now sort and get the latest suitable version
+        const latest = [...merged].filter(([k, v]) => v.some(x => x === projectGhc)).sort(([k1, v1], [k2, v2]) => comparePVP(k1, k2)).pop();
+        if (latest) {
+            return latest[0];
+
+        } else {
+          window.showErrorMessage(noMatchingHLS);
+          throw new Error(noMatchingHLS);
+        }


Here is the juice... we merge the maps.

And correctly merge all those maps.

fendor

I believe the changes are good as-is, but we need to make sure the documentation clearly explains all the possible user interactions.

E.g. the options system-ghcup, internal-ghcup and PATH are quite unclear to absolute beginners.

hasufell requested a review from fendor March 13, 2022 17:32

hasufell force-pushed the issue-542 branch from 3f8998d to 6078a4b Compare March 13, 2022 17:38

Consider user installed HLSes (e.g. via ghcup compile)

a9195f4

Fixes haskell#542

hasufell force-pushed the issue-542 branch from 6078a4b to a9195f4 Compare March 13, 2022 17:40

hasufell added 2 commits March 13, 2022 18:49

Be more strict about PATH vs serverExecutablePath

2c7fd41

We don't do random fallbacks anymore. If serverExecutablePath is set, it must exist. We only check PATH if no other setting is enabled.

Rework configuration setting and error handling

49b6dfe

hasufell commented Mar 13, 2022

View reviewed changes

package.json Show resolved Hide resolved

hasufell commented Mar 13, 2022

View reviewed changes

src/hlsBinaries.ts Outdated Show resolved Hide resolved

Make serverExecutablePATH override all other settings

484cd67

hasufell mentioned this pull request Mar 13, 2022

Difficult errors when no appropriate GHC is found #541

Closed

Fix windows

6ebfdef

fendor approved these changes Mar 14, 2022

View reviewed changes

hasufell force-pushed the issue-542 branch from 426f12e to cc58c9a Compare March 14, 2022 21:08

hasufell marked this pull request as draft March 14, 2022 21:13

hasufell force-pushed the issue-542 branch from cc58c9a to 95698ce Compare March 14, 2022 23:16

hasufell marked this pull request as ready for review March 14, 2022 23:17

hasufell commented Mar 14, 2022

View reviewed changes

hasufell force-pushed the issue-542 branch from 95698ce to 4788e33 Compare March 15, 2022 22:02

Consider all ghcup installed/compile HLSes

4493e48

And correctly merge all those maps.

hasufell force-pushed the issue-542 branch from 4788e33 to 4493e48 Compare March 15, 2022 22:14

hasufell added 2 commits March 16, 2022 11:40

Cleanup and improve error handling

757c0d6

Format prettier

1a9b77a

maerwald approved these changes Mar 16, 2022

View reviewed changes

fendor approved these changes Mar 17, 2022

View reviewed changes

fendor merged commit 950e02f into haskell:master Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider user installed HLSes (e.g. via ghcup compile) #543

Consider user installed HLSes (e.g. via ghcup compile) #543

hasufell commented Mar 13, 2022

hasufell Mar 13, 2022

hasufell Mar 13, 2022

hasufell Mar 13, 2022

hasufell Mar 13, 2022

fendor Mar 14, 2022

hasufell Mar 14, 2022

hasufell Mar 13, 2022

hasufell Mar 13, 2022

hasufell Mar 14, 2022

hasufell Mar 14, 2022

fendor Mar 14, 2022

hasufell Mar 14, 2022 •

edited

Loading

hasufell Mar 14, 2022

fendor Mar 14, 2022

fendor left a comment

fendor Mar 14, 2022

fendor Mar 14, 2022

hasufell Mar 14, 2022

fendor left a comment

Consider user installed HLSes (e.g. via ghcup compile) #543

Consider user installed HLSes (e.g. via ghcup compile) #543

Conversation

hasufell commented Mar 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasufell Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor left a comment

Choose a reason for hiding this comment

hasufell Mar 14, 2022 •

edited

Loading