haskell-language-server icon indicating copy to clipboard operation
haskell-language-server copied to clipboard

wrapper.in: Allow runtime ghc-pkgs to be a subset of compile-time ghc-pkgs

Open maralorn opened this issue 2 years ago • 4 comments

This still makes sure that ghc has been compiled with the same core libraries as hls while it allows runtime environments where other packages have been added to the ghc-pkg database.

This commit also adds that file to the sdist, so that distro packagers can use it.

Compare: https://github.com/NixOS/nixpkgs/pull/192540 and https://github.com/NixOS/nixpkgs/issues/175565 as motivations for this from nixpkgs.

maralorn avatar Sep 23 '22 02:09 maralorn

I guess this requires a review by @hasufell.

maralorn avatar Sep 23 '22 03:09 maralorn

What it does is this: It creates a list of ghc-pkgs present at compile time and saves them in an additional space delimited string of package names in PKG_LIST. It also saves the abi hashes for exactly that packages in ABI_HASHES.

Then at runtime it doesn call ghc-pkg list again but uses the PKG_LIST variable to query the ABI_HASHES only for those packages present at compile time. If one of the packages present at compile time isn‘t present at runtime or has a different abi, this will certainly create an abi hash match.

Yes, I have tested this.

  1. I have tested that the CNUmakefile generates the wrapper script how it is intended. e.g. with the correct PKG_LIST and ABI_HASHES.
  2. I have tested that script is executable and doesn’t report an ABI mismatch when started. (I have seen the script report an abi mismatch before this change.)

I agree that we could just hard-code the PKG_LIST, but I thought this solution is closer to the original and therefore stricter.

maralorn avatar Sep 23 '22 10:09 maralorn

Sounds reasonable. I can't comment on the nix parts.

hasufell avatar Sep 23 '22 14:09 hasufell

It sounds like this is quite a complex and fiddly change. Perhaps your excellent explanation for Julian should live in the code too, @maralorn ? :)

michaelpj avatar Sep 23 '22 16:09 michaelpj

Is this case possible with stock cabal? If you use the old v1-install maybe?

Because I wouldn't want to have nix specific workarounds in the code.

But I think it could indeed be a general issue.

hasufell avatar Sep 24 '22 00:09 hasufell

Is this case possible with stock cabal? If you use the old v1-install maybe?

Because I wouldn't want to have nix specific workarounds in the code.

But I think it could indeed be a general issue.

I honestly don’t know. Do I believe this is probable to be a problem for non-nix setups in practice? No. But I think you could simply install a package with ghc-pkg register --global and then the current script would fail. (Although I have never done this, and I wouldn’t even know how to produce a package that could then be fed into that command.) I doubt that this happens in the wild, but you never know.

I would reframe this from a workaround to a correctness issue, though. According to Zubin here: https://github.com/haskell/haskell-language-server/pull/2675#discussion_r798636403 we need to make sure, that all the boot libraries hls has been compiled against are present with the same ABI hash as at compile time to prevent linking issues. (Although I am a bit fuzzy as to why exactly …) With this change, that is actually what we are checking. It is completely irrelevant if there are more packages present at runtime, so I‘d argue we shouldn‘t check for that.

But I have no strong opinion about this, if you think this is too much of a complication then we maintain it downstream, It’d be a tolerable inconvenience for us.

maralorn avatar Sep 25 '22 02:09 maralorn

@michaelpj I have added a little bit of comments to explain what’s going on. I can elaborate more if desired.

maralorn avatar Sep 25 '22 02:09 maralorn

I had a very helpful discussion with @wz1000 on matrix. I will summarize what I learned here, risking that you already know this, but it might be helpful to others:

The following 4 ABIs have to match:

  1. The boot libraries HLS was compiled with.
  2. The boot libraries HLS is linked against.
  3. The boot libraries TH splices are compiled with.
  4. The boot libraries TH splices are linked against.

1↔2 and 3↔4 are obvious, that’s how linking works. 2↔4 need to match because data flows between HLS and TH code during its evaluation and the datatypes need to be compatible. (Also apparently, apart from being not desirable, the way boot packages are referenced during linking means that it’s currently not possible to link TH splices against different abi-versions of boot packages than hls.)

The bindist hls does not bundle its own boot packages, but loads all of them from the environment in the project it is used on. So the wrapper is very important to check that 1↔2 matches. If, like we, e.g., do in nixpkgs, hls is distributed with and linked against its own boot packages, 1↔2 is automatically satisfied. But since HLS will link TH splices against the same boot packages it has itself been linked to, we need to check 3↔4. (Because TH splices can call code compiled by the local ghc against the local boot packages.) This means, either way, the wrapper needs to check that all boot packages hls was compiled against are present with the same ABI.

So circling back, this PR does in my opinion the right thing by comparing the ABIs for all boot packages in the wrapper, without assuming that only boot packages are in the database. In the bindist GNUmakefile it’s fair to assume that the currently available packages are actually the boot packages, but other distribution methods can populate the BOOT_PKGS variable differently.

maralorn avatar Sep 25 '22 04:09 maralorn