Remove 'de.itemis.xtext.antlr.feature' from target files by HannesWell · Pull Request #2219 · eclipse-xtext/xtext

HannesWell · 2023-04-15T05:55:07Z

As discussed in #2213 (comment), the de.itemis.xtext.antlr.feature should not be in the target-files.
It is only used for developer convenience in the xText development workspace and already added to that by the Xtext Oomph-setup.

cdietrich · 2023-04-15T06:06:03Z

it looks like its needed anyway cause tycho insists on resolving optional dependcies

HannesWell · 2023-04-15T08:18:54Z

it looks like its needed anyway cause tycho insists on resolving optional dependcies

That's unexpected. Maybe with the new resolver in Tycho 3/4 this is handled more gracefully.
I suggest to keep this open until Tycho 3 is used for xtext and then try it again?

LorenzoBettini · 2023-04-15T17:00:59Z

@HannesWell, if you have time, could you please check locally whether with Tycho 3.0.4 there's no error in TP resolution? I guess validate phase should be enough.

HannesWell · 2023-04-16T21:54:57Z

@HannesWell, if you have time, could you please check locally whether with Tycho 3.0.4 there's no error in TP resolution? I guess validate phase should be enough.

Tried it with Tycho 3.0.4 locally and the same error occurred.

@laeubi do you have any idea, why we get the resolution error below with that require-bundle entry (with and without the reexport)?
Require-Bundle: de.itemis.xtext.antlr;bundle-version="2.0.0";resolution:=optional;visibility:=reexport
I would expect Tycho to ignore optional dependencies if absent.

 [INFO] Resolving target definition file:/home/jenkins/agent/workspace/xtext_PR-2219/org.eclipse.xtext.xtext.generator/../xtext-r202203.target for environments=[macosx/cocoa/x86_64, win32/win32/x86_64, linux/gtk/x86_64], include source mode=honor, execution environment=StandardEEResolutionHints [executionEnvironment=OSGi profile 'JavaSE-11' { source level: 11, target level: 11}], remote p2 repository options=org.eclipse.tycho.p2.remote.RemoteAgent@1055d261...
 [INFO] Resolving dependencies of MavenProject: org.eclipse.xtext:org.eclipse.xtext.xtext.generator:2.31.0-SNAPSHOT @ /home/jenkins/agent/workspace/xtext_PR-2219/org.eclipse.xtext.xtext.generator/pom.xml
 [INFO] {osgi.os=macosx, osgi.ws=cocoa, org.eclipse.update.install.features=true, osgi.arch=x86_64}
 [ERROR] Cannot resolve project dependencies:
 [ERROR]   Software being installed: org.eclipse.xtext.xtext.generator 2.31.0.qualifier
 [ERROR]   Missing requirement: org.eclipse.xtext.xtext.generator 2.31.0.qualifier requires 'osgi.bundle; de.itemis.xtext.antlr 2.0.0' but it could not be found

This happens also with the new resolver.

laeubi · 2023-04-17T05:07:24Z

I would expect Tycho to ignore optional dependencies if absent.

If you want to ignore them simply remove them ;-)

Otherwise there are no "optional dependencies" in terms of a build-tool, because in contrast to the runtime where you probably do not execute all code pathes (or can handle the absence of a service / class / resource / ...) at compile time you need to compile everything.

Still I have created already:

Optional requirements fail the test resolve operation eclipse-tycho/tycho#2309

cdietrich · 2023-04-17T05:08:41Z

=> we have to keep them

HannesWell · 2023-04-17T06:36:03Z

Otherwise there are no "optional dependencies" in terms of a build-tool, because in contrast to the runtime where you probably do not execute all code pathes (or can handle the absence of a service / class / resource / ...) at compile time you need to compile everything.

Indeed, makes sense in general.
But in this case the classes are only accessed reflectively and therefore don't have to be present at compile-time either:
https://github.com/eclipse/xtext/blob/93acb221e6fabbc2d09e687365bfbbe4bdf29ce5/org.eclipse.xtext.generator/src/org/eclipse/xtext/generator/parser/antlr/AntlrToolFacade.java#L29

Wouldn't it make sense to use DynamicImport-Package in this case?
@cdietrich how can I test if that still works at runtime.

laeubi · 2023-04-17T06:37:16Z

But in this case the classes are only accessed reflectively and therefore don't have to be present at compile-time either:

Then one should use Dynamic-ImportPacakge instead ...

That feature is only used for developer convenience in the Xtext development workspace and already added to that by the Xtext Oomph-setup. Use 'DynamicImport-Package' instead of Required-Bundle (or Import-Package) to make Xtext still compile without that dependency. Since the classes form 'de.itemis.xtext.antlr' are accessed reflectively they are not needed at compile-time.

cdietrich · 2023-04-17T06:41:39Z

i dont know the
Dynamic-ImportPacakge
do you
@szarnekow

szarnekow · 2023-04-17T06:43:07Z

I'm aware of the concept of Dynamic-ImportPackage but have never used it. Conceptionally it should do the trick.

laeubi · 2023-04-17T06:43:36Z

cdietrich · 2023-04-17T06:44:36Z

=> would need to be tested if it works.

HannesWell · 2023-04-17T06:50:10Z

=> would need to be tested if it works.

Yes. Can you tell me an elegant way to do that? Then I can test it this evening.

In general, if this should be worked out in a clean way there could be a general WhatEverTool interface in the generator Plugin which is implemented by the itemes-implemention for example as declarative service. Then at the place where now the class is looked up, the OSGi framework could be asked for that service.
If that should work outside of OSGi, the Service-Loader mechanism of Java could be used.
But both would require a bit more rework on both sites (which I'm currently not eager to do^^).

cdietrich · 2023-04-17T06:52:04Z

the dependeny is in xtext.xtext.generator.
so i expect if the itemis plugin is in tp
and you create a new xtext project wizard
and then run/debug the workflow file,
it should not download antlr but load the antlr.generator from classpath instead

HannesWell · 2023-04-18T16:39:53Z

I tested it with the example dsl xtext project from the xtext dev workspace and there I see the following print-out:

581  INFO  GenModelHelper     - Registered GenModel 'http://www.eclipse.org/xtext/common/JavaVMTypes' from 'platform:/resource/org.eclipse.xtext.common.types/model/JavaVMTypes.genmodel'
707  INFO  AntlrToolFacade    - Downloading file from 'https://download.itemis.com/antlr-generator-3.2.0-patch.jar' ...
1291 INFO  AntlrToolFacade    - Finished downloading.
1318 INFO  XtextGenerator     - Generating org.xtext.example.mydsl.MyDsl
1380 INFO  EMFGeneratorFragment2 - Generating EMF model code

So is this kind of download expected or not?

Nevertheless I thought about it again and assume the mwe2 workflow execution isn't an OSGi runtime, is it? Therefore the DynamicImport-Package header is actually meaningless there. But the project's dependencies are probably added to the classpath of the mwe2 runtime, aren't they? But for that the regular Import-Package header can be considered and DynamicImport-Package probably isn't.
When I revert to the old Import-Package header I don't get the download printout, even if the downloaded jar is absent.
So I have the impression that the Import-Package is necessary here and therefore also the IUs in the TP.

As far as I know there is know other trick to pull in the itemis-plugin into a launch, unless one specifies a launch config for it and adds it.

LorenzoBettini · 2023-04-18T16:42:34Z

the download is not expected: that's what we want to avoid by having the de.itemis in the target platform

cdietrich · 2023-04-18T16:45:33Z

the xtext.generator and xtext.xtext.generator have a dep to the itemis thing which has a dependency to antlr.generator. so when the itemis thing is there the antlrtoolfacade should be able to find and use it.

cdietrich · 2023-04-27T10:00:10Z

any update here? for me it works without download when the itemis plugin is present in tp.

HannesWell · 2023-04-27T21:36:41Z

any update here? for me it works without download when the itemis plugin is present in tp.

Unfortunately I cannot report anything successful. I also tried to use p2.inf to tell Tycho that the package requirement is not greedy but that seems not to be considered during initialization.

So unless there isn't or at least will be soon another trick to tell Tycho to ignore the Package during resolution, I think this attempt failed und the PR can be abandoned.

cdietrich · 2023-04-28T04:18:41Z

ok thx for your effort anyway

HannesWell · 2023-04-28T04:55:49Z

Should we then remove the items feature from the Oomph targlet? Since it is already in the target it is not nessesary to have it in the setup again.
If you agree I can create a PR.

cdietrich · 2023-04-28T04:56:57Z

yes please create a pr. but i will have to test this once the pr is open if it still is working

HannesWell · 2023-04-28T05:45:08Z

yes please create a pr. but i will have to test this once the pr is open if it still is working

Of course, please see #2641.

HannesWell force-pushed the removeItemisFeature branch from 5cc6dfd to 8daf70d Compare April 15, 2023 05:55

HannesWell force-pushed the removeItemisFeature branch 2 times, most recently from 9617c38 to 6d66202 Compare April 17, 2023 06:32

HannesWell force-pushed the removeItemisFeature branch from 6d66202 to ac50770 Compare April 17, 2023 06:40

cdietrich closed this Apr 28, 2023

HannesWell deleted the removeItemisFeature branch April 28, 2023 04:51

HannesWell mentioned this pull request Apr 28, 2023

Remove 'de.itemis.xtext.antlr.feature' from Setup targlet #2641

Merged

HannesWell mentioned this pull request May 16, 2023

Require-Bundle javax.inject #2176

Closed

Conversation

HannesWell commented Apr 15, 2023

Uh oh!

cdietrich commented Apr 15, 2023

Uh oh!

HannesWell commented Apr 15, 2023

Uh oh!

LorenzoBettini commented Apr 15, 2023

Uh oh!

HannesWell commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laeubi commented Apr 17, 2023

Uh oh!

cdietrich commented Apr 17, 2023

Uh oh!

HannesWell commented Apr 17, 2023

Uh oh!

laeubi commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdietrich commented Apr 17, 2023

Uh oh!

szarnekow commented Apr 17, 2023

Uh oh!

laeubi commented Apr 17, 2023

Uh oh!

cdietrich commented Apr 17, 2023

Uh oh!

HannesWell commented Apr 17, 2023

Uh oh!

cdietrich commented Apr 17, 2023

Uh oh!

HannesWell commented Apr 18, 2023

Uh oh!

LorenzoBettini commented Apr 18, 2023

Uh oh!

cdietrich commented Apr 18, 2023

Uh oh!

cdietrich commented Apr 27, 2023

Uh oh!

HannesWell commented Apr 27, 2023

Uh oh!

cdietrich commented Apr 28, 2023

Uh oh!

HannesWell commented Apr 28, 2023

Uh oh!

cdietrich commented Apr 28, 2023

Uh oh!

HannesWell commented Apr 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HannesWell commented Apr 16, 2023 •

edited

Loading

laeubi commented Apr 17, 2023 •

edited

Loading