Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doctest ghc pkg fix #10057

Closed
wants to merge 3 commits into from
Closed

Conversation

wismill
Copy link
Collaborator

@wismill wismill commented May 27, 2024

Please read Github PR Conventions and then fill in one of these two templates.


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.
  • 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!)

Revival of #8718

Fixes sol/doctest#396.

@sol

@wismill wismill added type: enhancement re: --with-compiler Concerning option `-w` and similar labels May 27, 2024
@wismill
Copy link
Collaborator Author

wismill commented May 27, 2024

I wonder if the key in the augmented ghc --info (currently just ghc) would better follow some format, such as ghc:path or path:ghc. This would allow to extend the concept to other keys/tools.

@geekosaur
Copy link
Collaborator

https://github.com/haskell/cabal/actions/runs/9250116181/job/25443085129?pr=10057#step:19:342
ScopedTypeVariables is included in the GHC2021 extension/language, but must be explicitly enabled for earlier versions.

@wismill
Copy link
Collaborator Author

wismill commented May 27, 2024

@geekosaur good catch, thanks! I went with TypeApplications which is more elegant IMHO.

@mpickering
Copy link
Collaborator

mpickering commented May 28, 2024

This patch seems to rely on some custom behaviour of doctest when acting as a GHC. The "ghc" variable is not (at the moment) part of ghc --info.

Perhaps a more principled way is to look at the LibDir variable and look in ../bin for ghc-pkg?

@wismill
Copy link
Collaborator Author

wismill commented May 28, 2024

@mpickering I am merely reviving #8718, so I lack the knowledge for the best approach. The use of this key ghc is explained at this comment. And:

AFAIK, e.g. hie-bios / hls use the same approach to get the correct flags for a source file + other tools could benefit from it too.

but:

I'm not aware that they would do anything in that regard right now.

I do not know the internals of these tools.

Perhaps a more principled way is to look at the "LibDirvariable and look in../bin` for ghc-pkg?

@mpickering Not expert, so I will wait that experts settle down on the best approach.

@sol
Copy link
Member

sol commented May 29, 2024

@mpickering I am merely reviving #8718, so I lack the knowledge for the best approach. The use of this key ghc is explained at this comment. And:

AFAIK, e.g. hie-bios / hls use the same approach to get the correct flags for a source file + other tools could benefit from it too.

but:

I'm not aware that they would do anything in that regard right now.

I do not know the internals of these tools.

Perhaps a more principled way is to look at the "LibDirvariable and look in../bin` for ghc-pkg?

@mpickering Not expert, so I will wait that experts settle down on the best approach.

To clarify, what I was saying is that hie-bios / hls use the same cabal repl --with-ghc=... trick to get the correct command-line options for GHC, AFAIK. my second point was that they could then also augment the --info output with the ghc key to benefit from this patch as well.

@sol
Copy link
Member

sol commented May 29, 2024

I think the cleanest solution would be to add a command-line option --with-repl so that you can invoke doctest with

$ cabal repl --with-repl=doctest

instead of

$ cabal repl --with-ghc=doctest

cabal then should also allow for both options to be specified at the same time, e.g.:

$ cabal repl --with-ghc=ghc-9.8.2 --with-repl=doctest

@mpickering
Copy link
Collaborator

Another solution to consider is using external commands to provide a cabal doctest command which hides with --with-ghc stuff from users. So you can just pass --with-ghc=... --with-ghc-pkg=....

For example: https://github.com/mpickering/cabal-doctest-demo/blob/master/cabal-doctest

You could implement this as a Haskell executable rather than a bash script and install both doctest and cabal-doctest when the user writes cabal install doctest.

I can't immediately imagine how augmenting --info like this will go wrong but it is definitely something ad-hoc which will cause head scratching in 10 years time when doctest doesn't exist in the same form. If this is the decided implementation strategy then please document how this works in the user manual and explain why the implementation is like this.

@wismill
Copy link
Collaborator Author

wismill commented May 30, 2024

cabal then should also allow for both options to be specified at the same time, e.g.:

$ cabal repl --with-ghc=ghc-9.8.2 --with-repl=doctest

@sol I think this is too complicated. doctest is currently compiled for a specific version of GHC, so providing this new option would still require that the user provide the correct pair (GHC, doctest), instead of cabal deducing it. I mean, it’s not really better than the current workaround you proposed:

$ cabal repl --with-ghc=doctest --with-hc-pkg=ghc-pkg-9.8.2

The key it that we want cabal to deduce the correct path for other executables.


Perhaps a more principled way is to look at the LibDir variable and look in ../bin for ghc-pkg?

@mpickering looks a better option 👍

  • Is this entry inconsistent across the GHC version currently supported by cabal? i.e. missing entry, other format or semantics?
  • Are there settings where this would not work?

If one of the answer is yes, we should add a fallback with the current behavior. So the lookup would be:

  1. Try the path <LibDir>/../bin, where <LibDir> comes from ghc --info.
  2. If no target executable is found, try with the path of the compiler, either from --with-{ghc,compiler} or found in PATH.

This would solve the issue for doctest and provide a new feature for creative uses of cabal.

@sol
Copy link
Member

sol commented May 30, 2024

Perhaps a more principled way is to look at the LibDir variable and look in ../bin for ghc-pkg?

@mpickering looks a better option 👍

I'm definitely not opposed, will probably be more robust than what Cabal currently does.

  • Is this entry inconsistent across the GHC version currently supported by cabal? i.e. missing entry, other format or semantics?

Prior to GHC 9.4, apparently that would need to be ../../bin instead of ../bin.

If one of the answer is yes, we should add a fallback with the current behavior. So the lookup would be:

  1. Try the path <LibDir>/../bin, where <LibDir> comes from ghc --info.
  2. If no target executable is found, try with the path of the compiler, either from --with-{ghc,compiler} or found in PATH.

This sounds good to me 👍

cabal then should also allow for both options to be specified at the same time, e.g.:

$ cabal repl --with-ghc=ghc-9.8.2 --with-repl=doctest

@sol I think this is too complicated. doctest is currently compiled for a specific version of GHC, so providing this new option would still require that the user provide the correct pair (GHC, doctest), instead of cabal deducing it. I mean, it’s not really better than the current workaround you proposed:

$ cabal repl --with-ghc=doctest --with-hc-pkg=ghc-pkg-9.8.2

The key it that we want cabal to deduce the correct path for other executables.

There's still the issue that when doctest acts as a proxy for GHC, then theghc-paths package is not complied correctly (not sure if any other package are affected). For that reason a user should always do a clean build first, before invoking cabal repl --with-ghc=doctest.

--with-repl would make this more robust, with the disadvantage that now the user will be responsible to make sure that versions of doctest and ghc fit.

@wismill
Copy link
Collaborator Author

wismill commented May 30, 2024

Prior to GHC 9.4, apparently that would need to be ../../bin instead of ../bin.

- Prior to GHC 9.4, apparently that would need to be `../../bin` instead of `../bin`.
+ Starting from GHC 9.4, apparently that would need to be `../../bin` instead of `../bin`.
-- ghc-9.2 --info
 ("LibDir","~/.ghcup/ghc/9.2.8/lib64/ghc-9.2.8") 
-- ghc-9.4 --info
 ("LibDir","~/.ghcup/ghc/9.4.8/lib64/ghc-9.4.8/lib") 

It doesn’t look like this entry is very stable. So we would have to check yet another path. Plus I would say it is safer to check subdirs than parent dirs.

The current solution with the ghc entry seems easier now.

@mpickering
Copy link
Collaborator

@wismill I imagine in 9.2 the path is $libdir/bin and in 9.4 the path is $libdir/../bin.

Are you both against my proposed solution of using an external cabal command? That seems to solve many issues to me.

@mpickering
Copy link
Collaborator

This issue also reminds me of something similar where when you use a cross compiler then cabal will fail to find ghc-pkg due to the prefix. Perhaps yet another solution is to try to make the detection logic account for a prefix and for doctest to also install a suitable wrapper script which would point to the right ghc-pkg.

@wismill
Copy link
Collaborator Author

wismill commented May 30, 2024

Are you both against my proposed solution of using an external cabal command? That seems to solve many issues to me.

@mpickering sorry, we answered both at the same time and I did not see your proposal.

I do not think the cabal-doctest file you link would solve the issue though: isn’t cabal still picking whatever ghc-pkg is in PATH? How would you select a different GHC version, apart by using say ghcup set ghc XXX then running you script?

Or do you mean we somehow reverse the roles: e.g. a bash script cabal-doctest that:

  • Has as first argument the doctest executable, the rest will be passed to cabal as is;
  • Runs e.g. doctest --info to query the various executables it needs to pass to --with-xxx, in order to not depend on cabal’s resolution for these; say it stores them in $DOCTEST_xxx variables;
  • Finally runs e.g.:
    cabal repl --with-compiler="$1" --with-hc-pkg="$DOCTEST_PKG" "${@:2}"
    Note that there might be other arguments needed to pass to cabal to avoid incorrect path resolution.

This is simply automatizing the workaround and leave the responsibility to doctest to configure cabal args. It sounds like a proper solution:

cabal-doctest doctest-9.2
cabal-doctest path/to/doctest --some --extra=arguments --for-cabal

The extra arguments could be filtered to avoid overriding. This script would be agnostic to doctest/ghc/cabal versions. We could even have a stack variant I guess.

@sol
Copy link
Member

sol commented May 30, 2024

@wismill I imagine in 9.2 the path is $libdir/bin and in 9.4 the path is $libdir/../bin.

Depending on whether you want the wrapper script or the binary it is:

  • GHC 9.2 and earlier $libdir/bin (binary) and $libdir/../../bin (wrapper script)
  • GHC 9.4 and later $libdir/../bin (binary) and $libdir/../../../bin (wrapper script)

At least for 9.2, I think you need to use the wrapper script. For later versions the binary seems to works, not sure exactly how it finds the libdir, but it seems to work from what I have tried. Still not entirely sure whether you want to rely on that, maybe always going with the wrapper script is the safer bet.

From the information that I have right now, going by libdir instead of executable path seems more robust. At least I can't come up with any immediate downsides. That's why as of now I would be in favor of that approach.

Are you both against my proposed solution of using an external cabal command? That seems to solve many issues to me.

I think an external cabal subcommand could be neat. If I can choose, then I want all three:

  1. A more reliable way for Cabal to determine ghc-pkg by looking at LibDir
  2. A --with-repl option so that doctest does not have to proxy calls to ghc
  3. An external cabal subcommand as sugarcoating for the end user

@mpickering
Copy link
Collaborator

@wismill

The script I linked takes the approach of first installing a version of doctest which matches whichever ghc you are using for your project (in setup_doctest). (So you don't have to manually install doctest for many different ghc versions)

Then the second part automates the --with-repl passing. It should also probably query ghc-pkg in setup-doctest and then pass --with-ghc-pkg here as well.

With the external command support in the new version of cabal you can run this as cabal doctest.

Another idea would be that you keep the installation story for doctest the same as it is now (ie you manually install doctest for the version you want), but it also installs a cabal-doctest wrapper which hard-codes in the paths to the right ghc-pkg version in the wrapper.

Your idea is another possibility yes, that isn't something I thought of. It overall seems quite flexible using an external command like this.

@wismill
Copy link
Collaborator Author

wismill commented May 31, 2024

@mpickering @sol I now fully understood the operating and interest of cabal external command. See sol/doctest#424 for an implementation of cabal-doctest with some of the ideas developped previously.

cabal install doctest --program-prefix "-9.4" --with-compiler ghc-9.4
cabal install doctest --program-prefix "-9.6" --with-compiler ghc-9.6

# This will not depend on the ghc version in PATH
cabal doctest-9.4
cabal doctest-9.6

I think this is clean solution, although a the mechanism presented in the current MR or a “custom programs resolver” is also interesting.

@wismill
Copy link
Collaborator Author

wismill commented May 31, 2024

There is a minor issue: I need to hit “Enter” to finish the program when invoked with cabal doctest, but not when invoked as CABAL=cabal doctest.

Not sure what is causing this. Could it from the cabal side?

@mpickering
Copy link
Collaborator

@wismill Perhaps something to investigate in the cabal side, make a ticket? I can look into it after next week (I am on holiday).

@wismill
Copy link
Collaborator Author

wismill commented Jul 12, 2024

@sol is working on a proper solution for doctest in sol/doctest#425.

@wismill wismill closed this Jul 12, 2024
@sol sol mentioned this pull request Jul 12, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re: --with-compiler Concerning option `-w` and similar type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doctest in multi-GHC setting
4 participants