search

eclipse / Xpect

This repository has been rewritten to move to the Eclipse Foundation. Find the old history here: https://github.com/TypeFox/Xpect
http://www.xpect-tests.org/
Eclipse Public License 2.0
30 stars 28 forks source link

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

Closed mor-n4 closed 5 years ago

mor-n4 commented 5 years ago

See #264.

szarnekow commented 5 years ago

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

mor-n4 commented 5 years ago

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

szarnekow commented 5 years ago

It's available in the latest orbit drops.

mor-n4 commented 5 years ago

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 commented 5 years ago

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

I'll test this tomorrow with our n4js build.

mor-n4 commented 5 years ago

@meysholdt please have a look. Thanks!

mor-n4 commented 5 years ago

@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 commented 5 years ago

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

cdietrich commented 5 years ago

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

cdietrich commented 5 years ago

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 commented 5 years ago

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

mor-n4 commented 5 years ago

@mmews-n4 just created the CQ. Thanks!

dhuebner commented 5 years ago

@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 commented 5 years ago

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

cdietrich commented 5 years ago

you will pull the new classgraph with Xtext 2.19 anyway.

mor-n4 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

projects with java 11 wont use Xtext 2.14

mmews-n4 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

meysholdt commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

i see maybe add the source too to be consistent

mor-n4 commented 5 years ago

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 commented 5 years ago

yes and no: the question is:

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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