Use linked resource instead of filesystem #2658

mickaelistria · 2023-05-16T12:03:28Z

For all metadata files, except the .project (which may be harder to move).
This also copies some logic from the filesystem to ProjectsManager. We cannot have the filesystem bundle requiring and referencing the main jdt.ls bundle as this would cause classloading error.

rgrunber · 2023-05-16T15:03:56Z

A while ago we had a discussion where you mentioned that .classpath, .settings files can be set using linked resources, so doing this is likely the right approach. We should use that filesystem customization as little as possible. The tests likely need some investigation.

mickaelistria · 2023-05-16T15:05:59Z

I'm looking at the tests; however, it looks like it's not allowed to put linked resources under <workspace>/.metadata. Is it OK if we move those to <workspace>/.projects-metadata ?

fbricon · 2023-05-16T15:13:57Z

your code only handles the case of projects that are automatically created by jdt.ls (default and invisible). Buildship or m2e would still create the eclipse config files in the source folder, on import, without the FS trick. Unless there's a simple way to workaround that problem, I don't see linked resources as a viable solution.

mickaelistria · 2023-05-16T15:48:21Z

Buildship or m2e would still create the eclipse config files in the source folder, on import, without the FS trick.

OK, I see. I guess the importer can be reworked to first create the Java project, using ProjectsManager.createJavaProject() and then configuring it instead of making them fully create the project.

mickaelistria · 2023-06-26T12:12:42Z

Related: eclipse-platform/eclipse.platform#517

mickaelistria · 2023-07-10T15:48:40Z

I will try to investigate how this works or not with Bazel support ( redhat-developer/vscode-java#909 (comment) )

mickaelistria · 2023-07-19T11:12:49Z

It looks like vscode-java-bazel already puts eclipse metadata into a .eclipse folder inside project at import; so this patch doesn't seem to change that.

mickaelistria · 2023-09-15T15:16:39Z

It would be nice if this could be revisited sometimes. Working without the URI hacks would close the doors to many potential bugs.

rgrunber · 2023-09-15T15:54:42Z

@jdneo do you think you might have time to look here. I see this is completely removing the jdt.ls.filesystem bundle so definitely would be nice to thoroughly test this.

jdneo · 2023-10-31T02:56:13Z

So if it's Windows-specific issue, CI wouldn't see it?

I guess it's a Windows issue. I tried using Files.move(metadataFile.getLocation().toFile().toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING); to replace .renameTo and the metadata files won't exist on project root then.

We may also want to track if it happens that the linked resources are failed to be established. Will all thrown exception be sent as telemetry? This is a question to @rgrunber and @testforstephen

mickaelistria · 2023-10-31T07:34:15Z

I tried using Files.move(metadataFile.getLocation().toFile().toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING); to replace .renameTo and the metadata files won't exist on project root then.

Thanks for the tip, I've changed the code to use it.

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

jdneo · 2023-11-02T03:12:29Z

There is an error in the log when import a workspace for the first time in the debug mode:

!ENTRY org.eclipse.jdt.core 4 4 2023-11-02 11:11:09.411
!MESSAGE Exception while reading /jdt.ls-java-project/.classpath
!STACK 0
java.io.IOException: Bad format
	at org.eclipse.jdt.internal.core.JavaProject.decodeClasspath(JavaProject.java:1323)
	at org.eclipse.jdt.internal.core.JavaProject.readFileEntriesWithException(JavaProject.java:3081)
	at org.eclipse.jdt.internal.core.JavaProject.readFileEntries(JavaProject.java:3091)
	at org.eclipse.jdt.internal.core.JavaProject.writeFileEntries(JavaProject.java:3475)
	at org.eclipse.jdt.internal.core.JavaModelManager$PerProjectInfo.writeAndCacheClasspath(JavaModelManager.java:1570)
	at org.eclipse.jdt.internal.core.JavaModelManager$PerProjectInfo.writeAndCacheClasspath(JavaModelManager.java:1582)
	at org.eclipse.jdt.internal.core.SetClasspathOperation.executeOperation(SetClasspathOperation.java:81)
	at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:740)
	at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2453)
	at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2478)
	at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:811)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3685)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3645)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3701)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.createJavaProject(ProjectsManager.java:439)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.createJavaProject(ProjectsManager.java:366)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.initializeProjects(ProjectsManager.java:114)
	at org.eclipse.jdt.ls.core.internal.handlers.InitHandler$1.runInWorkspace(InitHandler.java:260)
	at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:43)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; Premature end of file.
	at java.xml/com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:262)
	at java.xml/com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:342)
	at org.eclipse.jdt.internal.core.JavaProject.decodeClasspath(JavaProject.java:1321)
	... 19 more

Not sure if this is related with my dev environment or it is an issue.

mickaelistria · 2023-11-06T09:28:49Z

Can you please describe the exact steps to reproduce this issue? Do you think it can be captured in a unit test?

jdneo · 2023-11-07T02:55:25Z

Sure.

I'm using VS Code + PDE to launch this PR.
Remove the workspace folder runtime-JDT-LS that is used at debug runtime entirely. To find that folder, you can
1. Trigger the command Open Java Language server log file
2. Reveal the opened log file in system file explorer
3. you can find it going up: ../../runtime-JDT-LS
Use jdt.ls.socket-stream.launch to launch the JDT.LS
Use the vscode launch config Launch Extension - JDTLS Client to launch the client
After import finished, open the server log: runtime-JDT-LS/.metadata/.log

mickaelistria · 2023-11-07T13:04:11Z

Good news: I can reproduce it with a test, I'll just add a log listener to capture the issue so it may fail the test.

mickaelistria · 2023-11-07T16:01:43Z

@jdneo I have added a check for this issue and fixed it.

jdneo · 2023-11-08T08:04:52Z

@mickaelistria Found a new issue on a MacOS machine today when I tried this PR. I think the root cause is that the accuracy of attributes.creationTime().toInstant() and Instant.now() are different, which makes some of the metadata file fail to be linked.

Below are what I observed.

Track the start time before calling Instant creationRequestInstant = Instant.now();

Track the end time and the duration after a project is created. From the screenshot, you can see that the duration is 68787125 nano seconds, which is ~ 68ms.

Because the duration is so short and the different accuracy when comparing two time instant, it fails to return the expected result:

mickaelistria · 2023-11-08T12:29:02Z

Thanks for the test and report analysis @jdneo !
I've update the patch so that isNewer skips sub-second accuracy. Tests seem good so far; can you please check on macOS?

jdneo · 2023-11-09T02:56:52Z

It's working on my Mac now.

Though, I'm not sure only comparing on second is safe enough. Because 1 second can really do so many things for a computer. Not to mention that the hardware will be more powerful as the time goes by.

mickaelistria · 2023-11-09T06:31:57Z

I don't see a way to go reliably under the precision of creationTime (which is good on Linux and Windows apparently). Do you know a way to get more precise creationTime for a file on Mac?

mickaelistria · 2023-11-09T10:07:44Z

I tried something else; dynamically looking at accuracy. Can you please check whether it works on macOS? If it does, then I think it's a good enough fix; that will scale when macOS filesystem gets better precision.

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

jdneo · 2023-11-14T08:02:59Z

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

+			ChronoUnit smallestSupportedUnit = Stream.of(ChronoUnit.NANOS, ChronoUnit.MILLIS, ChronoUnit.SECONDS) // IMPORTANT: keeps units in Stream from finer to coarser
+				.filter(creationInstant::isSupported).filter(instant::isSupported) //
+				.findFirst().orElse(null);
+			if (smallestSupportedUnit != null) {


Check on MacOS today, looks like we still have the problem here.

smallestSupportedUnit is assigned to ChronoUnit.NANOS, though the accuracy of creationInstant is second, makes this method returns false finally.

OK thanks, so should we just hardcode the rule for macOS to trim to the second?

I guess so. I couldn't think of any better solution for this case.

(I was thinking about find out all the already existing metadata files before import started, but that may have too much perf penalty and may be not worth it.)

I was thinking about find out all the already existing metadata files before import started, but that may have too much perf penalty and may be not worth it.

The difficulty here is that we don't know at that stage what folders the importers are going to consider as projects, so we're not sure which files matter or not.

I guess you can just those files start from rootPaths, because all the metadata files that exist before import happens will not be linked, so we don't need to care about what folders will be imported by which importers.

I'm not optimistic about the performance though...

You mean recursively crawling the whole subtrees under rootPAths for pre-existing .classpath, .project and so on?
Indeed, it could impact heavily performance for users using the wrong root (eg their home or filesystem root), but for a regular development project, it should be fast enough (I don't expect it to take more than 1 seconds for the biggest projects). But if we're ready to loose 1 second, we can just put a 1 second wait directly after Instant.now , and the check will then be reliable on all OSs ;)

In the new patch, I've hardcoded that Instant should be truncated and compared by the second on macOS.

But if we're ready to loose 1 second, we can just put a 1 second wait directly after Instant.now , and the check will then be reliable on all OSs

That's a good insight! I think there are some key metrics we need to track when roll out this change. Then we can make the decision based on the data, like:

how many users happen to meet the two equally instants on second during import.

how many users fail to link the files.

For example, if the data shows that very few users have two equally instant on second, then we can just simply trim to the second.

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

Use linked resources instead of filesystem hack as this one can return invalid locations through standard API. This removes the filesystem bundle, the logic is now moved in ProjectManagers and a workspace listener takes care of using linked resources for metadata. This also remove the InvisibleProjectMetadataTest.testMetadataFileLocation() test because creating a new project in Eclipse workspace now enforces creation of a .settings/org.eclipse.core.resources.prefs file. Where the metadata for the invisible project is stored doesn't matter as the internal project is internal and not visible to user anyway.

jdneo · 2023-11-15T02:53:58Z

I'm thinking about this case that this solution might not be able to handle:

User opens a large-scale project, the import takes a long time.
Before the import finishes, user closes VS Code.
Now, part of the modules are imported with metadata files generated at project root, but not enough time to link them. And they will be persisted at the project root since then.

mickaelistria force-pushed the linked-resources branch from 5b73422 to 853e6e5 Compare May 16, 2023 15:51

mickaelistria force-pushed the linked-resources branch 3 times, most recently from 98a4eb6 to 02b875a Compare June 9, 2023 20:59

mickaelistria force-pushed the linked-resources branch 3 times, most recently from cb10a2b to f89ed19 Compare June 12, 2023 14:04

mickaelistria force-pushed the linked-resources branch 2 times, most recently from 8b5604d to 0dd82bd Compare June 20, 2023 14:56

mickaelistria marked this pull request as draft June 20, 2023 20:00

mickaelistria force-pushed the linked-resources branch 4 times, most recently from 80e718d to 711168f Compare June 22, 2023 16:49

mickaelistria force-pushed the linked-resources branch 3 times, most recently from 480faf8 to fae8c0e Compare July 2, 2023 17:10

mickaelistria marked this pull request as ready for review July 2, 2023 20:00

mickaelistria force-pushed the linked-resources branch from fae8c0e to a0d6f72 Compare July 2, 2023 20:12

rgrunber added enhancement debt labels Jul 4, 2023

mickaelistria force-pushed the linked-resources branch from 27f6734 to 00b2937 Compare October 30, 2023 13:54

mickaelistria force-pushed the linked-resources branch from 00b2937 to 0740db9 Compare October 31, 2023 07:33

jdneo reviewed Oct 31, 2023

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/LinkResourceUtil.java Show resolved Hide resolved

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/LinkResourceUtil.java Outdated Show resolved Hide resolved

mickaelistria force-pushed the linked-resources branch from 0740db9 to 1d172c7 Compare October 31, 2023 09:31

mickaelistria force-pushed the linked-resources branch from 1d172c7 to ef20a90 Compare November 7, 2023 13:23

mickaelistria force-pushed the linked-resources branch from ef20a90 to 3dfddaf Compare November 8, 2023 12:26

mickaelistria force-pushed the linked-resources branch from 3dfddaf to f2d66aa Compare November 9, 2023 10:06

fbricon reviewed Nov 9, 2023

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/LinkResourceUtil.java Outdated Show resolved Hide resolved

mickaelistria force-pushed the linked-resources branch 2 times, most recently from 8a3d0e7 to e608650 Compare November 9, 2023 10:52

fbricon reviewed Nov 9, 2023

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/LinkResourceUtil.java Outdated Show resolved Hide resolved

mickaelistria force-pushed the linked-resources branch from e608650 to 100c00e Compare November 9, 2023 11:32

jdneo reviewed Nov 14, 2023

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/LinkResourceUtil.java Show resolved Hide resolved

mickaelistria force-pushed the linked-resources branch from 100c00e to 633da38 Compare November 14, 2023 09:08

rgrunber mentioned this pull request Dec 28, 2023

Cannot import maven project from workspace #2999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use linked resource instead of filesystem #2658

Use linked resource instead of filesystem #2658

mickaelistria commented May 16, 2023

rgrunber commented May 16, 2023

mickaelistria commented May 16, 2023

fbricon commented May 16, 2023

mickaelistria commented May 16, 2023

mickaelistria commented Jun 26, 2023

mickaelistria commented Jul 10, 2023

mickaelistria commented Jul 19, 2023

mickaelistria commented Sep 15, 2023

rgrunber commented Sep 15, 2023

jdneo commented Oct 31, 2023

mickaelistria commented Oct 31, 2023

jdneo commented Nov 2, 2023 •

edited

mickaelistria commented Nov 6, 2023

jdneo commented Nov 7, 2023 •

edited

mickaelistria commented Nov 7, 2023

mickaelistria commented Nov 7, 2023

jdneo commented Nov 8, 2023

mickaelistria commented Nov 8, 2023

jdneo commented Nov 9, 2023 •

edited

mickaelistria commented Nov 9, 2023

mickaelistria commented Nov 9, 2023

jdneo Nov 14, 2023

mickaelistria Nov 14, 2023

jdneo Nov 14, 2023

mickaelistria Nov 14, 2023

jdneo Nov 14, 2023

mickaelistria Nov 14, 2023

mickaelistria Nov 14, 2023

jdneo Nov 14, 2023

jdneo commented Nov 15, 2023

Use linked resource instead of filesystem #2658

Are you sure you want to change the base?

Use linked resource instead of filesystem #2658

Conversation

mickaelistria commented May 16, 2023

rgrunber commented May 16, 2023

mickaelistria commented May 16, 2023

fbricon commented May 16, 2023

mickaelistria commented May 16, 2023

mickaelistria commented Jun 26, 2023

mickaelistria commented Jul 10, 2023

mickaelistria commented Jul 19, 2023

mickaelistria commented Sep 15, 2023

rgrunber commented Sep 15, 2023

jdneo commented Oct 31, 2023

mickaelistria commented Oct 31, 2023

jdneo commented Nov 2, 2023 • edited

mickaelistria commented Nov 6, 2023

jdneo commented Nov 7, 2023 • edited

mickaelistria commented Nov 7, 2023

mickaelistria commented Nov 7, 2023

jdneo commented Nov 8, 2023

mickaelistria commented Nov 8, 2023

jdneo commented Nov 9, 2023 • edited

mickaelistria commented Nov 9, 2023

mickaelistria commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdneo commented Nov 15, 2023

jdneo commented Nov 2, 2023 •

edited

jdneo commented Nov 7, 2023 •

edited

jdneo commented Nov 9, 2023 •

edited