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

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 opened this issue Oct 23, 2023 · 5 comments
Assignees
Labels
bug Something isn't working with the product
Milestone

Comments

@scottkurz
Copy link
Member

scottkurz commented Oct 23, 2023

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 OpenLiberty/liberty-language-server#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: eclipse/lsp4e#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 scottkurz added the bug Something isn't working with the product label Oct 23, 2023
@scottkurz scottkurz added this to the 23.0.12 milestone Oct 23, 2023
@scottkurz
Copy link
Member Author

Newer lsp4e issue: eclipse/lsp4e#807

@scottkurz
Copy link
Member Author

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.

@scottkurz scottkurz modified the milestones: 23.0.12, 24.0.3 Nov 10, 2023
@dshimo
Copy link

dshimo commented Jan 23, 2024

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

@scottkurz
Copy link
Member Author

scottkurz commented Jan 24, 2024

2024-01-24 DXDI UPDATE -

As noted in the last comment, this deserves a fresh look in light of OpenLiberty/liberty-language-server#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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working with the product
Projects
Status: Done
Development

No branches or pull requests

4 participants