Add --with-repl flag to modify program the "repl" starts with

[mpickering]

Programs like doctest and hie-bios want to use cabal-install in order
to discover the correct options to start a GHC session. Previously they
used the --with-compiler option, but this led to complications since
the wrapper was called for compiling all dependencies and so on, the
shim had to be more complicated and forward arguments onto the user's
version of GHC.

The --with-repl command allows you to pass a program which is used
instead of GHC at the final invocation of the repl. Therefore the
wrappers don't have to deal with being a complete shim but can
concentrate on intercepting the arguments at the end.

This commit removes the special hack to not use response files with
--interactive mode. Tools are expected to deal with them appropiately,
which is much easier now only one invocation is passed to the wrapper.

Fixes #9115

---

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
  • Is the change significant? If so, remember to add significance: significant in the changelog file.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[mpickering]

@sol @fendor Can you please test this patch works for your use case?

TODO

• Documentation
• Changelog

[fendor]

haskell/hie-bios#466

> hie-bios debug src/HIE/Bios.hs                                                                                                       
Root directory:        /home/hugin/Documents/haskell/hie-bios
Component directory:   /home/hugin/Documents/haskell/hie-bios
GHC options:           @/tmp/ghci84462-0.rsp
GHC library directory: CradleSuccess "/home/hugin/.ghcup/ghc/9.6.7/lib/ghc-9.6.7/lib"
GHC version:           CradleSuccess "9.6.7"
Config Location:       No explicit config found
Cradle:                Cradle{ cradleRootDir = "/home/hugin/Documents/haskell/hie-bios", cradleOptsProg = CradleAction: Cabal}
Dependencies:          hie-bios.cabal cabal.project cabal.project.local

The branch passes the test-suite with the cabal binary from this branch.

[sol]

I'm in the process of testing this with doctest and I think ran in the exact issue this PR is gonna address. doctest fails with:

/home/sol/.cache/hie-bios/wrapper-b54f81dea4c0e6d1626911c526bc4e36: line 4: : No such file or directory

What I think happened here:

1. I opened a Haskell file of some project that depends on ghc-path in vim
2. HLS started building dependencies through the wrapper script, putting a corrupted version of ghc-path into the store
3. Later, when I built doctest it picked up the corrupted ghc-path package from the store, resulting in a broken doctest executable.

@fendor please correct me if I'm wrong, but I think you are building dependencies through the wrapper script, right? This is generally not safe! What you should do instead:

1. First ensure that all dependencies are already built, without using the wrapper script (e.g. cabal build --only-dependencies)
2. Only after all dependencies are in place, invoke cabal repl --with-ghc ...
3. As a long-term solution, switch to --with-repl ASAP

HLS is currently corrupting the cabal store and I assume the average users will not have the expertise to figure out what is even going on in a situation like this.

Possibly, cabal should include the canonical path to the used compiler into the package hashes?

[sol]

doctest works with sol/doctest#470.

@mpickering thanks a lot for working on this. I think this will make things quite more robust.

[ulysses4ever]

@sol you should be an approving contributor on this repo iirc. Could you please formally approve this PR if you're happy with it?

[ulysses4ever]

sol you should be an approving contributor on this repo iirc. Could you please formally approve this PR if you're happy with it?

Same to @fendor

[mpickering]

@sol You should be aware that using --with-repl requires the user is using Cabal >=3.15 library. A user may have a custom setup which is built against Cabal-3.14, and that wouldn't support the flag. I added a constraint to ensure that an up-to-date Cabal library version is chosen if required.

[fendor]

LGTM, one nitpick in the documentation

[fendor]

HLS is currently corrupting the cabal store and I assume the average users will not have the expertise to figure out what is even going on in a situation like this.

Why is the current hie-bios approach corrupting the cabal store? Not doubting you, just want to understand what's going on.

EDIT: oh I should have read the full comment, so ghc-paths is the issue? Yeah, that's not great.

[sol]

HLS is currently corrupting the cabal store and I assume the average users will not have the expertise to figure out what is even going on in a situation like this.

Why is the current hie-bios approach corrupting the cabal store? Not doubting you, just want to understand what's going on.

1. If you build ghc-paths through the wrapper script, then ghc-paths is borked as it will produce paths to your wrapper script instead of to ghc.
2. If you later have an install plan that includes ghc-path then the broken package from the store is reused (even if you don't build through any wrapper script!).
3. The only real way to get rid of the broken package is to rm -rf the store directory.

@fendor Does this make sense?

[mpickering]

@sol I'm not sure what the bug is exactly, but it seems like a different bug which is exposed by using the hie-bios wrapper.

In general, relying on the existence of a ghc binary to still be present at the path which was used to build ghc-paths is not going to work. This issue isn't specific to the wrapper. You could imagine building ghc-paths on a different server for example.

[fendor]

It makes sense! I doubt we will change that now since the wrapper is finally going to be phased out, anyway, but I understand now.

[sol]

@sol I'm not sure what the bug is exactly, but it seems like a different bug which is exposed by using the hie-bios wrapper.

It's not a "bug" that you could easily fix, at least not that I could think of. But you could argue that ghc-paths is conceptionally broken, I guess. However this approach has been used by haddock and doctest for a very long time and I'm not aware of a better approach.

simonmar/ghc-paths#4 (comment)

[sol]

@sol you should be an approving contributor on this repo iirc. Could you please formally approve this PR if you're happy with it?

We already have two approvals.

I can still offer to do a proper code review, but that would need to wait until tomorrow. It's late here already.

[geekosaur]

People have been noting that ghc-paths is a horrible, unstable hack that nevertheless gets the job done for years, I think?

[ulysses4ever]

@sol I'd appreciate if you could do a proper review at your convenience, if it's possible during this week. We still have things to do before 3.16 is finalized, so it'd only be wise to get this feature scrutinized as much as we can afford in the time we have.

[ulysses4ever]

@mpickering do you plan to look into the CI failures?

[mpickering]

I have been looking all day, They are some bug on windows

Running: "D:\a\_temp\cabal-testsuite-12064\cabal-multi.dist\work\.\dist\build\x86_64-windows\ghc-9.8.4\cabal-with-repl-exe-0.1.0.0\x\test-exe\build\test-exe\test-exe" "@D:\a\_temp\cabal-testsuite-12064\cabal-multi.dist\tmp\ghc5CDD.rsp"

"My specific executable"

D:\a\_temp\cabal-testsuite-12064\cabal-multi.dist\work\.\dist\multi-out-63884\paths\cabal-with-repl-exe-0.1.0.0-inplace-test-exe: removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:DeleteFile "\\\\?\\D:\\a\\_temp\\cabal-testsuite-12064\\cabal-multi.dist\\work\\dist\\multi-out-63884\\paths\\cabal-with-repl-exe-0.1.0.0-inplace-test-exe": permission denied (The process cannot access the file because it is being used by another process.)

I can't reproduce it locally and I don't know how to fix it. It seems that the test completes, but then when the folder is cleaned up the deletion fails because it thinks the executable is still being used for something.

[ulysses4ever]

Oh my, classic Windows... Well, worst case we could disable these tests on Windows.

[mpickering]

I am going to skip the test on windows as I don't have time to investigate this to the root.

[mpickering]

@jasagredo Perhaps you could try and reproduce this if you had a spare moment. I have used your helpful skipIfCIAndWindows helper.

[jasagredo]

@mpickering I can give it a look, however not in the near future as I am quite busy these weeks 😢

[mpickering]

@mpickering I can give it a look, however not in the near future as I am quite busy these weeks 😢

No problem, just wanted to flag it up in case you were interested.

[ulysses4ever]

FTR I created a label "re: --with-repl" for future issues: re: --with-repl

@mpickering may be it's a good time to set the merge label so that the countdown for last two days starts? If any last-minute reviews come in in that period, any comments they will have will block merging, so we'll still have time to discuss.

[sol]

@mpickering I'm grateful that you put in the work to solve this in a clean way.

The code looks good to me.

1. There's one comment that disrupted my reading flow somehow. I left two alternative suggestions, please feel free to cherry pick whatever you think is useful.
2. There's still some code duplication in the code dealing with constraints. It's not a big issue, but as replAction is already spanning over 270 lines of code, personally I would take every opportunity to keep things in check.
3. Not really excited about response files in general, but I guess they're a necessary evil for Windows/macOS. I left a comment, but no action needed.

[mpickering]

Thanks @sol for the great review. A pleasure doing business with you. I applied the changes you suggested and now assigned to merge.