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

[WIP] parallelising ghcide tests #4364

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

komikat
Copy link
Contributor

@komikat komikat commented Jul 21, 2024

Using getCurrentDirectory and setCurrentDirectory makes it difficult to run tests in parallel since one thread modifying the global CWD variable would affect all other test threads. The goal of this PR is to deliberate on ways to avoid using the aforementioned functions (by making paths absolute) and have type guarantees for these absolute paths.

Note [Root Directory] from Development.IDE.Core.Shake

We keep track of the root directory explicitly, which is the directory of the project root.
We might be setting it via these options with decreasing priority:

  1. from LSP workspace root, resRootPath in LanguageContextEnv.
  2. command line (--cwd)
  3. default to the current directory.

Using getCurrentDirectory makes it more difficult to run the tests, as we spawn one thread of HLS per test case.
If we modify the global Variable CWD, via setCurrentDirectory, all other test threads are suddenly affected,
forcing us to run all integration tests sequentially.

Also, there might be a race condition if we depend on the current directory, as some plugin might change it.
e.g. stylish's loadConfig. #4234

But according to https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
The root dir is deprecated, that means we should cleanup dependency on the project root(Or $CWD) thing gradually,
so multi-workspaces can actually be supported when we use absolute path everywhere(might also need some high level design).
That might not be possible unless we have everything adapted to it, like 'hlint' and 'evaluation of template haskell'.
But we should still be working towards the goal.

We can drop it in the future once:

  1. We can get rid all the usages of root directory in the codebase.
  2. LSP version we support actually removes the root directory from the protocol.

Thank you @fendor for mentioning haskell/cabal#9718 which seems to be doing something similar albeit for completely different reasons.

  • Consider using a typed path library #4277
    ref Path newtype declaration #4372
  • remove references to setCurrentDirectory where possible
    • hls-stylish-plugin
  • investigate using lspEnv's resRootPath as the source of truth for the root directory.
    Details

    `doInitialize` uses the `defaultRoot` parameter, which is passed down from `setupLSP` (`argsProjectRoot`) in `Arguments{..}` in `defaultArguments` which seems to be taken from `getCurrentDirectory`.

@komikat
Copy link
Contributor Author

komikat commented Jul 25, 2024

Tracking setCurrentDirectory usage

  • hls-test-utils/src/Test/
    runSessionWithTestConfig uses keepCurrentDirectory if testShiftRoot

the loadConfig function calls setCurrentDirectory and
getCurrentDirectory for recursively searching the
current dir for `.stylish_haskell.yaml`, this has
been replaced by a function which directly chooses
the parent directory of the file as the currentDirectory,
the `search` function nonetheless looks recursively outwards.

TODO: cabalLanguageExtensions parsing support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant