GH-264: Xpect files cannot be launched in IDE under Java 9

[mor-n4]

See #264.

[szarnekow]

Instead of hand-rolling smth, you may want to consider using io.github.classgraph (see eclipse/xtext-extras#433)

[mor-n4]

Yes, @cdietrich already mentioned this in the issue. However, he said it's not in the orbit yet.

[szarnekow]

It's available in the latest orbit drops.

[mor-n4]

Good!

But then more changes are required than just my small bug fix. Some existing code needs to be replaced. Also a CQ is needed.

I'll have a look. Maybe I can provide something...

[mor-n4]

build green:
https://ci.eclipse.org/xpect/job/Xpect/job/GH-264/1/

I'll test this tomorrow with our n4js build.

[mor-n4]

@meysholdt please have a look. Thanks!

[mor-n4]

@meysholdt

Just to provide some background:

The original code

		if (classLoader instanceof URLClassLoader) {
			URLClassLoader ucl = (URLClassLoader) classLoader;

from ClasspathUtil.java isn't compatible with Java 9+ because as of version 9 the system class loader does no longer extend URLClassLoader.

[mor-n4]

@dhuebner since your are currently working on Xpect, you might also wanna have a quick look. Thanks!

[cdietrich]

@mor-n4 you should sign off all commits to get the eca validation running (and likely squash the commits into one)

[cdietrich]

also https://ci.eclipse.org/xpect/job/Xpect/job/GH-264/lastSuccessfulBuild/artifact/org.eclipse.xpect.releng/p2-repository/target/repository/plugins/ does not seems to contain classgraph
i assume you have to package that as well. (have a look at what we do with hamcrest in feature.xmls)

[cdietrich]

also somebody with commit rights should file a (piggy back) CQ for classgraph.

[mor-n4]

@mmews-n4 just created the CQ. Thanks!

[dhuebner]

@mor-n4 I'm not familiar with the ClassGraph API, but your code replacement looks okay.
What I dislike is a mandatory dependency to the "new" Orbit bundle.
This will cause existing projects to update there target platforms to pick the new dependency.
Would it be an option to first check the instance Type of the classloader and if it's not an URLClassLoader try to use ClassGraph?
Also you mentioned an other solution for the class loader cast exception in #264
In general I don't like adding dependencies to 3rd party library for a one-liner.

[dhuebner]

@mor-n4
I prefer this simple backward compatible solution https://github.com/eclipse/Xpect/tree/GH-264-simple

[cdietrich]

you will pull the new classgraph with Xtext 2.19 anyway.

[mor-n4]

@dhuebner

In general I don't like adding dependencies to 3rd party library for a one-liner.

Neither do I. That's why I had first implemented a solution without adding a new dependency. See my first commit in this PR.
However, it was proposed to use classgraph which is also used in Xtext. For consistency between the closely related projects I figured it makes sense.

[dhuebner]

you will pull the new classgraph with Xtext 2.19 anyway.

Most customer project with Xpect are using Xtext 2.14 and older. So it's not a pro argument to me.

[cdietrich]

projects with java 11 wont use Xtext 2.14

[mmews-n4]

We don't know whether the one-liner is a fully adequate substitute for the graphclass library, also with regard to all versions of Java 9+ (i.e. 10, 11, 12, 13, and beyond).

[mor-n4]

@dhuebner

What I dislike is a mandatory dependency to the "new" Orbit bundle.
This will cause existing projects to update there target platforms to pick the new dependency.

From what I understand, just the Xpect build is configured to pick classgraph from the Orbit. But some customer project using Xpect and updating to a new version of Xpect (that has the dependency to classgraph) could pick classgraph 4.8.35 from elsewhere, e.g. maven central (as long as that customer project is not an Eclipse project).

[cdietrich]

no. this is why i proposed it to package it to the update site e.g.
https://github.com/eclipse/xtext-umbrella/blob/e217c1c63c0ec67d00a530041e4e093c175a67ed/releng/org.eclipse.xtext.sdk.p2-repository/category.xml#L17
and in standalone it will be picked from maven central anyway if you adapt the pom

[dhuebner]

@mor-n4 I just described my opinion, I'm not reviewing your PR. @meysholdt Will do

[meysholdt]

I'm fine with this change.

I very much like that idea that the responsibility to correctly list all ClassPath entries is no longer with Xpect. Casting down to the URLClassLoader has always been a hack.

The group of users that wants to migrate to the latest Xpect version and at the same time stick with an old Xtext version will have to adjust their target platform to include the ClassGraph bundle from Orbit. But that's ok, since they as well will benefit from correct classpath scanning and us being able to devote our time to more valuable features.

The group of users that uses the latest version of Xtext won't have migration effort as @cdietrich pointed out...

[meysholdt]

no. this is why i proposed it to package it to the update site e.g.
https://github.com/eclipse/xtext-umbrella/blob/e217c1c63c0ec67d00a530041e4e093c175a67ed/releng/org.eclipse.xtext.sdk.p2-repository/category.xml#L17
and in standalone it will be picked from maven central anyway if you adapt the pom

Xpect could do it in the same way.

[mor-n4]

no. this is why i proposed it to package it to the update site e.g.
https://github.com/eclipse/xtext-umbrella/blob/e217c1c63c0ec67d00a530041e4e093c175a67ed/releng/org.eclipse.xtext.sdk.p2-repository/category.xml#L17
and in standalone it will be picked from maven central anyway if you adapt the pom

Xpect could do it in the same way.

I had already added this when @cdietrich pointed that out the first time. See
https://ci.eclipse.org/xpect/job/Xpect/job/GH-264/5/artifact/org.eclipse.xpect.releng/p2-repository/target/repository/plugins/

[cdietrich]

i see maybe add the source too to be consistent

[mor-n4]

BTW: if it is true that the original io.github.classgraph in version 4.8.35 from some other location like maven central would not work (why? isn't it a problem to have the same ID then?), should we then maybe better specify the full version 4.8.35.v20190528-1517 in the manifest of org.eclipse.xpect too?

[cdietrich]

yes and no: the question is:

• eclipse usecase: no since not in target / installed
• standalone: yes

having the full version in target is a thing of targets so that it wont accept it without qualifier whilst eclipse version policy wants the qualifier and maven does not have it

[mor-n4]

@dhuebner

@mor-n4 I just described my opinion, I'm not reviewing your PR.

No problem. What you proposed was my initial idea, too, but I can see the advantages the others mentioned (consistency with other projects, etc.).

Any case, thanks for the feedback!

[mor-n4]

Recent changes:

1. added the classgraph source to the update site
2. @mmews-n4 created a CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20234

I guess this concludes the discussion and we will later merge this after fixing the eca checker issues.

Thanks everyone for the feedback, esp. @cdietrich for the help!

[mor-n4]

Closing this PR; due to problems with eclipse/eca checker it's now replaced by #268 (which contains the exact same changes).