OpenLiberty / liberty-tools-eclipse

Eclipse Public License 2.0
11 stars 8 forks source link

Updates to build file (pom.xml, build.gradle) Maven/Gradle plugin re: config location aren't dynamically picked up by LCLS so no code assist given #461

Closed scottkurz closed 6 months ago

scottkurz commented 11 months ago

USE CASE

The default location for config files: server.xml, server.env, bootstrap.properties are in src/main/liberty/config.

Suppose we use some of the LMP/LGP parms enabling non-default config locations (e.g. <serverEnvFile> as doc'd here).

Ideally the server.env and bootstrap.properties files at the "new" locations (once e.g. pom.xml is saved) will be picked up via LCLS code assist integrated in LTE.

PROBLEM

The LCLS support for this https://github.com/OpenLiberty/liberty-language-server/pull/212 builds upon the workspace/didChangeWatchedFiles LSP operation/message, but this is not yet implemented by the LSP4E library used by LTE for LSP function: https://github.com/eclipse/lsp4e/issues/547

IDEAS TO ADDRESS

  1. Monitor the plugin cfg separately in LTE and have LTE notify LCLS "directly" (i.e. not in a way building upon the lsp4e support) when the plugin cfg XML file has changed. LT Intellij is I believe doing something like this

  2. Perhaps have the LCLS use some other message or event as a trigger to reload the plugin cfg?? ( I don't currently have a suggestion of where to start here or what this would be.)

  3. Perhaps another sort-of-workaround would be to handle 'bootstrap.properties' and 'server.env' no matter the location, e.g. even from any directory not actively configured to LMP/LGP. (Perhaps there's a case to be made to do this anyway, independently of this issue, even).

NOTE

It would also not be terrible to ship without support for this use case. The config won't change too frequently. You'd just have to know what you need to do to "recycle"... e.g. can you open/close the project or do you have to cause the whole LibertyLS process to exit?

We should at least be able to document the workaround if we go with the approach of living with the problem. Perhaps there's something we can do to make the workaround story better if so, short of fully supporting the dynamically-edited file.

scottkurz commented 11 months ago

Newer lsp4e issue: https://github.com/eclipse/lsp4e/issues/807

scottkurz commented 10 months ago

Let's close the milestone noting the POC done here: 786bd17 which shows that we can use an Eclipse-native approach to workaround the lack of lsp4e support.

In the linked commit, we listen for plugin-config.xml changes using an Eclipse-specific listener registration, and we then build up our DidChangeWatchedFilesParams within our LCLS "client" and send it over. The client already has a connection to the LS endpoint, and so the LS can handle this operation & message without needing lsp4e's assistance.

dshimo commented 7 months ago

Bumping this issue as the workspace/didChangeWatchedFiles request is necessary for providing hover/completion support for processing env vars

scottkurz commented 7 months ago

2024-01-24 DXDI UPDATE -

As noted in the last comment, this deserves a fresh look in light of https://github.com/OpenLiberty/liberty-language-server/issues/68. I'm not aware though that the core use case around this issue is any more severe or different because of the new pathways involved in validating Liberty config var refs. As far as I am aware, that assistance will still work OK even with custom config locations as long as those locations are built into a liberty-plugin-config.xml (it is only dynamic updates to that file after LCLS has read/loaded this info that will not be picked up).

So, as noted we have successfully done a POC of a workaround which has Liberty Tools Eclipse do its own watching of a liberty-plugin-config.xml change.

We should check back with Angelo on the plan for the lsp4e issue(s) at some point.

We should also participate in the discussion of that issue to understand what the design looks like in terms of "registering" as a "watched" file (in regards to DidChangeWatchedFiles notifications). Is it going to be based on file extensions or Eclipse content types? Will it be one and the same registration as a registration with the LS itself or will it be a separate, more granular type of registration?

scottkurz commented 7 months ago

As an aside, just looking over the io.openliberty.tools.eclipse.lsp4e plugin.xml...

Not sure it matters but the "io.openliberty.tools.eclipse.org.microprofile.tools.microprofile.mp-properties" type should probably extend "org.eclipse.jdt.core.javaProperties"" like the bootstrap.properties one did instead of the "org.eclipse.core.runtime.text" type