Community
Participate
Working Groups
After Bug 525948 and Bug 522333, Launching needs to come up with a wiki/guideline for adopters to follow the rules to support their launch configuration for Java 9. Now thinking about generalizing, currently launcher is calling getClasspath() to get classpath and getClasspathAndModulepath to get the modulepath which is totally unnecessary as getClasspathAndModulepath gives both. I think we can have a constant to define if getClasspathAndModulepath can be used for both. This is beacuse of the various ways of combination of inheritance. Like Junit has it's own launch delegate and getClasspath but it was calling super.getClasspath where as m2e uses JavaLaunchDelegate but has a totally overridden getClasspath and PDE again has a different combination. Because of m2e failures we had to continue using getClasspath Bug 522333. So for a launch Configuration, suppose IJavaLaunchConfigurationConstants.ATTR_USE_CLASSPATH_AND_MODULEPATH is set to true, Java launcher can use getClasspathAndModule path only. if false it can use getClasspath.
I am planning to do this early M6, any objections ?
Looking at JUnit and Maven Implementation (org.eclipse.m2e.internal.launch.MavenLaunchDelegate) Rather than introducing a new attribute and make things complicated I think it will be better to @Deprecate getClasspath() method so that slowly people start moving to getClasspathAndModulepath()
Will do it in Early 4.9
New Gerrit change created: https://git.eclipse.org/r/134950
created https://github.com/eclipse/mwe/issues/60 to address deprecation in MWE
Please also add it to the porting guide.
Gerrit change https://git.eclipse.org/r/134950 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=89530b29cb538a73f57f4b32cdf3f543258b9bc6
Will resolve after updating Porting guide.
Increasing the minor version was wrong. There was no API addition. Unfortunately we can't change the version back, so please add an API Tools filter to get rid of the error. NOTE: It is now visible as an error since I changed the project settings.
New Gerrit change created: https://git.eclipse.org/r/136024
Gerrit change https://git.eclipse.org/r/136024 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=0e3a75b61f445307a9be2320888d98a94307c783
This change breaks bndtools since the classpath entries for the project are not by definition the same as the ones we need for launching. I find this a very inelegant solution for what is only a unmeasurable small performance improvement. Sadly, I need to support old Eclipse's and new Eclipses and I do not see a clean way to support both without keeping getClasspath()
(In reply to Peter Kriens from comment #12) > This change breaks bndtools since the classpath entries for the project are > not by definition the same as the ones we need for launching. > > I find this a very inelegant solution for what is only a unmeasurable small > performance improvement. > > Sadly, I need to support old Eclipse's and new Eclipses and I do not see a > clean way to support both without keeping getClasspath() Performance has a big impact in big workspaces with Projects having a lot of dependencies. With Java 9 breaking changes, we are not left with many options.
It used to be first correct, then performance. This is a breaking change of public API. I do not think it is correct to make such breaking changes in a point release? You could let the getClasspath() return an empty array. Then call it from getClasspathAndModulePath if it is non-empty, use the single loop to extract the module path and classpath. Anyway, breaking performance improvements should at least have some metrics to show that they actually improve performance? In this case, I've serious doubts it can be measured even for very large builds. The iteration is over a list of at a low number of entries (between 10-100) that are in memory.
(In reply to Peter Kriens from comment #14) > It used to be first correct, then performance. This is a breaking change of > public API. I do not think it is correct to make such breaking changes in a > point release? > > You could let the getClasspath() return an empty array. Then call it from > getClasspathAndModulePath if it is non-empty, use the single loop to extract > the module path and classpath. > > Anyway, breaking performance improvements should at least have some metrics > to show that they actually improve performance? In this case, I've serious > doubts it can be measured even for very large builds. The iteration is over > a list of at a low number of entries (between 10-100) that are in memory. The implementations of this and the Launch Delegates are very different in all the implementors like maven JDT UI, PDE etc. It might actually return a valid empty list so we can assume based on that. And beyond java 8 getClasspath() will be incorrect without modulepath.
Shouldn't org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate.supportsModule return false by default, and subclasses that are moduleaware would override it and return true? Then org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate.getClasspathAndModulepath could check this and if it returns false invoke org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate.getClasspath
(In reply to Till Brychcy from comment #16) > Shouldn't > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > supportsModule > return false by default, and subclasses that are moduleaware would override > it > and return true? > > Then > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > getClasspathAndModulepath could check this and if it returns false invoke > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > getClasspath Ideally yes, but the problem is the combination state like now, where Ant, JUnit and PDE have not supported it yet but sometime they just use (rather than overriding) the default Java one which supports, so we cannot rely on that as well.
(In reply to Sarika Sinha from comment #17) > (In reply to Till Brychcy from comment #16) > > Shouldn't > > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > > supportsModule > > return false by default, and subclasses that are moduleaware would override > > it > > and return true? > > > > Then > > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > > getClasspathAndModulepath could check this and if it returns false invoke > > org.eclipse.jdt.launching.AbstractJavaLaunchConfigurationDelegate. > > getClasspath > > Ideally yes, but the problem is the combination state like now, where Ant, > JUnit and PDE have not supported it yet but sometime they just use (rather > than overriding) the default Java one which supports, so we cannot rely on > that as well. Another idea: maybe a check could be done in getClasspathAndModulepath whether the concrete instance class has overridden getClasspath but not getClasspathAndModulepath and use the getClasspath result in this case.
(In reply to Till Brychcy from comment #18) > > Another idea: maybe a check could be done in getClasspathAndModulepath > whether the concrete instance class has overridden getClasspath but not > getClasspathAndModulepath and use the getClasspath result in this case. Do we have a way other than reflection to find it out?
(In reply to Sarika Sinha from comment #19) > (In reply to Till Brychcy from comment #18) > > > > Another idea: maybe a check could be done in getClasspathAndModulepath > > whether the concrete instance class has overridden getClasspath but not > > getClasspathAndModulepath and use the getClasspath result in this case. > > Do we have a way other than reflection to find it out? I don't think so. But is there any problem with that? Maybe this could be done in the body of the default supportsModule() implementation - I don't really expect anybody to explicitly override it to return false (rather than implement module support). But existing and new classes that do support modules could override it to explicitly return true to skip the reflection based check. Then getClasspathAndModulepath could check supportsModule as written above.
We did think about this previously but rejected because of reflection.
(In reply to Sarika Sinha from comment #21) > We did think about this previously but rejected because of reflection. And using reflection is worse than breaking the API?
(In reply to Till Brychcy from comment #22) > (In reply to Sarika Sinha from comment #21) > > We did think about this previously but rejected because of reflection. > > And using reflection is worse than breaking the API? No, but because the user has to finally add getClasspathAndModulepath with Java 9 onward and it will give just an interim relief with reflection.
(In reply to Sarika Sinha from comment #23) > No, but because the user has to finally add getClasspathAndModulepath with > Java 9 onward and it will give just an interim relief with reflection. That is true, but many users still do use java 8 and with this change: https://git.eclipse.org/r/#/c/134950/2/org.eclipse.jdt.launching/launching/org/eclipse/jdt/launching/JavaLaunchDelegate.java getClasspath is not called even when the user uses Java 8 or earlier, so old implementations that used to work with java 8 are broken. Or am I overlooking something?
Sarika, this needs a fix. Reflection is not nice, so, if you can find another solution, e.g. by adding a new API, that would be preferred. But as a last resort you'll have to use reflection in this particular case.
New Gerrit change created: https://git.eclipse.org/r/136998
(In reply to Dani Megert from comment #25) > Sarika, this needs a fix. Reflection is not nice, so, if you can find > another solution, e.g. by adding a new API, that would be preferred. But as > a last resort you'll have to use reflection in this particular case. API will not help here, Using reflection and use the getClasspathAndModulepath if found else use the deprecated getClasspath method.
Gerrit change https://git.eclipse.org/r/136998 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=287d2087ab11da42aa248e3eb2a9e31049430bd4
I've just looked at the patch but not tried it, but I don't understand how it works. Just invoking getMethod should always return a non-null result for both methods (namely the implementations in AbstractJavaLaunchConfigurationDelegate. So I'd expect a check if java.lang.reflect.Method.getDeclaringClass() for "getClasspathAndModulepath" is AbstractJavaLaunchConfigurationDelegate.class but the result for "getClasspath" is not.
Reopening, see Comment 29
New Gerrit change created: https://git.eclipse.org/r/137107
(In reply to Till Brychcy from comment #29) > I've just looked at the patch but not tried it, but I don't understand how > it works. > Just invoking getMethod should always return a non-null result for both > methods (namely the implementations in > AbstractJavaLaunchConfigurationDelegate. > > So I'd expect a check if java.lang.reflect.Method.getDeclaringClass() for > "getClasspathAndModulepath" is AbstractJavaLaunchConfigurationDelegate.class > but the result for "getClasspath" is not. I don't think we can even rely on that. If we want to support getClasspath, Let us use the deprecated method till Java 8 is sunset and the remove it then and only use getClasspathAndModulepath after Java 8. Is it ok?
(In reply to Sarika Sinha from comment #32) > > So I'd expect a check if java.lang.reflect.Method.getDeclaringClass() for > > "getClasspathAndModulepath" is AbstractJavaLaunchConfigurationDelegate.class > > but the result for "getClasspath" is not. > > I don't think we can even rely on that. You mean in case somebody has subclassed a subclass that gets upgraded to implement both? Not sure if this really an issue. But a more paranoid solution would be to simply check if the declaring class of getClasspath is the same as the declaring class of getClasspathAndModulepath and if not, use getClasspath. Disadvantage is that subclasses will have to continue overriding "getClasspath" to trigger usage of the new method. > > If we want to support getClasspath, Let us use the deprecated method till > Java 8 is sunset and the remove it then and only use > getClasspathAndModulepath after Java 8. Unfortunately Java 9 broke too much old code, so I expect Java 8 to be used for a very long time (even if Oracle stops supporting it, others will). Maybe a compromise would be to always use "getClasspath" if the actual runtime is Java 8 or older and getClasspathAndModulepath if it is Java 9 or newer.
New Gerrit change created: https://git.eclipse.org/r/137112
We can run with java 9 without having any modulepath definition like we do in Ant also, so can't rely on that as well. For 4.11 I think it is better to go by using both methods approach and then decide in 4.12.
Gerrit change https://git.eclipse.org/r/137112 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=bac90ede4bba8083511847434d8204cdfb65282f
Moving to 4.13.
Hopefully, will be able to take this up soon.
I have not been able to derive a good solution for this.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
