Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Hlint data-files can not be easily distributed #1143

Closed
fendor opened this issue Mar 21, 2019 · 24 comments · Fixed by #1598
Closed

Hlint data-files can not be easily distributed #1143

fendor opened this issue Mar 21, 2019 · 24 comments · Fixed by #1598
Assignees
Labels
build Continuous integration and building type: refactor Refactor and tidy up internals.

Comments

@fendor
Copy link
Collaborator

fendor commented Mar 21, 2019

hlint demands that its data-file locations are known at compile time. If you remove these files, for example hlint.yaml, hlint will fail with a message and lets hie crash

hlint: user error (Failed to find requested hint files:
  ~/.cabal/store/ghc-8.4.4/hlint-2.1.15-8782d6ab3dc227b1c49b92fdeae8ecd837138c4d8f742c2f87f0c13927a32609/share/hlint.yaml
)

In this case, hlint has stored its data-files in ~/.cabal/store.

Unfortunately, if you copy the executable hie instead of installing from source, hie itself chokes on that error, since the data-files are not copied, thus, not available. We should, at the very least, catch that error and avoid crash.

Following options to resolve this issue:

  • Fix this upstream, hlint should just create this file in some location or use defaults.
  • Provide all data-files in a custom datadir directory.
  • Turn hlint into an official run-time dependency and demand a working installation.

This is a blocker for #1068

@fendor fendor changed the title Location of hlint.yaml hlint will fail with a message and lets hie crash Mar 21, 2019
@fendor fendor changed the title hlint will fail with a message and lets hie crash Location of hlint.yaml Mar 21, 2019
@samuelpilz
Copy link
Contributor

Moreover: hie it crashes when hlint does, which is undesirable (in my opinion). Can this be fixed?

@fendor fendor changed the title Location of hlint.yaml Hlint data-files can not be easily distributed Mar 21, 2019
@nponeccop
Copy link
Contributor

Is this a duplicate of #400?

@samuelpilz
Copy link
Contributor

@nponeccop It does tackle a similar issue.

I am currently in the process of evaluating runtime-dependencies of hie and found out that the 2 issues described are still relevant.
However, the runtime-dependency for hlint should be tackled in a seperate issue to the one for cabal-helper.

@fendor
Copy link
Collaborator Author

fendor commented Mar 24, 2019

I propose the following, slightly weird, solution:
Use the package file-embed to embed the content of the data-files at compile time into hie.
On startup, check if these files exist at a pre determined location, e.g. $XDG_CONFIG_HOME/haskell-ide-engine/hlint. If yes, supply it to hlint as the --data-dir. Otherwise, write the content of the embedded files into that directory and continue.

Pro:

  • Solves the run-time dependency of hlint on its data-files

Con:

  • Might be error prone.
  • Increases size of hie binary.

@samuelpilz
Copy link
Contributor

I would like to tackle this at install-time. My plan:

  • in install.hs: call cabal get hlint, then store the hlint/data into $HIE_DIR/hlint-data
  • in hie: call hlint with --datadir=$HIE_DIR/hlint-data

My suggestion is that HIE_DIR=$XDG_DATA_HOME/hie, which is $HOME/.local/share by default. When I am able to reliably build binary packages, the directories $XDG_DATA_DIRS (default to /usr/share:/usr/local/share) will also searched for hie/hlint-data content.
See: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

@mpickering @fendor is this fine?

@jneira
Copy link
Member

jneira commented Jan 1, 2020

Mmm i think the solution could be easier for hie and other hlint clients if hlint could look for the data files in a standard location, like $XDG_DATA_HOME/hlint
Users would have an standard location to put their custom global hlint yaml file and we could advise that it is needed when using precompiled binaries or installing hie using cabal or stack.
Of course we could try to be clever and download the default yaml file to that default location it it doesnt exist but i think this should be done at runtime, to make possible use precompiled binaries.

@alanz
Copy link
Collaborator

alanz commented Jan 1, 2020

I am pretty sure that if we made an appropriate PR on upstream hlint it would be accepted, and then can become a standard for all users of it in library mode.

@fendor
Copy link
Collaborator Author

fendor commented Jan 1, 2020

I dont think that data-files can be installed into $XDG_DATA_HOME/hlint reliably, stack and cabal decide on the installation location, same with nix.
I think doing something like cabal get hlint at run-time into $XDG_DATA_HOME/hie is a good idea.
But also installing hlint data-files into $XDG_DATA_HOME/hie at compile-time would be fine, since packaging tools are responsible then to set-up the environment correctly.

@alanz
Copy link
Collaborator

alanz commented Jan 1, 2020

I think compile-time installation into the hie data area probably makes the most sense, as you suggest @fendor .

@jneira
Copy link
Member

jneira commented Jan 1, 2020

My idea was honour the actual order but, after falling back first to actual cwd, looking at $XDG_DATA_DIR/hlint as last resort. I think it would be good for hlint users cause this way they can use a known, standard, build tool agnostic and system wide location where place their hlint.yaml and it will make easier for us handle it 😉 . That way he change will be totally backwards compatible.

It would be similar to the first option commented by @fendor but i feel $XDG_DATA_HOME/hlint would be more correct and logical than $XDG_DATA_HOME/hie.

Independently of the location, i see doing the copy of the file at build time as a complement with runtime, but if we want to use precompiled binaries (and i am firmly conviced that we should offer them) we'll need the second one for sure, right?
At least we'll need to advise users about that requirement and ideally show an informative error with the advise if the file is not there at hie startup (whatever is the reason, including an accidental remove after build)

Otoh, how could be done the copy of the file at build time? with a custom Setup.hs, using install.hs ? The second one would not work if we upload hie to hackage/stackage and custom Setup.hs's are reputed to be fragile.

@fendor
Copy link
Collaborator Author

fendor commented Jan 1, 2020

Does hlint not already do some of the described stuff?

I dont think we are allowed to claim $XDG_DATA_HOME/hlint since we are not hlint, we are hie, this directory is not ours to claim, in my understanding of XDG spec.

My idea would have been that we are listening to a special hie_hlint_data_dir env var and if this is not set, default to $XDG_DATA_HOME/hie/hlint. We write the data-files via our install script to that location and all packaging tools, e.g. debian, need to either define that env-var or copy the data-files there, themselves. I have no experience with a custom Setup.hs, so maybe that is useful, too?

Obtaining the files at run-time would be doable, too.

Yeah, a good error message must be added.

@jneira
Copy link
Member

jneira commented Jan 2, 2020

Does hlint not already do some of the described stuff?

yeah except looking in $XDG_DATA_DIR/hlint as last step

I dont think we are allowed to claim $XDG_DATA_HOME/hlint since we are not hlint, we are hie, this directory is not ours to claim, in my understanding of XDG spec.

I was thinking in add the support to hlint itself (cause i think it would be good for hlint users too)

My idea would have been that we are listening to a special hie_hlint_data_dir env var and if this is not set, default to $XDG_DATA_HOME/hie/hlint. We write the data-files via our install script to that location and all packaging tools, e.g. debian, need to either define that env-var or copy the data-files there, themselves. I have no experience with a custom Setup.hs, so maybe that is useful, too?

Well to be fair this way (using $XDG_DATA_DIR/hie) we could have a specific hlint data file customizable for hie needs.
I am afraid that windows has not an official package manager (chocolatey is not widely used). Moreover you will not have the files if you use directly the precompiled binaries for *nix.

Obtaining the files at run-time would be doable, too.

I think its main advantage is that it would cover all executable use cases (build from source, installing using cabal, stack or a os package manager, using directly the precompiled binary, etc)

Yeah, a good error message must be added.

It always would be needed, whatever we choose to do before that (including download the file at runtime: it could fail)

@fendor
Copy link
Collaborator Author

fendor commented Jan 2, 2020

I decided to implement it at run-time but also do the right thing at compile-time:
By default, install.hs installs hlint data-files to XDG_DATA_HOME/hie/hlint.
At run-time, we honour the env variable HIE_HLINT_DATADIR to override the data-files location.
If the data-files do not exist, we ask the user if they want to install the data-files with: https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#window_showMessageRequest
If there are no data-files, then hlint just does not work, but gracefully. So, no crashes and so on.

Last question for me: should we offer to install the data-files to HIE_HLINT_DATADIR if it is set, or do we rely on the user to do the right thing?

@fendor
Copy link
Collaborator Author

fendor commented Jan 3, 2020

Back to the drawing board, after the feedback in #1540

As I understood it now, we can aim to perform the following:

  • Solve the problem only at compile-time
    • Install hlint-data-files into $XDG_DATA_HOME/hie/hlint (with install.hs build-data)
      • Is using cabal get and stack unpack an acceptable solution?
    • Prefer hlint_datadir over custom env variable.
  • If data-files arent properly initialised, show an error log message with a hint (link to our readme?) on how to solve it.

Open questions:

  • Hacking on hie (now a simple cabal install is definitely not enough for basic functionality)
  • Tests (we have to adapt the tests to create the hlint dir.)
  • Maybe we can only $XDG_DATA_HOME/hie/hlint for data-files, if the hlint data_dir does not exist on the system?

cc @jneira, @alanz, @bubba would this solution be better?

@jneira
Copy link
Member

jneira commented Jan 3, 2020

mmm i think the last @alanz proposal was to do something similar to hlint itself (correct me if i am wrong):

  • At build time: package in a tar or zip the contens:
    • hie and hie-wrapper executables
    • a subfolder hlint/data with the required hlint data files inside
      • taking the data files using stack/cabal or direct download (it only will be done once in our ci)
  • At runtime:
    • Set the variable hlint_datadir (only if it is nos previously set by the user) to point to getExecutablePath() </> "/hlint/data". This way, as long the user honour the structure of the package mentioned, the data files will be found
      • Only in case we can't pass directly the path (getExecutablePath() </> "/hlint/data") to hlint api (although we should honour the hlint_datadir if it is set by the user)
    • Check the data files are in that directory and show an error, suggesting users restore o create it (using the original package)

NOTE: getExecutablePath() is from http://hackage.haskell.org/package/base-4.12.0.0/docs/System-Environment.html

@alanz
Copy link
Collaborator

alanz commented Jan 3, 2020

@jneira I agree with your solution, with the one proviso that the current usage continues to work.

In other words, looking for the data files in this installed directory should be a fallback if they are not found where they are expected.

If necessary we should make a PR on hlint to assist in this.

@Avi-D-coder
Copy link
Collaborator

This seems like a flaw in hlint.
As I understand it hlint produces at compile time hlint.yaml, the contents may change with between versions and in rare cases be edited by the user. If I were packaging it I would put it in /usr/share and it would be replaced on every update. This makes using it as config useless, but the alternative is /etc/ and there it should not be replaced each version.

Packaging aside, If I do stack install and then rm -rf ~/.stack hlint and hie should still work.

hlint should embed the config default and look for hlint.yaml in XDG config if global config overrides are desired addition to existing support for local .hlint.yaml config.

@fendor
Copy link
Collaborator Author

fendor commented Jan 4, 2020

It does not produce it at compile-time, it just compiles the location of hlint.yaml, which is determined by cabal or stack, into the executable. The file hlint.yaml is a yaml file in the top-level directory of hlint, namely in data.

I think, it also does something like that, too. You may want to take a look at ndmitchell/hlint#620, were we discussed this issue with the author. I am sure you can add something to it, if you want to.

@Avi-D-coder
Copy link
Collaborator

@fendor thanks for pointing that conversion out.
See Avi-D-coder/hlint@b50d9cf for the embed approach.

@jneira
Copy link
Member

jneira commented Jan 7, 2020

Good news: we have to do nothing to get the required behaviour cause we already are hitting the code path that search the data files inside the executable path (in our case hie)
I've tested it:

  • renaming the hlint.yaml inside the cabal store build dir for hlint where my hie installation was looking for hlint.yaml
  • check that hie throws the error cause it cant found it, mentioning the dir in the cabal store
lint: user error (Failed to find requested hint files:
  D:\csd\ghc-8.6.5\hlint-2.2.5-92f7ca9cdeeef4f61326e0e6caa9726612d1ec03\share\hlint.yaml
)
  • renaming the cabal store build dir to make hlint does not looking for there
  • creating a folder data in the same directory where the hie executable is (D:\bin in my case) and place hlint.yaml inside
  • check that hie returns to work fine
  • renaming hlint.yaml inside the above folder but keeping the folder
  • check that hie throws the error but mentioning the path where the hie executable is:
lint: user error (Failed to find requested hint files:
  D:\bin\data\hlint.yaml
)

So only left package data/hlint.yaml with hie.exe to make it work!
I love it when a plan comes together! 😄

@jneira
Copy link
Member

jneira commented Jan 9, 2020

Closed by #1545

@jneira
Copy link
Member

jneira commented Jan 14, 2020

Reopen to track the file embed alternative. There is a pr opened by @Avi-D-coder in hlint: ndmitchell/hlint#824
Afaiu we will use it automatically if the pr is merged but we will have to document it and stop distributing the file in releases

@jneira jneira reopened this Jan 14, 2020
@jneira
Copy link
Member

jneira commented Jan 21, 2020

hlint already has the data embedded: see #1587
Next tasks:

  • test hie with no hlint data file in the known locations
  • update release config to remove hlint-yaml
  • update docs

@jneira
Copy link
Member

jneira commented Jan 24, 2020

I've tested it works locally, renaming hlint data files in the store-dir and removing the data dir next to hie executable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Continuous integration and building type: refactor Refactor and tidy up internals.
Projects
None yet
7 participants