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

Map LSP paths #8026

Closed
wants to merge 7 commits into from
Closed

Map LSP paths #8026

wants to merge 7 commits into from

Conversation

weirdan
Copy link
Collaborator

@weirdan weirdan commented May 29, 2022

Allows mapping of paths passed over LSP protocol. This is useful where you need
to run Psalm language server inside a container where project paths differ from
those on host machine.

Partly addresses #6919 (for LSP only)

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label May 29, 2022
@weirdan weirdan force-pushed the map-lsp-paths branch 2 times, most recently from 4c16ee9 to d1e58b4 Compare May 29, 2022 03:59
@weirdan
Copy link
Collaborator Author

weirdan commented Nov 27, 2022

4.x branch is closed now as we prepare for the 5.0 release. Please target the master branch instead.

@orklah
Copy link
Collaborator

orklah commented Nov 27, 2022

On your own PR :D nice!

@orklah
Copy link
Collaborator

orklah commented Dec 20, 2022

Closing as this targets 4.x which is closed. Please reopen on master going forward

@orklah orklah closed this Dec 20, 2022
@m-graham
Copy link

@weirdan are you planning on targeting the master branch with your original merge request?

@weirdan weirdan reopened this Jan 23, 2023
@weirdan weirdan changed the base branch from 4.x to master January 23, 2023 20:28
@weirdan
Copy link
Collaborator Author

weirdan commented Jan 23, 2023

@m-graham are you using this branch?

@m-graham
Copy link

Hi @weirdan,

I work from within a dev container and realized after sending my previous message that I could still use the VSCode plugin by specifying that it uses a TCP connection.

I'm not actively using this branch.

@weirdan
Copy link
Collaborator Author

weirdan commented Jul 21, 2023

@tm1000 as our LSP expert, do you think it would be better to use the following approach instead?

  1. Capture rootUri sent from the client (we do not support multiple workspace folders)
  2. Assume the server cwd in the container is the same directory as rootUri on the host machine (e.g. if we're running in /var/www/ folder in the container and received rootUri: file:///home/weirdan/src/myproject, it means /home/weirdan/src/myproject folder on the host machine was mounted as /var/www into the container).
  3. Having both of these pieces of info use them to translate between /var/www/src/filename.php and file:///home/weirdan/src/myproject/src/filename.php when communicating with the client.

This works for me with my local patched Psalm, and has an added benefit of not requiring any configuration.

@tm1000
Copy link
Contributor

tm1000 commented Jul 21, 2023

  1. Capture rootUri sent from the client (we do not support multiple workspace folders)

So technically I think we could support multiple workspace folders from the extension but it would just launch separate Psalm instances for each workspace. Thus I think what you propose still works

This works for me with my local patched Psalm, and has an added benefit of not requiring any configuration.

So I think this works. The way I'm doing it now for all of my docker containers is I actually have psalm running outside of the container so it runs locally on the system. I'm not saying that works for for everyone. But that way I dont run into docker networking issues like this.

I like that you don't have to configure anything but I believe someone out there will want/need to be able to configure that. Is it possible to allow that? I think bare minimum it needs to be configurable, not that they need to change it from day 1

@weirdan
Copy link
Collaborator Author

weirdan commented Jul 21, 2023

we could support multiple workspace folders from the extension but it would just launch separate Psalm instances for each workspace.

I believe that's already the case, see psalm/psalm-vscode-plugin#184

I believe someone out there will want/need to be able to configure that. Is it possible to allow that?

Possibly. I'm wouldn't go with the config file approach (as in this PR) though, it's too much hassle to set up for end users. The way I see it, it would be limited to LSP use case, so either a CLI switch (e.g. --map-folder=/home/weirdan/src/project:/var/www/) or LS initialization options entry (received during initialize request in InitializeParams.initializationOptions element). CLI switch looks like it would be easier to implement both on the server and client side.

This won't immediately allow running Psalm in Docker containers with psalm-vscode-plugin though. Unlike some other plugins that just allow one to specify the command to execute, with psalm-vscode-plugin it's not possible to specify something like docker-compose exec -T php php -dmemory_limit=-1 vendor/bin/psalm-language-server --verbose.

@tm1000
Copy link
Contributor

tm1000 commented Jul 24, 2023

we could support multiple workspace folders from the extension but it would just launch separate Psalm instances for each workspace.

I believe that's already the case, see psalm/psalm-vscode-plugin#184

Oh. and I merged it too! I forgot

I believe someone out there will want/need to be able to configure that. Is it possible to allow that?

Possibly. I'm wouldn't go with the config file approach (as in this PR) though, it's too much hassle to set up for end users. The way I see it, it would be limited to LSP use case, so either a CLI switch (e.g. --map-folder=/home/weirdan/src/project:/var/www/) or LS initialization options entry (received during initialize request in InitializeParams.initializationOptions element). CLI switch looks like it would be easier to implement both on the server and client side.

The CLI switch is fine as usually most of those I then store in the Configuration class in the LSP itself which means they are easily changeable on the fly from vscode (or other LSPs)

This won't immediately allow running Psalm in Docker containers with psalm-vscode-plugin though. Unlike some other plugins that just allow one to specify the command to execute, with psalm-vscode-plugin it's not possible to specify something like docker-compose exec -T php php -dmemory_limit=-1 vendor/bin/psalm-language-server --verbose.

Yes this is true it will require additional settings as I have seen in a few other plugins/extensions. I will work on that in the upcoming weeks along with sending configuration now that we support that in the LSP itself

@weirdan
Copy link
Collaborator Author

weirdan commented Jul 24, 2023

Superseded by #10033

@weirdan weirdan closed this Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants