Bug 518751 - [9] Improve updating of or remove ModulePathContainer
Summary: [9] Improve updating of or remove ModulePathContainer
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.7   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: ---   Edit
Assignee: JDT-Core-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug bulk move
Keywords:
Depends on:
Blocks: 518463
  Show dependency tree
 
Reported: 2017-06-25 17:03 EDT by Stephan Herrmann CLA
Modified: 2022-06-14 13:00 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Herrmann CLA 2017-06-25 17:03:26 EDT
Build id: Y20170620-0625

Similar to bug 506479, I saw SOE twice when trying to rename a package (which may have been referenced in "exports"). After that the workspace was unusable.

java.lang.StackOverflowError
        at java.base/java.io.UnixFileSystem.getBooleanAttributes0(Native Method)
        at java.base/java.io.UnixFileSystem.getBooleanAttributes(UnixFileSystem.java:240)
        at java.base/java.io.File.isFile(File.java:883)
        at org.eclipse.jdt.launching.JavaRuntime.getLibraryLocations(JavaRuntime.java:1739)
        at org.eclipse.jdt.internal.launching.JREContainer.computeClasspathEntries(JREContainer.java:297)
        at org.eclipse.jdt.internal.launching.JREContainer.getClasspathEntries(JREContainer.java:279)
        at org.eclipse.jdt.internal.launching.JREContainer.getClasspathEntries(JREContainer.java:396)
        at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2817)
        at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2975)
        at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:2080)
        at org.eclipse.jdt.internal.core.JavaProject.getClasspathEntryFor(JavaProject.java:1538)
        at org.eclipse.jdt.internal.core.PackageFragmentRoot.validateOnClasspath(PackageFragmentRoot.java:768)
        at org.eclipse.jdt.internal.core.PackageFragmentRoot.validateExistence(PackageFragmentRoot.java:818)
        at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:245)
        at org.eclipse.jdt.internal.core.Openable.openAncestors(Openable.java:505)
        at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:241)
        at org.eclipse.jdt.internal.core.Openable.openAncestors(Openable.java:505)
        at org.eclipse.jdt.internal.core.CompilationUnit.openAncestors(CompilationUnit.java:1190)
        at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:241)
        at org.eclipse.jdt.internal.core.SourceRefElement.generateInfos(SourceRefElement.java:107)
        at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:581)
        at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:318)
        at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:304)
        at org.eclipse.jdt.internal.core.AbstractModule.getRequiredModules(AbstractModule.java:28)
        at org.eclipse.jdt.internal.core.ModulePathContainer.getClasspathEntries(ModulePathContainer.java:45)
        at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2817)
Comment 1 Stephan Herrmann CLA 2017-07-11 16:28:08 EDT
Similar:

SourceModule(JavaElement).getElementInfo(IProgressMonitor) line: 318	
SourceModule(JavaElement).getElementInfo() line: 304	
SourceModule(AbstractModule).getRequiredModules() line: 28	
ModulePathContainer.getClasspathEntries() line: 45	
JavaProject.resolveClasspath(IClasspathEntry[], IClasspathEntry[], boolean, boolean) line: 2814	
JavaProject.resolveClasspath(JavaModelManager$PerProjectInfo, boolean, boolean) line: 2972	
JavaProject.getResolvedClasspath() line: 2077	
PackageFragmentRoot.getRawClasspathEntry() line: 574	
PackageFragmentRoot.fullInclusionPatternChars() line: 415	
CompilationUnit.validateCompilationUnit(IResource) line: 1058	
CompilationUnit.validateExistence(IResource) line: 1350	
CompilationUnit(Openable).generateInfos(Object, HashMap, IProgressMonitor) line: 245	
SourceModule(SourceRefElement).generateInfos(Object, HashMap, IProgressMonitor) line: 107	
SourceModule(JavaElement).openWhenClosed(Object, boolean, IProgressMonitor) line: 581	
SourceModule(JavaElement).getElementInfo(IProgressMonitor) line: 318	
SourceModule(JavaElement).getElementInfo() line: 304	
SourceModule(AbstractModule).getRequiredModules() line: 28	
ModulePathContainer.getClasspathEntries() line: 45	
JavaProject.resolveClasspath(IClasspathEntry[], IClasspathEntry[], boolean, boolean) line: 2814	
JavaProject.resolveClasspath(JavaModelManager$PerProjectInfo, boolean, boolean) line: 2972	
JavaProject.getResolvedClasspath() line: 2077	
PackageFragmentRoot.getRawClasspathEntry() line: 574	
PackageFragmentRoot.fullInclusionPatternChars() line: 415	
CompilationUnit.validateCompilationUnit(IResource) line: 1058	
CompilationUnit.validateExistence(IResource) line: 1350	

Here, validating existence of CU module-info.java indirectly requires the same CU to be opened, which requires validating its existence ...

Here my last actions where:
- delete module-info.java
- clean the project
Comment 2 Stephan Herrmann CLA 2017-10-09 16:55:36 EDT
This seems to require a design decision:
- flesh out ModulePathContainer to robustly handle all kinds of resource changes
- or, remove it and approach downstream plug-ins to provide their own, specific "target platforms"

Anybody not going to EclipseCon is invited to have their say here. Otherwise I suggest to discuss this over a coffee in Ludwigsburg :)
Comment 3 Manoj N Palat CLA 2017-10-10 00:03:07 EDT
(In reply to Stephan Herrmann from comment #2)
> I suggest to discuss this over a coffee in Ludwigsburg :)

@Stephan: putting your name as QA contact - you can convince any of the committer attendees (over coffee or beer - I leave it you :)) to take over this :)
Comment 4 Stephan Herrmann CLA 2017-10-10 07:48:34 EDT
(In reply to Manoj Palat from comment #3)
> (In reply to Stephan Herrmann from comment #2)
> > I suggest to discuss this over a coffee in Ludwigsburg :)
> 
> @Stephan: putting your name as QA contact - you can convince any of the
> committer attendees (over coffee or beer - I leave it you :)) to take over
> this :)

Still, as input for that discussion I'd like to see a statement by the author(s) of ModulePathContainer.
Comment 5 Jay Arthanareeswaran CLA 2017-10-23 11:09:37 EDT
Note: When and if we decide to remove the ModulePathContainer, we need a transition plan for those who already have it in their classpath. Currently, although the code is ineffective, the module path node appears in the package explorer.
Comment 6 Manoj N Palat CLA 2017-11-13 21:58:00 EST
moving out of 4.7.2
Comment 7 Stephan Herrmann CLA 2017-12-19 14:03:34 EST
It seems I missed this on my cheat sheet for discussions with Dani :-/

To make this an actionable question: do we expect the following to be a relevant / important / high priority use case:

- Projects are *not* subject to dependency management by PDE, m2e or the like.

- Projects are modular and have dependencies expressed in module-info

- User expects that required modules are automatically found by the IDE,
  i.e., without the need to explicitly add dependencies between projects.


If the answer is some kind of "yes", then we still have two options:

- ModulePathContainer

- flesh out quick fixes etc. to automatically add project dependencies for
  all "requires".



And here's my personal say:

This could indeed by relevant for small experiments with Java 9, but is unlikely to happen for production code.

Due to difficulties with keeping the ModulePathContainer up-to-date, a quick-fix-like solution seems more like it.

I just wonder if we could find a way, to make JDT smart enough so that it can implicitly add project dependencies when we know we are in "module experimentation mode".
Comment 8 Noopur Gupta CLA 2017-12-20 04:32:54 EST
This was discussed with you in a private mail-chain titled "No more MODULE_PATH?". Here is what Sasi had responded in the mail (based on the discussion with Markus):

Primary concern with the container was that in the presence of multiple versions of the same module, the container cannot make a choice about which version to pick because version management is not in scope of the module system (yet). Situation would be similar to having multiple modules with the same name in the workspace. The container cannot make an assumption about the intentions of the user and hence it does not seem to serve much purpose other than for very simple cases. That is the same reason even the target platform like repository was set aside. Version management is an integral part of the target platform (in PDE) but it is not defined for java modules. So, it was decided that we will still have the container and may be provide a menu action to add it to a project's build path if the user so desires, but we will make it clear that it is not the preferred or the default way.
Comment 9 Stephan Herrmann CLA 2017-12-20 11:07:59 EST
Thanks, Noopur for filling in the gaps in an old man's memory :)

I think we agree that ModulePathContainer is not a capable concept in (most) productive use (where versioning is a must).

(In reply to Noopur Gupta from comment #8)
> So, it was decided that we will still have the container and may be provide
> a menu action to add it to a project's build path if the user so desires,
> but we will make it clear that it is not the preferred or the default way.

Where does this leave us wrt remaining bugs? IMHO in its current shape the warning should be stronger than saying "not the preferred way". But if we call it "experimental" and don't fix bugs, then it doesn't meet the goal of making simple module experiments super easy. Specifically for the use case of first-contact-with-modules experiments it should be rock solid.

I haven't tested this in a while, but *if* status is still basically unchanged from June, then I'm afraid the feature doesn't live up to its promise. Hence the question: fix or remove?

The other question being: has anybody fixed any bugs in this area since June? Tested this feature in recent builds?
Comment 10 Sasikanth Bharadwaj CLA 2017-12-21 00:37:23 EST
(In reply to comment #9)
> Thanks, Noopur for filling in the gaps in an old man's memory :)
> 
> I think we agree that ModulePathContainer is not a capable concept in (most)
> productive use (where versioning is a must).
> 
> (In reply to Noopur Gupta from comment #8)
> > So, it was decided that we will still have the container and may be provide
> > a menu action to add it to a project's build path if the user so desires,
> > but we will make it clear that it is not the preferred or the default way.
> 
> Where does this leave us wrt remaining bugs? IMHO in its current shape the
> warning should be stronger than saying "not the preferred way". But if we call
> it "experimental" and don't fix bugs, then it doesn't meet the goal of making
> simple module experiments super easy. Specifically for the use case of
> first-contact-with-modules experiments it should be rock solid.
> 
I guess it goes into the failed experiments bucket.
> I haven't tested this in a while, but *if* status is still basically unchanged
> from June, then I'm afraid the feature doesn't live up to its promise. Hence the
> question: fix or remove?
> 
I'd say remove, but there's the question of migration of existing projects that use the container (if any). 
> The other question being: has anybody fixed any bugs in this area since June?
> Tested this feature in recent builds?
No, to the best of my knowledge, for both
Comment 11 Noopur Gupta CLA 2017-12-21 02:16:17 EST
+1 to remove.

bug 518296 would have already updated the MP entry regarding this in June and we haven't seen any bug reports or comments. So I doubt if there are any active users.
Comment 12 Manoj N Palat CLA 2018-02-22 05:04:05 EST
moving out of 4.7.3  target
Comment 13 Stephan Herrmann CLA 2018-11-13 16:41:16 EST
ModuleBuilderTest.test_ModuleSourcePath_implicitdeps2 is being disabled on behalf of bug 540788. Will probably be removed via this bug.
Comment 14 Stephan Herrmann CLA 2019-05-19 18:15:53 EDT
Sorry for having to move this once more, to - hopefully - early 4.13.

I'd be more than happy if s.o. beats me to it.
Comment 15 Manoj N Palat CLA 2019-08-27 02:06:48 EDT
Bulk move out of 4.13
Comment 16 Eclipse Genie CLA 2022-06-14 13:00:57 EDT
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.