-
Notifications
You must be signed in to change notification settings - Fork 843
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
Conversation
...on/src/org/netbeans/modules/java/api/common/queries/GenericModuleInfoAccessibilityQuery.java
Outdated
Show resolved
Hide resolved
...on/src/org/netbeans/modules/java/api/common/queries/GenericModuleInfoAccessibilityQuery.java
Outdated
Show resolved
Hide resolved
...on/src/org/netbeans/modules/java/api/common/queries/GenericModuleInfoAccessibilityQuery.java
Show resolved
Hide resolved
...on/src/org/netbeans/modules/java/api/common/queries/GenericModuleInfoAccessibilityQuery.java
Outdated
Show resolved
Hide resolved
...on/src/org/netbeans/modules/java/api/common/queries/GenericModuleInfoAccessibilityQuery.java
Outdated
Show resolved
Hide resolved
...le.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
Show resolved
Hide resolved
...er/src/org/netbeans/modules/java/file/launcher/queries/SingleSourceCompilerOptQueryImpl.java
Outdated
Show resolved
Hide resolved
...er/src/org/netbeans/modules/java/file/launcher/queries/SingleSourceCompilerOptQueryImpl.java
Outdated
Show resolved
Hide resolved
...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java
Outdated
Show resolved
Hide resolved
...rver/src/org/netbeans/modules/java/lsp/server/singlesourcefile/CompilerOptionsQueryImpl.java
Outdated
Show resolved
Hide resolved
b1eba09
to
e1da983
Compare
e1da983
to
552c720
Compare
@sdedic, I believe I've updated the patch based on your comments, and generally improved it. Could you please take a look? Thanks! |
java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/api/SourceLauncher.java
Outdated
Show resolved
Hide resolved
...uncher/src/org/netbeans/modules/java/file/launcher/queries/LauncherSourceLevelQueryImpl.java
Outdated
Show resolved
Hide resolved
...le.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
Outdated
Show resolved
Hide resolved
...le.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
Outdated
Show resolved
Hide resolved
...le.launcher/src/org/netbeans/modules/java/file/launcher/queries/MultiSourceRootProvider.java
Show resolved
Hide resolved
@@ -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, () -> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
552c720
to
5f0f098
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
public static Process compileJavaSource(FileObject fileObject) { | ||
FileObject javac = JavaPlatformManager.getDefault().getDefaultPlatform().findTool("javac"); //NOI18N |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
But I would prefer to avoid doing that here - this PR is complex enough on it own.
agreed
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.) |
40afb55
to
192bb52
Compare
There was a problem hiding this 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)
It seems this integration causes issues in indexing:
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 |
And this also indicates that this is fishy:
This was reported when I opened a java file associated with a |
Uh sorry for this. I'll be looking into it. |
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 |
@matthiasblaesing, seems sensible to me. Will you please open a PR? Regarding |
@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)
repeat with master started on JDK 22ea
|
Uh, I always forget that the |
FYI, opened: |
@lahodaj I'm just updating a couple of platform applications to NB21, and the addition of the Is there any way we could achieve what you need in |
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).