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.

Sign up for GitHub

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

Jump to bottom

Fix ghcide handling project root #2543

Merged
merged 5 commits into from
Jan 4, 2022
Merged

Fix ghcide handling project root #2543

merged 5 commits into from
Jan 4, 2022

Conversation

drsooch
Copy link
Collaborator

@drsooch drsooch commented Dec 26, 2021 •
edited

NOTE: The two commits in this PR are distinct options for handling this issue.

This PR provides an alternate way to grab the project root/current working directory.

Prior to this PR the relative filepath "." was hard-coded for both
Db and Custom Commands. This results in inconsistent behavior with how
HLS derives it's hiedb location.

This new argument to the internal Ghcide Arguments, maps from the
executable Arguments argsCwd or by grabbing the current working
directory. If the user provides an option to --cwd we need to make
sure we make that filepath absolute.

@drsooch
Add new Ghcide Argument to track Project Root
89ed1a6 
This commit provides an alternate way to grab the project root/current working directory.

Prior to this commit the relative filepath "." was hard-coded for both
Db and Custom Commands. This results in inconsistent behaviour with how
HLS derives it's hiedb location.

This new argument to the internal Ghcide Arguments, maps from the
executable Arguments `argsCwd` or by grabbing the current working
directory. If the user provides an option to `--cwd` we need to make
sure we make that filepath absolute.

Finally, inside the command handler, if necessary, we will grab the
current working directory. We cannot provide a suitable default for this
argument, therefore we leave it as a `Maybe FilePath`, even though this
path should never be taken.
@drsooch drsooch requested a review from wz1000 December 26, 2021 18:31
@pepeiborra
Copy link
Collaborator

This PR provides an alternate way to grab the project root/current working directory.
Prior to this PR the relative filepath "." was hard-coded for both Db and Custom Commands. This results in inconsistent behavior with how HLS derives it's hiedb location.

For my benefit, could you explain the inconsistency or share an example of what could go wrong?

@drsooch
Copy link
Collaborator Author

drsooch commented Dec 26, 2021

This PR provides an alternate way to grab the project root/current working directory.
Prior to this PR the relative filepath "." was hard-coded for both Db and Custom Commands. This results in inconsistent behavior with how HLS derives it's hiedb location.

For my benefit, could you explain the inconsistency or share an example of what could go wrong?

Yes, I was just taking a look at the state of go-to documentation for external dependencies (#708). I was attempting to follow what @wz1000 had done, and noticed that when ghcide hiedb index ... is run it was loading the wrong project hiedb.

This effectively nullifies the point of indexing, as HLS will never pick up the indexed files. The actual call to getHieDbLoc is always done with "." which will hash to the same location each time (not the current project root). So it doesn't really effect anything that I'm aware but if anyone uses ghcide outlined by @wz1000 it should work as expected (sort of).

@wz1000 wz1000 mentioned this pull request Dec 27, 2021
Merged
wz1000
wz1000 approved these changes Dec 28, 2021
View reviewed changes
Copy link
Collaborator

@wz1000 wz1000 left a comment

Choose a reason for hiding this comment

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

Thanks!

pepeiborra
pepeiborra requested changes Dec 28, 2021
View reviewed changes
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Please fix lsp --cwd before merging. It would be great to get tests as well in a later PR

michaelpj
michaelpj reviewed Dec 28, 2021
View reviewed changes
ghcide/exe/Main.hs
-- when running commands directly from GHCIDE we need to provide the ABSOLUTE path to the project root (that's what HLS uses)
argsCwd <-case argsCwd of
Nothing -> IO.getCurrentDirectory
Just root -> IO.setCurrentDirectory root >> IO.getCurrentDirectory
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a little obscure: why not just call makeAbsolute on root? This relies on you knowing that getCurrentDirectory returns an absolute path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right and I originally did it that way, but I wanted to match the semantics found throughout out the codebase. For example, in Development.IDE.Main.defaultMain all of the argCommands use the same maybe IO.getCurrentDirectory return rootPath expression to get the project root.

@drsooch drsooch force-pushed the fix-ghcide-handling-project-root branch from 0d80826 to 875a1ce Compare December 29, 2021 04:57
@drsooch
Copy link
Collaborator Author

drsooch commented Dec 29, 2021

@pepeiborra just curious where does LSP.resRootPath get set? If I'm following the code correctly we should have access to the new argsProjectRoot argument in runLanguageServer, and it should (?) get mapped to the LSP.LanguageContextEnv in some way.

@drsooch
Copy link
Collaborator Author

drsooch commented Dec 29, 2021

Also just as an aside would it be worth exploring using Typed Paths? I know there are a few packages out there to use, this way it's possible to remove all confusion as to what state we are in?

@drsooch drsooch force-pushed the fix-ghcide-handling-project-root branch 2 times, most recently from 09d68cc to 875a1ce Compare December 29, 2021 20:49
@pepeiborra
Copy link
Collaborator

@pepeiborra just curious where does LSP.resRootPath get set? If I'm following the code correctly we should have access to the new argsProjectRoot argument in runLanguageServer, and it should (?) get mapped to the LSP.LanguageContextEnv in some way.

I think it's (optionally) set by the LSP initialize message.

@Anton-Latukha
Copy link
Collaborator

Is this ready to be merged?

@pepeiborra pepeiborra merged commit 3983d97 into haskell:master Jan 4, 2022
@Anton-Latukha
Copy link
Collaborator

Thanks, @drsooch for the work.

@drsooch drsooch deleted the fix-ghcide-handling-project-root branch August 20, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj left review comments

@pepeiborra pepeiborra pepeiborra approved these changes

@wz1000 wz1000 wz1000 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@drsooch @pepeiborra @Anton-Latukha @michaelpj @wz1000