Lb jdt compiler apt #1833
Lb jdt compiler apt #1833
Conversation
| <!-- to force the same version of jdt.compiler.apt and jdt.core | ||
| (for xtext-maven-plugin) | ||
| see https://github.com/eclipse/xtext-maven/issues/146 --> | ||
| <requirement> |
There was a problem hiding this comment.
should this be
<requirement>
<type>eclipse-feature</type>
<id>org.eclipse.jdt</id>
<versionRange>0.0.0</versionRange>
</requirement>
as in dmain model?
should/can we do this only if jdt is needed? i assume no
There was a problem hiding this comment.
@cdietrich If you mean to have some kind of checkbox in the wizard we could also do that; otherwise, in the POM, we cannot have such a level of configuration.
I had avoided including the whole org.eclipse.jdt feature because that's quite huge... in the domain model example (and, in general, in all Xbase DSLs UI tests), we need org.eclipse.jdt ONLY to include the jdt.launch fragment for macOS, and that's the only way: if you tried to include the macOS fragment the build would fail in non macOS environments).
There was a problem hiding this comment.
hmmm i am unsure. i have zero clue why fragment resolving is such a mess. @kthoms what do you think?
There was a problem hiding this comment.
if not should we add compiler.tool fragment too as we do it on maven side?
There was a problem hiding this comment.
@cdietrich the whole jdt feature would work as well (I guess) but it would be a lot of stuff as I said in my previous comment.
Concerning the jdt.macos.launch fragment, it does not work because the fragment would be taken into the target platform always and it would fail when not running on macOS, that's why in that case we need to take the whole feature: that fragment exists ONLY for macOS not for the other systems (to be honest, I don't know why...)
The compiler.tool was not needed in my experiments, that's why I did not add that (to keep things minimal)
There was a problem hiding this comment.
nope, manifest says java 11, but am not sure if this is also for the classes. and we also pull it e.g. here https://github.com/eclipse/xtext-extras/blob/8751673604b72cbe7536bb0a813b92c01761c636/org.eclipse.xtext.builder.standalone/build.gradle#L11
There was a problem hiding this comment.
nope the classes are 11
javap -v ./org/eclipse/jdt/internal/compiler/tool/EclipseCompiler.class | grep 55
major version: 55
=> wonder if there are paths in execution that also pull compiler.tool classes
There was a problem hiding this comment.
@cdietrich @kthoms please let me know what you prefer.
I'm not sure I understand your last comments @cdietrich : are you saying that compiler.tool can be safely added or it does not work depending on the Java version?
Concerning fragments, as far as I understand, the problem comes from how Tycho works: it only puts required bundles in the TP for tests, not features, and fragments like the ones we are talking about are only parts of features. The workaround we've always been using so far (and the one advertised by Tycho as well, from what I recall) is use the extraRequirements, that allows you to add to the test TP both features (like the jdt.feature for UI tests based on Xbase, for the macOS environment) and fragments like the jdt.apt of this issue.
There was a problem hiding this comment.
no i dont - it was false alarm. it just might not be used in your calls. i understand: you add jdt.core and compiler.tool so that both will be there and win.
i say: shouldt compiler.tool added too as we have all 3 in the maven dependencies
There was a problem hiding this comment.
So you're suggesting to add jdt.core, jdt.compiler.apt and jdt.compiler.tool?
Concerning jdt.compiler.tool, it doesn't seem to be required in my previous experiments, but I guess it wouldn't harm.
Concerning jdt.core, that's a bundle, so if one needs that then it's already part of the TP anyway. So it's not needed as an extra requirement. I might also think that jdt.core might interfere in contexts where jdt is not used at all (but that's just my gut feeling).
The fragments are harmless in contexts where jdt is not used.
Just to recap: this fix is needed only if one runs the xtext-maven-plugin during a Tycho build. It's not needed if the xtext-maven-plugin is run in a pure Maven build. It's not even needed in general if one never uses xtext-maven-plugin. That's why I kept the fix to the minimal required.
|
can you please squash both commits into 1 |
Yes of course, I can do that also upon merging, as far as I know |
|
am not sure if eclipse eca tooling likes squash on merge. |
OK, when you think we're ready I'll squash that locally. |
|
@kthoms i would really appreciate your feedback here. we want to build RC1 on monday and the release the monday that after |
|
@LorenzoBettini can you please do the squash. |
|
@cdietrich so from my side what should I do? |
|
you can rebase and squash you change, so that it deletes the compiler.tool dependency again. |
|
The change looks fine to me. It is correct that only the fragment is added and not the feature. Is this PR superseeded by #1836 ? The issue seems to be fixed by that. |
|
@kthoms in maven we use compiler.apt and compiler.tool |
|
The issue has not been seen with |
|
thx @kthoms |
Yes, as I said, it should be harmless and I tried to be as minimal as possible. So we're closing this one as superseded right? |
|
yes |
Closes eclipse/xtext-maven#146
by adding as extra requirement in the TP configuration of the parent POM the dependency org.eclipse.jdt.compiler.apt