Cannot import maven project from workspace #2999

snjeza · 2023-12-12T17:26:35Z

Fixes redhat-developer/vscode-java#2972

Steps to Reproduce:

See redhat-developer/vscode-java#2972 (comment)

Test vsix - VS Code 1.27.3

rgrunber

Seems to fix the issue for me. Just a few small things to fix.

Is it the case that JDTLS filesystem does not currently exclude any resources when performing operations ?

....ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JDTLSFilesystemActivator.java

rgrunber · 2023-12-28T18:31:32Z

....ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JDTLSFilesystemActivator.java

+	}
+
+	private void configureResourceFilters() {
+		IEclipsePreferences eclipsePreferences = InstanceScope.INSTANCE.getNode(JAVA_LS_PLUGIN_ID);


Just something to be aware of :
Although I'd have preferred making jdt.ls.filesystem directly depend on jdt.ls.core I'll allow this copying because in the long term , we have #2658, so we should avoid any link between this bundle and jdt.ls.core).

....ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JDTLSFilesystemActivator.java

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/PreferenceManager.java

snjeza · 2023-12-28T22:06:11Z

Is it the case that JDTLS filesystem does not currently exclude any resources when performing operations ?

Yes, it is.
This operation is slow -getProjectNameIfLocationIsProjectRoot

snjeza · 2023-12-28T22:19:20Z

@rgrunber I have updated the PR.

rgrunber · 2023-12-29T17:25:07Z

Is it the case that JDTLS filesystem does not currently exclude any resources when performing operations ?

Yes, it is. This operation is slow -getProjectNameIfLocationIsProjectRoot

I completely agree. All those node_modules/** files are definitely the issue.

My question was more about why we need to manually exclude files (based on filters) when we configure filters at initialization. I would have hoped the filesystem API would do it for us.

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

Line 690 in 2b7857b

    
           project.createFilter(JDTLS_FILTER_TYPE, new FileInfoMatcherDescription(CORE_RESOURCES_MATCHER_ID, resourceFilter), IResource.BACKGROUND_REFRESH, monitor);

. We configure filters at initialization for all projects (except default).

The javadoc says :

Adds a new filter to this container. Filters restrict the set of files and directories
in the underlying file system that will be included as members of this container.

I haven't looked too deeply, so maybe the API doesn't work that way. Update: Maybe the resource model is independent of the filesystem model and manual filtering is the right approach.

snjeza · 2023-12-29T17:43:03Z

I haven't looked too deeply, so maybe the API doesn't work that way.

The issue happens only when we import a project into a new workspace. In this case, getProjectNameIfLocationIsProjectRoot is called before we configure the project (and its filters)

rgrunber · 2023-12-29T17:54:39Z

You're right 😐

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandler.java

Lines 260 to 261 in 2b7857b

    
           projectsManager.initializeProjects(roots, subMonitor); 
        
           projectsManager.configureFilters(monitor);

. I guess it would get complicated to set the project filters in each kind of project importer :\

snjeza · 2023-12-29T18:13:53Z

I guess it would get complicated to set the project filters in each kind of project importer :\

The project filters already work that way

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

Line 668 in 2b7857b

for (IProject project : ProjectUtils.getAllProjects()) {

rgrunber

Overall looks good. I would just rename the commit message to make it clear we're basically re-implementing resource filtering in the filesystem implementation beause resource filtering isn't yet in place. Something like

JDT LS Filesystem should respect resource filtering

- Resource model filtering is set up after projects are imported, but
  some import operations (eg. Maven projects) could benefit from this

Once done, feel free to merge.

rgrunber · 2024-01-01T23:35:10Z

....ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JDTLSFilesystemActivator.java

+import java.util.Objects;
+import java.util.regex.Pattern;
+
+import org.eclipse.core.runtime.preferences.IEclipsePreferences;


You have some unused imports here. IEclipsePreferences and InstanceScope.

rgrunber · 2024-01-01T23:36:51Z

....ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JDTLSFilesystemActivator.java

+
+	protected void setResourcePatterns() {
+		resourcePatterns = new ArrayList<>();
+		for (String element: resourceFilters.split("::")) {


Can you leave a space between element and :

rgrunber · 2024-01-01T23:37:12Z

org.eclipse.jdt.ls.filesystem/src/org/eclipse/jdt/ls/core/internal/filesystem/JLSFsUtils.java

+            for (String segment: path.segments()) {
+                for (Pattern pattern: JDTLSFilesystemActivator.getResourcePatterns()) {


Same here. Space between the element and the :.

- Resource model filtering is set up after projects are imported, but some import operations (eg. Maven projects) could benefit from this

snjeza · 2024-01-01T23:59:30Z

Overall looks good. I would just rename the commit message to make it clear we're basically re-implementing resource filtering in the filesystem implementation beause resource filtering isn't yet in place. Something like

Fixed

snjeza requested review from fbricon, rgrunber, jdneo and testforstephen December 12, 2023 17:26

rgrunber added this to the End December 2023 milestone Dec 12, 2023

snjeza added the in progress label Dec 12, 2023

snjeza changed the title ~~[performance] Cannot import maven project from workspace~~ [WIP] [performance] Cannot import maven project from workspace Dec 12, 2023

snjeza force-pushed the issue-2972 branch 2 times, most recently from 0d82fcd to 978b93e Compare December 13, 2023 16:06

snjeza changed the title ~~[WIP] [performance] Cannot import maven project from workspace~~ [performance] Cannot import maven project from workspace Dec 13, 2023

snjeza removed the in progress label Dec 13, 2023

rgrunber added the performance label Dec 20, 2023

rgrunber changed the title ~~[performance] Cannot import maven project from workspace~~ Cannot import maven project from workspace Dec 20, 2023

rgrunber reviewed Dec 28, 2023

View reviewed changes

snjeza force-pushed the issue-2972 branch from 978b93e to f1728dd Compare December 28, 2023 22:14

rgrunber approved these changes Jan 1, 2024

View reviewed changes

snjeza force-pushed the issue-2972 branch from f1728dd to 22efbad Compare January 1, 2024 23:50

JDT LS Filesystem should respect resource filtering

a3dc74e

- Resource model filtering is set up after projects are imported, but some import operations (eg. Maven projects) could benefit from this

snjeza force-pushed the issue-2972 branch from 22efbad to a3dc74e Compare January 1, 2024 23:56

snjeza merged commit b652bd2 into eclipse-jdtls:master Jan 2, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cannot import maven project from workspace #2999

Cannot import maven project from workspace #2999

snjeza commented Dec 12, 2023 •

edited

Loading

rgrunber left a comment

rgrunber Dec 28, 2023

snjeza commented Dec 28, 2023

snjeza commented Dec 28, 2023

rgrunber commented Dec 29, 2023 •

edited

Loading

snjeza commented Dec 29, 2023

rgrunber commented Dec 29, 2023

snjeza commented Dec 29, 2023 •

edited

Loading

rgrunber left a comment •

edited

Loading

rgrunber Jan 1, 2024

snjeza Jan 1, 2024

rgrunber Jan 1, 2024

snjeza Jan 2, 2024

rgrunber Jan 1, 2024

snjeza Jan 2, 2024

snjeza commented Jan 1, 2024

		for (String segment: path.segments()) {
		for (Pattern pattern: JDTLSFilesystemActivator.getResourcePatterns()) {

Cannot import maven project from workspace #2999

Cannot import maven project from workspace #2999

Conversation

snjeza commented Dec 12, 2023 • edited Loading

rgrunber left a comment

Choose a reason for hiding this comment

rgrunber Dec 28, 2023

Choose a reason for hiding this comment

snjeza commented Dec 28, 2023

snjeza commented Dec 28, 2023

rgrunber commented Dec 29, 2023 • edited Loading

snjeza commented Dec 29, 2023

rgrunber commented Dec 29, 2023

snjeza commented Dec 29, 2023 • edited Loading

rgrunber left a comment • edited Loading

Choose a reason for hiding this comment

rgrunber Jan 1, 2024

Choose a reason for hiding this comment

snjeza Jan 1, 2024

Choose a reason for hiding this comment

rgrunber Jan 1, 2024

Choose a reason for hiding this comment

snjeza Jan 2, 2024

Choose a reason for hiding this comment

rgrunber Jan 1, 2024

Choose a reason for hiding this comment

snjeza Jan 2, 2024

Choose a reason for hiding this comment

snjeza commented Jan 1, 2024

snjeza commented Dec 12, 2023 •

edited

Loading

rgrunber commented Dec 29, 2023 •

edited

Loading

snjeza commented Dec 29, 2023 •

edited

Loading

rgrunber left a comment •

edited

Loading