Move the tycho-version into the maven.config and unify build arguments

[HannesWell]

This avoids duplicated specification of build parameters in the different builds.

However I'm a bit concerned because now if one runs the build locally test failures and errors do not let the build fail and one has to look it up on the console.

Alternatively we could use --fail-at-end, but then only projects are build that don't depend on the failed once. On the other hand, a test project should not have dependents? So probably that would be the better option.

[HannesWell]

Oh and it looks like M2E does not consider the maven.config and therefore shows many errors like this
org.eclipse.tycho:tycho-source-plugin:jar:${tycho-version} was not found ...

I think this is #274 and should not be too hard now?

[github-actions]

Unit Test Results

596 tests   590 ✔️  8m 27s ⏱️
  94 suites      6 💤
  94 files        0 ❌

Results for commit d52b667.

♻️ This comment has been updated with latest results.

[mickaelistria]

I don't think it's actually an improvement. Many settings in the script are fine for CI but undesired for a local build (because the test failure/error needs to be visible on a local build).

[HannesWell]

I don't think it's actually an improvement. Many settings in the script are fine for CI but undesired for a local build (because the test failure/error needs to be visible on a local build).

That's right, but therefore I replaced the ignore.failure/error properties by '--fail-at-end' which seems suitable in both cases.

[laeubi]

--fail-at-endis not the same as ignoring test-failures, this is usually used to record all tests but let the build succeed to validate the test results at another stage.

--fail-at-end will fail, but also continue the build if there are compile errors in some modules.

[github-actions]

Test Results

  214 files  ±0    214 suites  ±0   22m 20s ⏱️ + 5m 41s
  664 tests ±0    654 ✅ ±0  10 💤 ±0  0 ❌ ±0 
1 328 runs  ±0  1 306 ✅ ±0  22 💤 ±0  0 ❌ ±0 

Results for commit 2351c6d. ± Comparison against base commit 946c118.

♻️ This comment has been updated with latest results.

[laeubi]

FYI I have adjusted the build here:

• Slightly adjust the build files #1285

so maybe the both approaches should be merged.

[HannesWell]

so maybe the both approaches should be merged.

Merged your changes to and move the specification of the maven.test.error/failure.ignore properties to the CI scripts so that local builds can fail.

[HannesWell]

@laeubi I lost a bit track of your work in regards to support properties in the maven.config and are therefore not sure if it is supposed to work now? When I prepared the update to this PR m2e did not complain in my m2e-dev workspace so I assume this can be merged as soon as we agree upon it?

[laeubi]

Status is it should work in all relevant cases but not sure about leminx and maybe there are still some areas uncovered :-D

[HannesWell]

Status is it should work in all relevant cases but not sure about leminx and maybe there are still some areas uncovered :-D

OK, I can get my workspace into a state where at least m2e shows no errors about a missing tycho.version property.
But as soon as I open a pom, the language server complains about a missing version.

@mickaelistria could you help add support for maven.configs in the language server?
Would it be sufficient to adjust the InitializationOptionsProvider respectively the MavenRuntimeClasspathProvider?
I belive some parts of the magic could happen in.

m2e-core/org.eclipse.m2e.editor.lemminx/src/org/eclipse/m2e/editor/lemminx/MavenRuntimeClasspathProvider.java

Lines 57 to 59 in 69a6425

	LanguageServers.forProject(null).withPreferredServer(definition).excludeInactive()
	.collectAll((w, ls) -> CompletableFuture.completedFuture(ls)).thenAccept(lss -> lss.stream()
	.forEach(ls -> ls.getWorkspaceService().didChangeConfiguration(params)));

Requested changes were applied.

[HannesWell]

Now that we have #1692, we can finally use this.
When trying this out locally I noticed that changes in the maven.config are not immediately picked up by the LS (at least when I hover over the tycho-version property the old value is present).

In m2e internally it seems to work, at least the effective pom shows the updated version.

[laeubi]

You need to edit the file to get the changes reflected I think, or the LS must watch the file for changes but as one no really change this very often I think it is acceptable and the same will happen if you for example change a parent pom file that is currently not opened as a file in the editor.

@mickaelistria is there a way m2e can tell the language server to flush its cache?

[mickaelistria]

is there a way m2e can tell the language server to flush its cache?

No, and as mentioned in previous discussion, the goal is that m2e decides smartly and automatically when to flush its cache by monitoring all the files that participate to resolution (including maven.config) and flush the cache on changes, like it already does for pom.xml files; without involving the client.

[laeubi]

No, and as mentioned in previous discussion, the goal is that m2e decides smartly and automatically when to flush its cache by monitoring all the files that participate to resolution (including maven.config) and flush the cache on changes

I assume you mean lemminx? Where is this "automatic watching" implemented in lemminx? The main point is if two components watching files this double work can maybe avoided.

[mickaelistria]

Indeed, I mean lemminx*=maven*. There is no automatic watching implemeted in lemminx-maven at the moment, because LSP sends notifications for changed files that the clients configures as supported by this LS (currently only pom.xml files).

The main point is if two components watching files this double work can maybe avoided.

lemminx-maven work at best on its own without m2e. If this means duplicated work from m2e to lemminx-maven, then it's worth it.

[HannesWell]

The main point is if two components watching files this double work can maybe avoided.

lemminx-maven work at best on its own without m2e. If this means duplicated work from m2e to lemminx-maven, then it's worth it.

If there is a way to share things between m2e and the LSP I would consider it an advantage if this means less CPU-cycles are burned on a users computer and less memory is used. And having duplicated implementations is something I personally consider relatively bad and would only choose as last or at least later resort. I understand that the LemMinX-LSP want's to be standalone, but I still see an advantage in the things said before. Probably a good compromise has to be found.
I guess m2e can somehow poke the LSP using the approach asked in #848 (comment), can't it?

Anyways I think it would be best to discuss enhancements of the LSP integration in a dedicated ticket.

Now that #1692 is integrated and tested, we can finally eat our own dog-food and use this in m2e after only 146 builds in Jenkins.