Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for multi-file source launcher. #6329

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Aug 11, 2023

This patch is adding a support for (the future) multi-file source launcher:
https://openjdk.org/jeps/8304400

I was thinking (and tried for a bit) to mask this as a project, but it seemed the incompatibilities between "just a group of files" and "project" are too big. So, the patch enhances the needed queries to handle files in non-project directories (hopefully) reasonably well.

I've created a separate module to host the support for source launcher, to not pollute java.api.common.

Note this improves the handling of single-file source launching under debugger, when there's a package clause present (as long as the directory structure matches the package clause).

@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests labels Aug 11, 2023
@lahodaj lahodaj added this to the NB20 milestone Aug 11, 2023
@lahodaj lahodaj requested review from dbalek and sdedic August 11, 2023 10:33
@lahodaj lahodaj marked this pull request as ready for review August 11, 2023 10:33
@apache apache locked and limited conversation to collaborators Oct 6, 2023
@apache apache unlocked this conversation Oct 6, 2023
@neilcsmith-net neilcsmith-net modified the milestones: NB20, NB21 Oct 17, 2023
@apache apache locked and limited conversation to collaborators Nov 10, 2023
@apache apache unlocked this conversation Nov 10, 2023
@lahodaj lahodaj requested a review from sdedic November 14, 2023 12:30
@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 14, 2023

@sdedic, I believe I've updated the patch based on your comments, and generally improved it. Could you please take a look? Thanks!

@@ -315,7 +316,10 @@ public void indexingComplete(Set<URL> indexedRoots) {
delegates = this.delegates.toArray(new TextDocumentServiceImpl[this.delegates.size()]);
}
for (TextDocumentServiceImpl delegate : delegates) {
delegate.reRunDiagnostics();
ProxyLookup augmentedLookup = new ProxyLookup(Lookups.fixed(delegate.client), Lookup.getDefault());
Lookups.executeWith(augmentedLookup, () -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not needed, but Server.ConsumeWithLookup also passes sessionLookup contents and OperationContext to client requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note this runs outside of the event loop, this runs when indexing finishes in a background thread, so the client is not set.

Note this also relates to what settings are used during indexing - there are merged settings across all clients for used by the indexing (see SingleFileOptionsQueryImpl.globalOptions), but for the editor, we can use the sharper per-client settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. My point was that this wrapper sets up "client" (remembered in the past) for the given TextDocumentServiceImpl instance; so eventually a server > client notification/request finds its target. But an eventual Progress API call made during the TextDocumentService processing (that assumes OperationContext) will not find its per-client data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I misunderstood this to say this code is not needed. I suspect we don't have a meaningful OperationContext (as this runs asynchronously, and we probably should report any progress here anyway). Is there a way to get the sessionLookup? I don't think it is strictly necessary, but we could include that, in case it would be useful later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have that - but you could eventually expose it from LspServerState implemented by LanguageServerImpl . Maybe just make a comment that the lookup is different so when an issue pops up, this conversation will be reminded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I've added this:
40afb55

java/java.lsp.server/vscode/src/nbcode.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@singh-akhilesh singh-akhilesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +84 to 85
public static Process compileJavaSource(FileObject fileObject) {
FileObject javac = JavaPlatformManager.getDefault().getDefaultPlatform().findTool("javac"); //NOI18N
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about adding a JDK selector to the file properties of single source programs for NB 21 to decouple it from the runtime JDK. Is something like this planned? Will this still work in VSCode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I (and possibly others) are considering separating the runtime JDK and the JavaPlatform used by projects in VS Code extension. Maybe by changing the result of JavaPlatformManager.getDefaultPlatform().

This is because the extension will/may require some relatively new JDK, but the projects may be developed for an older version.

I don't think anyone really tried that so far, though. (Jaroslav did some change in this direction for Maven long time ago, I think, but we need something more general.)

For source launcher, we could add a UI to select the JDK - it probably would not work in VS Code, but the same is true for JDK selection for other project types. If you want to look at that, you are of course welcome to. Or someone else might get to it, although I can't promise that, of course.

But I would prefer to avoid doing that here - this PR is complex enough on it own.

Thanks for the comment!

Copy link
Member

@mbien mbien Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I (and possibly others) are considering separating the runtime JDK and the JavaPlatform used by projects in VS Code extension. Maybe by changing the result of JavaPlatformManager.getDefaultPlatform().

yes but since free floating java files are not associated with a project, users can't even influence on which JDK they want to run the single/multi-file-java-program (SMFJP?). Which is independent of the default JDK vs nb runtime discussion. Controlling this over the default JDK is one way of doing this, but what if users don't want to run a file on the default JDK but on a specific JDK?

A JDK selector could be potentially added to the file properties, just like we have in maven/ant/gradle projects.
SMFJP-jdk-selector

But I would prefer to avoid doing that here - this PR is complex enough on it own.

agreed

@lahodaj
Copy link
Contributor Author

lahodaj commented Dec 5, 2023

If there are no objections, I'll rebase, squash and if GH Actions are green integrate. Seems now is a good time - there should be some time to find and fix issues before the next release. Also 'JEP 458: Launch Multi-File Source-Code Programs' has been integrated today, so it is a high time to have a support for it. (JEP 458 is not the only usecase for this, but it is one of the major ones.)

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking forward to this feature :) (code looks good to me and smoke test worked too)

@lahodaj lahodaj merged commit 5b768b8 into apache:master Dec 6, 2023
36 checks passed
@matthiasblaesing
Copy link
Contributor

It seems this integration causes issues in indexing:

java.io.IOException: Setting attribute not allowed for JarFileSystem [/home/matthias/bin/jdk-17/lib/src.zip!java.base <- single_file_vm_options=]
	at org.openide.filesystems.JarFileSystem.writeAttribute(JarFileSystem.java:622)
	at org.openide.filesystems.JarFileSystem$Impl.writeAttribute(JarFileSystem.java:1173)
	at org.openide.filesystems.AbstractFileObject.setAttribute(AbstractFileObject.java:342)
	at org.openide.filesystems.AbstractFileObject.setAttribute(AbstractFileObject.java:325)
[catch] at org.netbeans.modules.java.file.launcher.SharedRootData.setNewProperties(SharedRootData.java:108)
	at org.netbeans.modules.java.file.launcher.SharedRootData.<init>(SharedRootData.java:84)
	at org.netbeans.modules.java.file.launcher.SharedRootData.lambda$ensureRootRegistered$0(SharedRootData.java:49)
	at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1220)
	at org.netbeans.modules.java.file.launcher.SharedRootData.ensureRootRegistered(SharedRootData.java:49)
	at org.netbeans.modules.java.file.launcher.indexer.CompilerOptionsIndexer.index(CompilerOptionsIndexer.java:41)
	at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor$2.run(Indexable.java:138)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runIndexer(RepositoryUpdater.java:274)
	at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor.index(Indexable.java:136)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doIndex(RepositoryUpdater.java:2749)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.lambda$index$0(RepositoryUpdater.java:2626)
	at org.netbeans.modules.parsing.impl.indexing.errors.TaskCache.refreshTransaction(TaskCache.java:540)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.index(RepositoryUpdater.java:2625)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$AbstractRootsWork.lambda$scanSource$3(RepositoryUpdater.java:5735)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.lambda$runInContext$4(RepositoryUpdater.java:2119)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2117)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2098)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.access$1400(RepositoryUpdater.java:135)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$AbstractRootsWork.scanSource(RepositoryUpdater.java:5770)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$AbstractRootsWork.scanSources(RepositoryUpdater.java:5443)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$RootsWork.getDone(RepositoryUpdater.java:5075)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doTheWork(RepositoryUpdater.java:3452)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task._run(RepositoryUpdater.java:6197)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.access$3400(RepositoryUpdater.java:5855)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.lambda$call$0(RepositoryUpdater.java:6116)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
	at org.netbeans.modules.parsing.impl.RunWhenScanFinishedSupport.performScan(RunWhenScanFinishedSupport.java:83)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6116)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6112)
	at org.netbeans.modules.masterfs.filebasedfs.utils.FileChangedManager.priorityIO(FileChangedManager.java:153)
	at org.netbeans.modules.masterfs.providers.ProvidedExtensions.priorityIO(ProvidedExtensions.java:335)
	at org.netbeans.modules.parsing.nb.DataObjectEnvFactory.runPriorityIO(DataObjectEnvFactory.java:118)
	at org.netbeans.modules.parsing.impl.Utilities.runPriorityIO(Utilities.java:67)
	at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.run(RepositoryUpdater.java:6112)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
	at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
	at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)

My indication is, that the issue is rooted in classes introduced in this PR and I observe this since a few minutes where I merged recent changes from master.

@lahodaj please have a look

@matthiasblaesing
Copy link
Contributor

matthiasblaesing commented Dec 7, 2023

And this also indicates that this is fishy:

Parent does not exist /tmp/vcs-1701986802781/vcs-1701986926097
Parent exists: /tmp/vcs-1701986802781
Has children []
msg
Parent does not exist /tmp/vcs-1701986802781/vcs-1701986926097
Parent exists: /tmp/vcs-1701986802781
Has children []
Caused: java.io.FileNotFoundException: Can't read /tmp/vcs-1701986802781/vcs-1701986926097/CargoPanel.java
	at org.netbeans.modules.masterfs.filebasedfs.fileobjects.FileObj.getInputStream(FileObj.java:194)
	at org.openide.filesystems.FileObject.asBytes(FileObject.java:687)
[catch] at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.lambda$getSourcePath$1(MultiSourceRootProvider.java:98)
	at java.base/java.util.Map.computeIfAbsent(Map.java:1054)
	at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.getSourcePath(MultiSourceRootProvider.java:96)
	at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.getSourceRoot(MultiSourceRootProvider.java:160)
	at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.isSourceLauncher(MultiSourceRootProvider.java:168)
	at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.getBootPath(MultiSourceRootProvider.java:172)
	at org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider.findClassPath(MultiSourceRootProvider.java:86)
	at org.netbeans.api.java.classpath.ClassPath.getClassPath(ClassPath.java:667)
	at org.netbeans.api.java.source.ClasspathInfo.create(ClasspathInfo.java:412)
	at org.netbeans.api.java.source.ClasspathInfo.create(ClasspathInfo.java:287)
	at org.netbeans.modules.java.source.parsing.JavacParser.init(JavacParser.java:282)
	at org.netbeans.modules.java.source.parsing.JavacParser.parseImpl(JavacParser.java:427)
	at org.netbeans.modules.java.source.parsing.JavacParser.parse(JavacParser.java:366)
	at org.netbeans.modules.parsing.impl.TaskProcessor.callParse(TaskProcessor.java:598)
	at org.netbeans.modules.parsing.impl.SourceCache.getResult(SourceCache.java:230)
	at org.netbeans.modules.parsing.api.ResultIterator.getParserResult(ResultIterator.java:115)
	at org.netbeans.api.java.source.JavaSource$MultiTask.run(JavaSource.java:496)
	at org.netbeans.modules.parsing.impl.TaskProcessor.callUserTask(TaskProcessor.java:586)
	at org.netbeans.modules.parsing.api.ParserManager$UserTaskAction.run(ParserManager.java:132)
	at org.netbeans.modules.parsing.api.ParserManager$UserTaskAction.run(ParserManager.java:116)
	at org.netbeans.modules.parsing.impl.TaskProcessor$2.call(TaskProcessor.java:181)
	at org.netbeans.modules.parsing.impl.TaskProcessor$2.call(TaskProcessor.java:178)
	at org.netbeans.modules.masterfs.filebasedfs.utils.FileChangedManager.priorityIO(FileChangedManager.java:153)
	at org.netbeans.modules.masterfs.providers.ProvidedExtensions.priorityIO(ProvidedExtensions.java:335)
	at org.netbeans.modules.parsing.nb.DataObjectEnvFactory.runPriorityIO(DataObjectEnvFactory.java:118)
	at org.netbeans.modules.parsing.impl.Utilities.runPriorityIO(Utilities.java:67)
	at org.netbeans.modules.parsing.impl.TaskProcessor.runUserTask(TaskProcessor.java:178)
	at org.netbeans.modules.parsing.api.ParserManager.parse(ParserManager.java:83)
	at org.netbeans.api.java.source.JavaSource.runUserActionTaskImpl(JavaSource.java:454)
	at org.netbeans.api.java.source.JavaSource.runUserActionTask(JavaSource.java:425)
	at org.netbeans.modules.java.JavaNode$IconTask$SourceIcon.computeIcon(JavaNode.java:518)
	at org.netbeans.modules.java.JavaNode$IconTask.run(JavaNode.java:480)
	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
	at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
	at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
	at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)
ALL [null]: Cannot read file CargoPanel.java in /tmp/vcs-1701986802781/vcs-1701986926097.
Parent does not exist /tmp/vcs-1701986802781/vcs-1701986926097
Parent exists: /tmp/vcs-1701986802781

This was reported when I opened a java file associated with a .form file.

@lahodaj
Copy link
Contributor Author

lahodaj commented Dec 8, 2023

Uh sorry for this. I'll be looking into it.

@matthiasblaesing matthiasblaesing self-assigned this Dec 10, 2023
@matthiasblaesing
Copy link
Contributor

This seems to be a minimal hotfix:

diff --git a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/SharedRootData.java b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/SharedRootData.java
index 9939a96e8152..3c9d9099224a 100644
--- a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/SharedRootData.java
+++ b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/SharedRootData.java
@@ -25,6 +25,8 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import org.netbeans.api.annotations.common.CheckForNull;
 import org.netbeans.modules.java.file.launcher.api.SourceLauncher;
@@ -43,6 +45,8 @@ import org.openide.util.Exceptions;
  */
 public class SharedRootData {

+    private static final Logger LOG = Logger.getLogger(SharedRootData.class.getName());
+
     private static final Map<FileObject, SharedRootData> root2Data = new HashMap<>();

     public static synchronized void ensureRootRegistered(FileObject root) {
@@ -104,11 +108,14 @@ public class SharedRootData {
         }
         String joinedCommandLine = SourceLauncher.joinCommandLines(options.values());
         try {
-            if (!joinedCommandLine.equals(root.getAttribute(SingleSourceFileUtil.FILE_VM_OPTIONS))) {
+            if (
+                    !root.getFileSystem().isReadOnly() // Skip read-only FSes (like JarFileSystem)
+                    && !joinedCommandLine.equals(root.getAttribute(SingleSourceFileUtil.FILE_VM_OPTIONS))
+            ) {
                 root.setAttribute(SingleSourceFileUtil.FILE_VM_OPTIONS, joinedCommandLine);
             }
         } catch (IOException ex) {
-            Exceptions.printStackTrace(ex);
+            LOG.log(Level.INFO, "Failed to set " + SingleSourceFileUtil.FILE_VM_OPTIONS + " for " + root.getPath(), ex);
         }
     }

diff --git a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
index 47a8bad00c02..7f5061d3fac4 100644
--- a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
+++ b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
@@ -92,7 +92,7 @@ public class MultiSourceRootProvider implements ClassPathProvider {
         if (DISABLE_MULTI_SOURCE_ROOT) return null;
         synchronized (this) {
             //XXX: what happens if there's a Java file in user's home???
-            if (file.isData() && "text/x-java".equals(file.getMIMEType())) {
+            if (file.isValid() && file.isData() && "text/x-java".equals(file.getMIMEType())) {
                 return file2SourceCP.computeIfAbsent(file, f -> {
                     try {
                         String content = new String(file.asBytes(), FileEncodingQuery.getEncoding(file));

I think the general problem is Exceptions.printStackTrace(ex);. That function does more than just logging the problem it also seems to block further processing. Switching to a regular logger seems sensible and less intrusive.

@lahodaj
Copy link
Contributor Author

lahodaj commented Dec 11, 2023

@matthiasblaesing, seems sensible to me. Will you please open a PR?

Regarding Exceptions.printStrackTrace not sure what you mean. I don't think it does anything super special.

@matthiasblaesing
Copy link
Contributor

@lahodaj for the changes I created #6832

For Exceptions.printStrackTrace I got the feeling, that once that is invoked, further executions of that code path is blocked until the problem is acknowledged, but that might be just a misinterpretation/misobservation ;-)

@mbien
Copy link
Member

mbien commented Dec 15, 2023

@lahodaj @matthiasblaesing i believe this causes another regression take the following snippet:

import java.util.stream.Gatherers;
public class SingleFileProgram {
    public static void main(String[] args) {
        System.out.println(Gatherers.class);
    }
}

open it in NB 20 started on JDK 22ea (build 27+ required)

  • you should see a hint suggesting to enable preview, run that hint
  • everything should be fine, errors will go away

repeat with master started on JDK 22ea

  • hint will successfully add the --enable-preview --source 22 line to the file properties
  • however, the errors will not go away, even after NB restart
  • (small followup bug: running the hint again will now add another --enable-preview flag to the properties)

@lahodaj
Copy link
Contributor Author

lahodaj commented Dec 15, 2023

Uh, I always forget that the java.lsp.server runs in ordinary IDE run as well, so any services registered there would be used for ordinary runs as well, like, in this case org.netbeans.modules.java.lsp.server.singlesourcefile.SingleFileOptionsQueryImpl. Will move the registration into nbcode.

@lahodaj
Copy link
Contributor Author

lahodaj commented Dec 15, 2023

FYI, opened:
#6854

@neilcsmith-net
Copy link
Member

@lahodaj I'm just updating a couple of platform applications to NB21, and the addition of the java.file.launcher dependency in refactoring.java is causing some issues. I'd really like to not bring in that dependency and its dependencies (particularly as I have to use the kill switch too).

Is there any way we could achieve what you need in refactoring.java in NB22 while removing the hard dependency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants