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

Refactor hls-test-util and reduce getCurrentDirectory after initilization #4231

Merged
merged 109 commits into from
May 27, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented May 14, 2024

What's done

  • Refactor the runSession* family function, properly add TestConfig, runSessionWithTestConfig, as the most generic runSession* function.
  • remove raraly used variants of runSession* functions and replaced by runSessionWithTestConfig.
  • migrate ExceptionTests ClientSettingsTests CodeLensTests CPPTests CradleTests to use the hls-test-utils
  • Only shift to lsp root when current root is different from the lsp root in DefaultMain of ghcide.
  • Remove most usage for getCurrentDirectory(After DefaultMain is called), Only remain those in top level of wrapper and exe, implement Overhauling the HLS testsuite #3736 (comment)

What's remains:

  • golden tests are not using TestConfig, maybe we can refactor them to use TestConfig and cut the number of variants too.
  • Hlint, Stan and some Template haskell tests still depend on current working dir. But they are out of our reach.
  • hie.yaml specify relative files
    cradle:
    direct:
      arguments: ["-unit" ,"@a-1.0.0-inplace"
                 ,"-unit" ,"@b-1.0.0-inplace" 
                 ,"-unit" ,"@c-1.0.0-inplace"
                 ]
    
    

@soulomoon

This comment was marked as outdated.

@@ -848,7 +846,7 @@ getModIfaceFromDiskAndIndexRule recorder =
hie_loc = Compat.ml_hie_file $ ms_location ms
fileHash <- liftIO $ Util.getFileHash hie_loc
mrow <- liftIO $ withHieDb (\hieDb -> HieDb.lookupHieFileFromSource hieDb (fromNormalizedFilePath f))
hie_loc' <- liftIO $ traverse (makeAbsolute . HieDb.hieModuleHieFile) mrow
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The doc said hieModuleHieFile would return a full path, I guess it is not necessary to absolute it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure about this sort of thing. If we need it to be absolute then I think it can be sensible to absolutize it just to be sure... since we don't track it in the types or anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should also introduce an abstraction that tracks whether we expect a path to be absolute or relative?

Copy link
Collaborator Author

@soulomoon soulomoon May 26, 2024

Choose a reason for hiding this comment

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

Seems like a good idea, some places we might need this.
path from [ghc, hiedb, lsp client, cradle].

The ones from ghc should depend on how we load the file into ghc in sessionloader?

Copy link
Collaborator Author

@soulomoon soulomoon May 26, 2024

Choose a reason for hiding this comment

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

It is quite a mess in our codebase, we might need to open an issue and try to sweep and standardize them.

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 14, 2024

Stan is blocking

https://github.com/soulomoon/haskell-language-server/blob/2ebfafcea751adb8d0be9c1ad9c40a4f8771f261/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs#L144

         -- Note that Stan works in terms of relative paths, but the HIE come in as absolute. Without
         -- making its path relative, the file name(s) won't line up with the associated Map keys.

----------------update

Same for hlint, will shift to root only for these test

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 27, 2024

Again, thank your for the reviews and suggestions, expecially since this PR contains a lot of changes, you have done me a great favor of tracking everything down ! I could not do it without your help.

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.

Just some more attention to the Note, I still don't quite get the LSP root thingy, where does this affect us?

ghcide/src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved
Comment on lines 546 to 548
-- But according to https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
-- The root dir is deprecated, but we still need this now,
-- since a lot of places in the codebase still rely on it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I don't quite get this, which root dir is this now?

ghcide/src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved
@soulomoon
Copy link
Collaborator Author

soulomoon commented May 27, 2024

Just some more attention to the Note, I still don't quite get the LSP root thingy, where does this affect us?

LSP root is deprecated, @michaelpj send me the link,
I guess that means we should cleanup dependency on the project root(Or $CWD) thing gradually, so muti workspaces can actually be supported when we use absolute path everywhere.

That might not be possible unless we have everything adapted to it, things like hlint and evaluation of template haskell.
But we should still be working towards the goal.

I'll add this to the note too.

@soulomoon soulomoon enabled auto-merge (squash) May 27, 2024 13:03
@soulomoon soulomoon merged commit 838a51f into haskell:master May 27, 2024
39 checks passed
@michaelpj
Copy link
Collaborator

I think we might want to seriously consider using one of the typed path libraries so we can assert that we have absolute paths everywhere. I agree it's just extremely unclear how you are supposed to deal with relative paths of any kind. There's also the issue that some of the tools we call like cabal are working directory sensitive, I don't know if we currently call cabal with --project-file or whatever we need to get it to run "as if" it were in a specific directory...

@fendor
Copy link
Collaborator

fendor commented May 28, 2024

Since we spawn a new process for cabal, we use https://hackage.haskell.org/package/process-1.6.20.0/docs/System-Process.html#t:CreateProcess to set the working directory for cabal, so that is unaffected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky test status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants