-
Notifications
You must be signed in to change notification settings - Fork 317
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
Remove 'de.itemis.xtext.antlr.feature' from target files #2219
Conversation
5cc6dfd
to
8daf70d
Compare
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. |
@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 |
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)?
This happens also with the new resolver. |
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: |
=> we have to keep them |
9617c38
to
6d66202
Compare
Indeed, makes sense in general. xtext/org.eclipse.xtext.generator/src/org/eclipse/xtext/generator/parser/antlr/AntlrToolFacade.java Line 29 in 93acb22
Wouldn't it make sense to use |
Then one should use |
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.
6d66202
to
ac50770
Compare
i dont know the |
I'm aware of the concept of Dynamic-ImportPackage but have never used it. Conceptionally it should do the trick. |
=> 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 |
the dependeny is in xtext.xtext.generator. |
I tested it with the example dsl xtext project from the xtext dev workspace and there I see the following print-out:
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. 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. |
the download is not expected: that's what we want to avoid by having the de.itemis in the target platform |
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. |
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. |
ok thx for your effort anyway |
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. |
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. |
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.