wrapper.in: Require runtime ghc-pkgs to be an abi compatible superset of bootpkgs

[maralorn]

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: NixOS/nixpkgs#192540 and
NixOS/nixpkgs#175565 as motivations for this from
nixpkgs.

[maralorn]

I guess this requires a review by @hasufell.

[hasufell]

Have you actually tested this? I haven't looked at the entire code, but I don't see how you compare both ABIs.

The abi is just a space delimited string of the abi hashes of the individual pkgs. How do you strip the compile time one of the extraneous packages?

I'd just hardcode the boot packages or a subset of them (as said: ghc and base) and don't use ghc-pkg list at all.

[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.

[hasufell]

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

[michaelpj]

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

[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.

[maralorn]

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: #2675 (comment) 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]

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

[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.

[hasufell]

Where exactly do you allow a subset of packages here? You check everything that's used at compile time. Even if it's not relevant to HLS and if it's not an actual boot package. That's not a subset. It's either exactly all boot libraries or a superset of them.

I think a better idea is really to:

1. hardcode the pkgs that are relevant to HLS ABI
2. Only check those

I'd also be fine to hardcode the entire list of boot libraries. These may diverge for GHC versions in the future, but we only support a certain range anyway.

[maralorn]

I am so, so, sorry. I had a stupid thinko/sign-error in the commit title. I meant all along, that the runtime packages can be a superset, not a subset of the compiletime/bootpkgs.

I think using the BOOT_PKGS variable in the wrapper script is the right thing and matching on all boot pkgs is also imo the right thing, because probably most packages are relevant to the HLS or TH ABI and figuring out which exactly aren’t could be annoying and probably not worth it. (At least that’s what Zubin argued for.)

Then we can argue about how to populate it. I think hardcoding it is simply more work, especially if we hardcode the versions, too. In all the cases (there are two) that I know where the wrapper is populated, the compiletime ghc-pkgs are exactly the boot pkgs, so I wouldn‘t worry about using that to populate the BOOT_PKGS variable …

[hasufell]

Sorry, I still don't follow. This will, imo, still fail if someone builds HLS in a non-nix env for a nix env, even if all boot ABIs match, because during runtime there are more packages in the global DB.

Same with cabal: if you install just one package into your global DB, ABI check will fail, even if all boot packages match.

That's why I'm arguing: this is a nix specific workaround.

[hasufell]

I think I misunderstood the approach a little. I thought we still use the whole global DB during runtime.

Anyway... one could argue this still allows the ABI check to over-match, but building the bindists via the Makefile is not supposed to be done by the user. So it's up to the distributor to not mess it up.

As such, I think this is fine.