[WIP] Download hlint data-files if they cant be found. #1540

fendor · 2020-01-02T20:55:14Z

Closes #1143
Changes the behaviour in the following way:

Introduces a initializeHlint function
Introduce HIE_HLINT_DATADIR env variable for the location of the data-files.
Download missing data-files with stack or cabal, whatever is available.
Copy data-files into XDG_DATA_HOME/hie/hlint
Write the data-dir location into the extensible state.

Missing stuff:

After downloading stuff, set the data-dir location in the extensible state.
Use a https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#window_showMessageRequest to ask if we should download stuff.
Add a test to prove the code does what we want.
Update install.hs to do this stuff for us, cc @jneira.
Check if at least one file is in the data-dir.

To check it out, open a project wait for typecheck, then reopen the project again. Now no data-files should be downloaded and hlint should produce diagnostics. On the first try, it definitely does not produce diagnostics, since I didnt implement After downloading stuff, set the data-dir location in the extensible state. yet.

fendor · 2020-01-02T21:10:30Z

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs

+            Cabal -> ["get", hlintName]
+            Stack -> ["unpack", hlintName]


What if the cabal is not updated and doesnt know about the hlint version?
What if the hlint version is not on stackage? (very unlikely, just feels like not good software engineering)

Mmm and what about download the data file directly from https://raw.githubusercontent.com/ndmitchell/hlint/v2.2.5/data/hlint.yaml?
Are the rest of files inside data needed for the hie usage?
Hardcoding the url is not ideal, but it uses fewer moving parts

Yes, that is also an option. Just making sure that we get the one as per the version of hlint we are using in the build. Probably the best option.

fendor · 2020-01-02T21:11:14Z

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs

-  let hOpts = hlintOpts lintFile (oneHintPos <$> mhint)
+getIdeas :: MonadIO m => HlintDataDir -> FilePath -> Maybe OneHint -> ExceptT String m [Idea]
+getIdeas dataDir lintFile mhint = do
+  let hOpts = hlintOpts dataDir lintFile (oneHintPos <$> mhint)


Do more locations need to be updates?

fendor · 2020-01-02T21:12:09Z

src/Haskell/Ide/Engine/Server.hs

+                logm "Downloading hlint data-files now."
+                liftIO ApplyRefact.downloadHlintDatafiles
+                logm "Finished downloading."


Should probably be messages to the user.

Yeah, progress status messages in the status bar would be great for that

fendor · 2020-01-02T21:13:40Z

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs

+hlintName :: String
+hlintName = "hlint-" ++ VERSION_hlint
+
+hlintDataDirDefaultLocation :: MonadIO m => m FilePath
+hlintDataDirDefaultLocation = liftIO $ getXdgDirectory XdgData ("hie" </> "hlint")


I feel like we have to duplicate this code in install.hs, which I think is problematic, especially since we have to VERSION_hlint no access. Any ideas @jneira?

we are using the same version of hlint for all ghc versions, right? It is hardcoded in all stack-*.yaml, so maybe it is not a big deal hardcoding it in the script? Not very elegant, though.

Mmm maybe overriding the environment variable hlint_datadir defined by cabal in Paths_hlint.hs before installing hie, so hlint data files will go in that location (not sure it works that way)

fendor · 2020-01-02T21:14:14Z

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs

+      liftIO (doesFileExist explicitDir)
+        >>= \case
+          True ->
+            return $ Right explicitDir


Add a check if the directory contains any files.

alanz · 2020-01-02T21:20:13Z

I am not convinced that this is the right way to do this.

Can we not copy the config file at compile time? Even if we write some sort of helper exe.

And if we do end up doing this, I do not believe the downloading code belongs in the ApplyRefact plugin.

Perhaps we should reach out to @ndmitchell for some ideas around this. I am sure he has a way of installing hlint for CI via a simple download. Rather tap into existing infrastructure.

fendor · 2020-01-02T21:37:32Z

We discussed this issue in ndmitchell/hlint#620. I took away from it, that it is probably best to use cabal get or stack unpack to get the data-files.
We can do it only at compile-time and handle it like we do with hoogle.
Downloading them at run-time has the advantage that it is viable for users to just download the executable and it works right away. But if you think this is not worth the additional overhead, we can do it only at compile-time.

alanz · 2020-01-02T21:45:33Z

The hlint install CI script does

$ curl -sSL https://raw.github.com/ndmitchell/neil/master/misc/run.sh
#!/bin/sh
# This script is invoked from my Travis commands
# It bootstraps to grab the a binary release and run it
set -e # exit on errors

PACKAGE=$1
if [ -z "$PACKAGE" ]; then
    echo No arguments provided, please pass the project name as the first argument
    exit 1
fi
shift

case "$(uname)" in
    "Darwin")
        OS=osx;;
    MINGW64_NT-*|MSYS_NT-*)
        OS=windows;;
    *)
        OS=linux
esac

if [ "$OS" = "windows" ]; then
    EXT=.zip
    ESCEXT=\.zip
else
    EXT=.tar.gz
    ESCEXT=\.tar\.gz
fi

echo Downloading and running $PACKAGE...
# Don't go for the API since it hits the Appveyor GitHub API limit and fails
RELEASES=$(curl --silent --show-error /~https://github.com/ndmitchell/$PACKAGE/releases)
URL=/~https://github.com/$(echo $RELEASES | grep -o '\"[^\"]*-x86_64-'$OS$ESCEXT'\"' | sed s/\"//g | head -n1)
VERSION=$(echo $URL | sed -n 's@.*-\(.*\)-x86_64-'$OS$ESCEXT'@\1@p')
TEMP=$(mktemp -d .$PACKAGE-XXXXXX)

cleanup(){
    rm -r $TEMP
}
trap cleanup EXIT

retry(){
    ($@) && return
    sleep 15
    ($@) && return
    sleep 15
    $@
}

retry curl --progress-bar --location -o$TEMP/$PACKAGE$EXT $URL
if [ "$OS" = "windows" ]; then
    7z x -y $TEMP/$PACKAGE$EXT -o$TEMP -r > /dev/null
else
    tar -xzf $TEMP/$PACKAGE$EXT -C$TEMP
fi
$TEMP/$PACKAGE-$VERSION/$PACKAGE $*

Downloading /~https://github.com/ndmitchell/hlint/releases/download/v2.2.5/hlint-2.2.5-x86_64-linux.tar.gz (from the hlint releases page on github) shows it has the exe and data dir.

Can't we do this at compile time?

fendor · 2020-01-02T21:55:20Z

We can do all the stuff at compile-time (not sure what the script does, but we can solve the problem at compile-time). @jneira and I just thought that downloading the files at run-time would be more user-friendly.

alanz · 2020-01-02T22:07:35Z

@fendor the fewer moving parts the better. I would rather we install it something like the hlint CI images, where you get a platform-specific, ready-to-roll executable, with all its data files.

Otherwise we end up re-implementing a download manager, dealing with security issues, proxies, corporate firewalls, failed downloads, version mismatches, partial downloads and corrupt files, etc.

jneira · 2020-01-02T23:57:32Z

@fendor the fewer moving parts the better. I would rather we install it something like the hlint CI images, where you get a platform-specific, ready-to-roll executable, with all its data files.

A zip/tar self-contained with all the data files needed woud work too for precompiled binaries.

Otherwise we end up re-implementing a download manager, dealing with security issues, proxies, corporate firewalls, failed downloads, version mismatches, partial downloads and corrupt files, etc.

Agree it is better to avoid it if possible. To be fair the pr tried to download the data files indirectly using stack or cabal (that have more chances to work if you are already using them)

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs

jneira · 2020-01-03T05:34:11Z

Reading the hlint code i've found where it is using the executable path to looking for the data file:

/~https://github.com/ndmitchell/hlint/blob/master/src/CmdLine.hs#L53-L60

It is in the path called by the hlint executable and no the lib (we are using it as a lib, right?) so we would need set hlint_datadir with the value of getExecutablePath ++ "/hlint/data". I dont fully like use env vars but it would be the quick one, although if the api let pass the value of the path we should use it.

I think we still should show an error with a suggesting about how to fix it manually in case the data files would not be there.

fendor · 2020-01-03T11:35:00Z

Closing in favour of a different solution.

ndmitchell · 2020-01-15T16:36:48Z

I received a PR to embed the files via Template Haskell, which I'm planning to accept after changes. With that done, you won't have to worry again: ndmitchell/hlint#824

Query for data-files and download them on-the-fly

03f2935

fendor requested review from lukel97, alanz and jneira January 2, 2020 21:08

fendor commented Jan 2, 2020

View reviewed changes

lukel97 reviewed Jan 3, 2020

View reviewed changes

src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs Show resolved Hide resolved

fendor mentioned this pull request Jan 3, 2020

Hlint data-files can not be easily distributed #1143

Closed

fendor closed this Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Download hlint data-files if they cant be found. #1540

[WIP] Download hlint data-files if they cant be found. #1540

fendor commented Jan 2, 2020 •

edited

Loading

fendor Jan 2, 2020 •

edited

Loading

jneira Jan 2, 2020 •

edited

Loading

alanz Jan 2, 2020

fendor Jan 2, 2020

fendor Jan 2, 2020

jneira Jan 2, 2020

fendor Jan 2, 2020 •

edited

Loading

jneira Jan 2, 2020

fendor Jan 2, 2020 •

edited

Loading

alanz commented Jan 2, 2020

fendor commented Jan 2, 2020 •

edited

Loading

alanz commented Jan 2, 2020

fendor commented Jan 2, 2020

alanz commented Jan 2, 2020

jneira commented Jan 2, 2020 •

edited

Loading

jneira commented Jan 3, 2020 •

edited

Loading

fendor commented Jan 3, 2020

ndmitchell commented Jan 15, 2020 •

edited

Loading

[WIP] Download hlint data-files if they cant be found. #1540

[WIP] Download hlint data-files if they cant be found. #1540

Conversation

fendor commented Jan 2, 2020 • edited Loading

fendor Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

jneira Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

alanz Jan 2, 2020

Choose a reason for hiding this comment

fendor Jan 2, 2020

Choose a reason for hiding this comment

fendor Jan 2, 2020

Choose a reason for hiding this comment

jneira Jan 2, 2020

Choose a reason for hiding this comment

fendor Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

jneira Jan 2, 2020

Choose a reason for hiding this comment

fendor Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

alanz commented Jan 2, 2020

fendor commented Jan 2, 2020 • edited Loading

alanz commented Jan 2, 2020

fendor commented Jan 2, 2020

alanz commented Jan 2, 2020

jneira commented Jan 2, 2020 • edited Loading

jneira commented Jan 3, 2020 • edited Loading

fendor commented Jan 3, 2020

ndmitchell commented Jan 15, 2020 • edited Loading

fendor commented Jan 2, 2020 •

edited

Loading

fendor Jan 2, 2020 •

edited

Loading

jneira Jan 2, 2020 •

edited

Loading

fendor Jan 2, 2020 •

edited

Loading

fendor Jan 2, 2020 •

edited

Loading

fendor commented Jan 2, 2020 •

edited

Loading

jneira commented Jan 2, 2020 •

edited

Loading

jneira commented Jan 3, 2020 •

edited

Loading

ndmitchell commented Jan 15, 2020 •

edited

Loading