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

Update to LemMinX-Maven 0.11.1 #1692

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Conversation

HannesWell
Copy link
Contributor

Copy link

github-actions bot commented Feb 18, 2024

Test Results

  214 files  ±0    214 suites  ±0   16m 50s ⏱️ - 4m 23s
  664 tests ±0    654 ✅ ±0  10 💤 ±0  0 ❌ ±0 
1 328 runs  ±0  1 306 ✅ ±0  22 💤 ±0  0 ❌ ±0 

Results for commit f238991. ± Comparison against base commit 93f9495.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Contributor Author

@mickaelistria, @laeubi any idea, why the EditorTest now fail?
I can reproduce it locally but I understand why there are no markers published (the changes don't affect that). If I import the test pom's in a IDE launched from my m2e workspace the expected error markers show up.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

@HannesWell are the adjustment of the test class required here due to the update? If not I would revert those as a first step.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

One test complains:

Feb 18, 2024 11:51:14 PM org.eclipse.tm4e.core.registry.Registry _doLoadSingleGrammar
WARNING: No grammar source for scope [source.java]
Feb 18, 2024 11:51:14 PM org.eclipse.tm4e.core.internal.rule.RuleFactory _compilePatterns
WARNING: CANNOT find grammar for scopeName [source.java]. I am [[email protected]]
Feb 18, 2024 11:51:14 PM org.eclipse.tm4e.core.internal.rule.RuleFactory _compilePatterns
WARNING: REMOVING BeginEndRule{id=79,name=source.java.embedded.xml} ENTIRELY DUE TO EMPTY PATTERNS THAT ARE MISSING

so maybe the test is not happy about the warnings?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

@HannesWell I have now tried to test this and with a local m2e I see no (language server) error markers generated when I open it in a "pure" maven project, but maybe it is intentional, if I open it with Generic Editor, if I insert some gibberish in the file I do get error markers from language server, so in general something happens...

@mickaelistria
Copy link
Contributor

I do get error markers from language server,

Are those real Maven errors reported by the LS, or just some XML syntax stuff?

@mickaelistria
Copy link
Contributor

I also support that the test shouldn't be changed with the update of lemminx-maven if possible.
Note that Lemminx-Maven doesn't care about underlying IMavenProject and so on from m2e. So the structure of the project doesn't matter. Actually, it's even better to make the test independent of m2e IMavenProject or nature stuff, so when m2e drops usage of natures or becomes more flexible with project structure, the tests would still run.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

I do get error markers from language server,

Are those real Maven errors reported by the LS, or just some XML syntax stuff?

It looks like XML validation errors, I think the best would be to bring this testcase to lemminx-maven to see if it fails there to @mickaelistria can you suggest a test for testing diagnostic reports, I'll try to create one then.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

Okay I noticed that lemminx has actually a test for it pom-without-artifactId.xml / org.eclipse.lemminx.extensions.maven.participants.SimpleModelTest.testMissingArtifactIdError() that seem to work, so somewhere now is something missing in between.

@mickaelistria
Copy link
Contributor

If lemminx-maven was released, it can be assumed to work.
If no Maven info is reported to m2e, it's most likely that Maven-LemMinX is not properly integrated: a dependency may be missing, or conflicting... There are options to debug the LS and enable more logging, or to even pass the necessary flags to debug it (from another IDE instance).

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

@szarnekow can you probably have a look here?
It seems m2e uses latest and greatest lsp4j, lemminx itself seem to have some issues:

if I manually raise the versions in lemminx-maven the test still runs (even though some other can't work due to changed API)

@mickaelistria
Copy link
Contributor

According to your comment, it seems to me that the issue is more that the version of lemminx embedded in Wild Web Developer is not compatible with this release of lemminx-maven.
However, I don't see any change betweem currnet and previous release of lemminx-maven that has affected the code in a way that makes it rely on mutable APIs that might now be broken.

@mickaelistria
Copy link
Contributor

I've checked and my former comment is incorrect: both Wild Web Developer and LemMinX-Maven do include/require the same 0.27 version of lemminx. So it doesn't look like an inconsistency in here.
And the version of LSP4J inside m2e shouldn't really matter because the LS is running as a separate process, and that the version of LSP4J is taken from the lemminx "uber-jar". It's a different execution domain and there is no strong reason that the LSP4J in the IDE leaks into the language sever.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

The only changes are my ones and dependency updates, but its hard to guess if WWD is/could be affected...

@mickaelistria
Copy link
Contributor

Does one of your change in lemminx-maven require a new version of the resolver, newer than what m2e includes and injects in lemminx together with lemminx-maven?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

My changes should be 100% backward compatibile, the only thing I could think of is maybe maven API version I'm using the org.apache.maven.cli.CLIManager.SET_USER_PROPERTY over the (deprecated) org.apache.maven.cli.CLIManager.SET_SYSTEM_PROPERTY ... but would expect that the compiler inline this anyways an lemminx-maven ships with what it compiles for?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

Does one of your change in lemminx-maven require a new version of the resolver, newer than what m2e includes and injects in lemminx together with lemminx-maven?

How does m2e inject maven to leminx?!?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

It seems to be the constant, I have no clue why it compiles / run in lemminx-maven but fails at m2e.... if I recompile with the deprecated one everything works.

@mickaelistria
Copy link
Contributor

How does m2e inject maven to leminx?!?

MavenRuntimeClasspathProvider resolves the list of jars that are added to LemMinX's classpath.

It seems to be the constant, I have no clue why it compiles / run in lemminx-maven but fails at m2e....

Maybe LemMinX-Maven tests uses a different version of Maven that what is in m2e?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

Then it might be that m2e uses an outdated maven-embedder jar I still don't understand why the compiler is not inline this compile time constant!

@HannesWell
Copy link
Contributor Author

@HannesWell are the adjustment of the test class required here due to the update? If not I would revert those as a first step.

They are not, but as written above they don't influence the test-failure.

It seems to be the constant, I have no clue why it compiles / run in lemminx-maven but fails at m2e....

Maybe LemMinX-Maven tests uses a different version of Maven that what is in m2e?

Is eclipse/lemminx-maven#526 related?

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

I think i found it here:

This all is really a bit unfortunate as the forked process seems to use a flat classpath and therefore did not notice if anything is missing even though it is declared on the OSGi level... also it sounds strange that m2e gives jars to the language server if it already seem to be a standalone process why does it not using its own?

@mickaelistria
Copy link
Contributor

also it sounds strange that m2e gives jars to the language server if it already seem to be a standalone process why does it not using its own?

IIRC, the goal was to save some ~30MB, but if it appears that it's overall better to have lemminx-maven come with its own Maven jars despite the extra resource cost, it would be OK to do it.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

I think another option would be to run the server in a (isolated) OSGi framework, that way it can simply use already installed bundles... but I first need to check how WWD start up everything.

@HannesWell
Copy link
Contributor Author

I think another option would be to run the server in a (isolated) OSGi framework, that way it can simply use already installed bundles... but I first need to check how WWD start up everything.

Can't it just run in the same OSGi runtime? IIRC @mickaelistria you presented a while ago that the JDT-LS now also runs in the same process which even allows sharing objects, in case of m2e project information and caches.

@mickaelistria
Copy link
Contributor

I think another option would be to run the server in a (isolated) OSGi framework, that way it can simply use already installed bundles... but I first need to check how WWD start up everything.

The idea has strong technical merits, it was already discussed but the "human resource" aspect weigh more here:
Neither LemMinX nor LemMinx-Maven are tested nor supported in an OSGi environment at the moment. Those might work, but then it would mean what we have in Eclipse land becomes a much differnet distribution from what is tested upstream. As the delta with upstream becomes bigger, it means the maintenance effort would grow as well, which is the opposite of what we want with language servers overall.
I would rather see lemminx-maven duplicating Mavan jars that starting to deal with maintenance of lemminx and lemminx-maven in an OSGi environment.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

I would rather see lemminx-maven duplicating Mavan jars that starting to deal with maintenance of lemminx and lemminx-maven in an OSGi environment.

It seems there is already lemminx-maven-0.11.2-SNAPSHOT-zip-with-dependencies.zip what is currently 11 mb.

@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

I have now created:

this would allow m2e to look at the manifest and either use OSGi resolver or P2 resolver to get know what the lemminx-maven has as a dependency. Lemminx itself seem to be an OSGi bundle already...

@laeubi laeubi merged commit 8c9e028 into eclipse-m2e:master Feb 19, 2024
7 checks passed
@laeubi
Copy link
Member

laeubi commented Feb 19, 2024

Finally everything green 🥇

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.

3 participants