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

Probe-tools: Print stack ghc version #3093

Merged
merged 5 commits into from
Aug 12, 2022

Conversation

andys8
Copy link
Collaborator

@andys8 andys8 commented Aug 11, 2022

The ghc version on the $PATH is often not relevant for stack projects. The ghc version stack is using is printed in addition.

Example Output

haskell-language-server version: 1.7.0.0 (GHC: 9.2.4) (PATH: [...]/haskell-language-server-wrapper) (GIT hash: 0e74593e8df9665067174ca27543006c9a6ad230)
Tool versions found on the $PATH
cabal:          3.8.1.0
stack:          2.7.5
ghc:            9.2.4
stack ghc:      8.10.7

Discussion

@michaelpj Followup to this comment to sketch the idea for a change. Do you have something like this change in mind?

@michaelpj
Copy link
Collaborator

Yeah, this looks nice! Do we think "stack ghc" is obvious enough? I kind of want to say "ghc provided by stack in this project" but that's a bit long.

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2022

I like the idea in general, but I am worried stack ghc writes something to stdout, killing LSP.

That's why we run it in silent mode in hie-bios: https://github.com/haskell/hie-bios/blob/master/src/HIE/Bios/Cradle.hs#L817

Also, one more issue, since we might trigger a download in this case, hls-wrapper might freeze up with no feedback, since we call it before finding the cradle type: https://github.com/haskell/haskell-language-server/blob/master/exe/Wrapper.hs#L137

And if the command fails for any reason (e.g. can't install GHC because we are out of space), then we won't launch the hls-wrapper lsp service to report errors to the user and it will just crash.

@andys8
Copy link
Collaborator Author

andys8 commented Aug 12, 2022

Thanks for the feedback @fendor . These definitely are valid points to consider.

You mentioned standard output could kill the language server. Is the output and execution of --probe-tools relevant for the execution as language server via --lsp / "hls-wrapper lsp service"?

Update: Okay, I think that's the case in the second reference you posted.

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2022

No, it is not relevant, but truly great for debugging, since you tell people: Copy-paste the logs from VSCode (or editor of choice) and link them. I suppose the issue also asks you to paste haskell-language-server-wrapper --probe-tools, but editors may affect how hls is launched, e.g. by modifying the PATH. I think, the only way to be sure to know which versions were used is to output them during execution as the lsp server.

@andys8
Copy link
Collaborator Author

andys8 commented Aug 12, 2022

What do you have in mind as a solution to the mentioned problems?

  • Calling stack in "silent mode": What did you mean by that? Using stackCradle and runGhcCmd from Cradle
  • Changing the stack command / arguments? But e.g. stack --no-install-ghc path --compiler-exe definitely seems to print unwanted information to stdout.
  • Not adding "stack ghc" at all because it depends on the directory again and could lead to the unwanted side effects?
  • Only adding stack ghc to probe tools if hie has a stack cradle?
  • Not using the same probe tools output when actually.launchinh the server (to avoid risks)?

To repeat the core issue that should be addressed: For stack projects users might post the output of --probe-tools in issues but it doesn't contain the ghc version used in the stack project.

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2022

I am unfortunately not entirely sure how we can solve the issue here. I can imagine, there are multiple valid solutions.

However, I personally like your first/fourth suggestion. We can use probeTools, and if the cradle is a stack cradle (the respective call to it is just three lines later), we can add the compiler information. If the cradle is not a stack cradle, we can basically say that in the logs. That information is helpful as well.

@andys8
Copy link
Collaborator Author

andys8 commented Aug 12, 2022

The more I start digging into the current state, the more I've the feeling I'm trying to reinvent something that already exists 😆

On hls launch there is a lot of debug output, including the ghc version of the cradle. Maybe it only needs to be moved around to be included in probe tools instead?

https://github.com/andys8/haskell-language-server/blob/511527c3e3e2160fe692159af947b814657e55b7/exe/Wrapper.hs#L125-L144

And there even is haskell-language-server-wrapper --project-ghc-version.

So reusing already existing functionality seems to make sense, but comes at the cost of adding or depending on the whole cradle business to the simple implementation of Ide.Version or Ide.Main. While the two commands seem similar from the outside, the place where the information is available is quite deep inside the initialization. Still reusing should be the way to go 😉

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2022

You are not strictly speaking re-inventing something, we are trying to show debug information at a stage, where we don't know the details yet.

The purpose of probe-build-tools is to show what tools and versions we are going to pick to discover the cradle type (e.g. the specifics of how to build our project). Found tools will influence what cradle we choose, e.g. if cabal or ghc are missing, but stack exists -> choose stack. The stack ghc version does not matter for that process.

The cradle type shows you, what has been chosen. Once we know that, we can print more specific information, such as stack ghc version. Before we know we are using a stack-cradle, it actually doesn't make sense at all to print the stack ghc version.

Thus, I think it makes more sense to print cradle specific debug information. Then we have the following stages:

  • Some details about how we are going to choose a cradle
  • Details about a cradle, e.g. chosen ghc version, maybe stuff like that.

Edit: similar to what your current code does in Wrapper.hs.

@andys8
Copy link
Collaborator Author

andys8 commented Aug 12, 2022

I have to admit that I didn't understand and know of the duplication and differences between Ide.Main and Wrapper.Main. It's not clear to me how haskell-language-server-wrapper and haskell-language-server exactly play together and why the user can use both via cmd line.

From what I've seen in the implementation it seems that haskell-language-server-wrapper --probe-tools is what users would use and would from an implementation standpoint easy to add the output aim for.

This commit would use the existing - therefore safer way - to ask for the ghc version the cradle is using: d74f009

What do you think of it?
(I didn't revert the previous implementation yet so it's there for comparison in this PR, but would remove it before merge)

Demo

haskell-language-server-wrapper --probe-tools
haskell-language-server version: 1.7.0.0 (GHC: 9.2.4) (PATH: /home/andreas/dev/repository/haskell-language-server/.stack-work/install/x86_64-linux/55c53cd9269c4a2809da5f988f853f231b3f377e3f974e54bb63c400e6f6f0ac/9.2.4/bin/haskell-language-server-wrapper) (GIT hash: 511527c3e3e2160fe692159af947b814657e55b7)
Tool versions found on the $PATH
cabal:          3.8.1.0
stack:          2.7.5
ghc:            9.2.4

Tool versions found by cradle
ghc:            8.10.7

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2022

I think the idea works well

The ghc version on the $PATH is often not relevant for stack projects.
The ghc version stack is using is printed in addition.
@andys8 andys8 force-pushed the probe-tools-print-stack-ghc-version branch from b04d7b5 to f66e9e8 Compare August 12, 2022 12:27
@andys8 andys8 marked this pull request as ready for review August 12, 2022 12:27
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comments

exe/Wrapper.hs Outdated Show resolved Hide resolved
exe/Wrapper.hs Outdated Show resolved Hide resolved
Co-authored-by: fendor <fendor@users.noreply.github.com>
andys8 added a commit to andys8/haskell-language-server that referenced this pull request Aug 12, 2022
@andys8 andys8 force-pushed the probe-tools-print-stack-ghc-version branch from 740cca9 to 6cfb608 Compare August 12, 2022 13:19
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fendor fendor added the merge me Label to trigger pull request merge label Aug 12, 2022
@mergify mergify bot merged commit e20b026 into haskell:master Aug 12, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
* Probe-tools: Print stack ghc version

The ghc version on the $PATH is often not relevant for stack projects.
The ghc version stack is using is printed in addition.

* Probetools: cradle ghc version added to wrapper

* Revert stack ghc changes to Ide.Main

* Update exe/Wrapper.hs

Co-authored-by: fendor <fendor@users.noreply.github.com>

* Probe tools: Print version with padded spaces

Addressing <haskell#3093 (comment)>

Co-authored-by: fendor <fendor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants