Skip to content

[NETBEANS-6881][NETBEANS-6880][NETBEANS-6921] Additional fixes for the multi-file launcher:#6912

Merged
lahodaj merged 1 commit intoapache:masterfrom
lahodaj:multi-file-launcher-fixes
Jan 14, 2024
Merged

[NETBEANS-6881][NETBEANS-6880][NETBEANS-6921] Additional fixes for the multi-file launcher:#6912
lahodaj merged 1 commit intoapache:masterfrom
lahodaj:multi-file-launcher-fixes

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jan 3, 2024

  • checking for null return result for getClassPath(COMPILE)
  • filtering out directories that belong to a project from the multi-file launcher source path
  • more thoroughly prevent creating of the multi-file launcher source ClassPath for files under projects

@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 Jan 3, 2024
@lahodaj lahodaj added this to the NB21 milestone Jan 3, 2024
@lahodaj lahodaj requested a review from sdedic January 3, 2024 12:18
@matthiasblaesing
Copy link
Contributor

Thank you for the update - however, that behavior seem still to be buggy. I have an tmp folder with Payara (Jakarta EE server) expanded inside it. The moment I create a "test.java" file in tmp, the payara6 folder is marked as "error". To get a clean state I tested in a new folder:

image

Download Link for Payara

https://nexus.payara.fish/repository/payara-community/fish/payara/distributions/payara/6.2023.12/payara-6.2023.12.zip?hsCtaTracking=333f68b3-3177-4fdc-b6b6-360169c8fa4e%7C9d3ef224-15ca-4add-aeb3-caa5ecffee75

I did nothing (at least not consciously) to indicate to NetBeans, that it should touch the payara6 folder, so I'm surprised, that it gets scanned.

Two things make me nervous:

  • I see the disaster that is indexing huge loads of JS code - the indexer is useless there, but burns away my CPU
  • I tried to work with NetBeans on nb-javac and the moment NetBeans picks of the OpenJDK checkout, it becomes unusable

Both cases need to be fixed at some point, but at least they are projects and thus there is a reason for indexing. If a single java file is enough to trigger indexing the whole subfolder structure, this cries problem.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 3, 2024

@matthiasblaesing, I wonder what is your proposal on how things should work (or if it is just "exactly the same way as before").

More specifically, JEP 458:
https://openjdk.org/jeps/458

is part of JDK 22. I personally don't need a support for it at all, and but for various reasons there's a need to support it in the VS Code extension.

This JEP is basically "any groups of files anywhere on a disk may be used as a project by running one of them". Given how project-oriented NetBeans is, it is not really clear how to approach it. And, to get some features work reliably or at all, we need the index.

There are various ways to approach this, including:

  1. when we encounter a file, we find the corresponding source root, and index it. This is the current behavior. Works until someone tries to open something too big and weird in Favorites.
  2. as above, just don't index. Things may work worse, but less change in behavior from the NetBeans 20 state.
  3. as 1., but have some predefined directories which we would ignore, like /tmp or $HOME. I don't really this, though.
  4. as 1., but only when the file is opened in the editor, not aggressively when its seen.
  5. force the user to somehow mark the directories and/or files that should be part of the multi-source handling.
  6. not support JEP 458.

I guess I am fine with any approach inside the NetBeans/Swing UI. I don't think I would like to turn down the behavior for the VS Code extension, but it should be possible to have different behaviors. (Moreover, given the "Workspace" concept, the issue does not really exist in VS Code.)

Thanks!

@matthiasblaesing
Copy link
Contributor

I'm not sure what is the "right" answer. I think that IDE support will always be hard for this kind of project (sorry about the word-play). The interesting bit I read there is, that the launcher must ignore all java files, that are not referenced by the file being run:

Only .java files whose classes are referenced by the program are compiled. This allows developers to play with new versions of code without worrying that old versions will be compiled accidentally. For example, suppose the directory also contains OldProg.java, whose older version of the Prog class expects the Helper class to have a method named go rather than run. The presence of OldProg.java, with its latent error, is immaterial when running Prog.java.

So what can you expect from the IDE in such a mess flexible setup? For example should refactoring tools ignore non referenced files? Should errors flagged in "renamed" files? Should references between the components be checked?

I only worked with VSCode shortly and my understanding is, that what you called "Workspace" is a "directory" opened for working with it. This sounds an awful lot like the "Lightweight C/C++-Project" for LSP based native support and would also match option 5 you listed. I think at some point we'll need an "ephemeral project" manager to remove such projects, but a stop gap?!

@mbien
Copy link
Member

mbien commented Jan 5, 2024

just a thought:

NetBeans can store file properties for java files (as used for jvm/program args). I am wondering if it would be a better default, to have the multi-file mode disabled by default (no scanning), and only enable it when a flag on the (main) file is set? (VSCode can turn this on by default if it wants to)

This could be even an editor hint at some point in future which sets the flag. (the hint could do some "probing" before suggesting to enable scanning for the folder the file is in)

edit: so essentially the main file can become the "project" in a way, but it isn't one by default

@mbien
Copy link
Member

mbien commented Jan 6, 2024

just to be sure I tested this PR and it doesn't influence #6921, which is another regression caused by the multi-file launcher PR.

@neilcsmith-net
Copy link
Member

Just catching up on release related tasks, and so not looked fully in depth at the changes. However, we have a policy in place that master should always be in a releasable state. The number of release critical issues here is a concern. Unless there is an obvious fix here, then either the feature addition needs reverting, or it needs switching off (by default) before we freeze and branch.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 8, 2024

FWIW, I'll make an editor hint, to explicitly enable the scanning. I'll try to work on that today. I'll also need to investigate the #6921 - I thought that was already solved, but apparently not. Sorry for trouble.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 8, 2024

So, I've tweaked the behavior by:

  • never touching read-only filesystems. This is an enhancement to what @matthiasblaesing did a few weeks back, this time the source launcher queries simply should not respond to files on read-only filesystems.
  • by default, the source CP is not registered into the GlobalPathRegistry, which means it should not be indexed. There's a hint for the user to register the path, and there's also a location-based setting to override the behavior in the VS Code extension. (The user choice is persistent, and there's no real UI to revert it back, aside from clearing a file attribute from the ${userdir}/var/attributes.xml file.)

Please let me know what you think.

@matthiasblaesing
Copy link
Contributor

Thank you. First quick test seems to indicate, that this improves the situation a lot. However I saw two issues while "playing" with this. One issue I see in the log is this:

[exec] WARNING [org.netbeans.modules.java.source.parsing.JavacParser]: ParserResultTask: org.netbeans.modules.java.file.launcher.hints.UnregisteredRootHint@763a180e doesn't provide phase, assuming RESOLVED

the other thing is this:

     [exec] WARNING [org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater]
     [exec] java.lang.NullPointerException: Passed null to FileOwnerQuery.getOwner(FileObject)
     [exec]     at org.netbeans.api.project.FileOwnerQuery.getOwner(FileOwnerQuery.java:84)
     [exec]     at org.netbeans.modules.lsp.client.bindings.CustomIndexerImpl.lambda$index$1(CustomIndexerImpl.java:63)
     [exec]     at org.netbeans.modules.lsp.client.bindings.CustomIndexerImpl.handleStoredFiles(CustomIndexerImpl.java:91)
     [exec]     at org.netbeans.modules.lsp.client.bindings.CustomIndexerImpl.index(CustomIndexerImpl.java:52)
     [exec]     at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor$2.run(Indexable.java:138)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runIndexer(RepositoryUpdater.java:274)
     [exec]     at org.netbeans.modules.parsing.spi.indexing.Indexable$MyAccessor.index(Indexable.java:136)
     [exec] [catch] at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doIndex(RepositoryUpdater.java:2749)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.lambda$index$0(RepositoryUpdater.java:2626)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.errors.TaskCache.refreshTransaction(TaskCache.java:540)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.index(RepositoryUpdater.java:2625)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.lambda$scanFiles$2(RepositoryUpdater.java:3332)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.lambda$runInContext$4(RepositoryUpdater.java:2119)
     [exec]     at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2117)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.runInContext(RepositoryUpdater.java:2098)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater.access$1400(RepositoryUpdater.java:135)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.scanFiles(RepositoryUpdater.java:3290)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$FileListWork.getDone(RepositoryUpdater.java:3832)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Work.doTheWork(RepositoryUpdater.java:3452)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task._run(RepositoryUpdater.java:6197)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.access$3400(RepositoryUpdater.java:5855)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.lambda$call$0(RepositoryUpdater.java:6116)
     [exec]     at org.openide.util.lookup.Lookups.executeWith(Lookups.java:288)
     [exec]     at org.netbeans.modules.parsing.impl.RunWhenScanFinishedSupport.performScan(RunWhenScanFinishedSupport.java:83)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6116)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task$2.call(RepositoryUpdater.java:6112)
     [exec]     at org.netbeans.modules.masterfs.filebasedfs.utils.FileChangedManager.priorityIO(FileChangedManager.java:153)
     [exec]     at org.netbeans.modules.masterfs.providers.ProvidedExtensions.priorityIO(ProvidedExtensions.java:335)
     [exec]     at org.netbeans.modules.parsing.nb.DataObjectEnvFactory.runPriorityIO(DataObjectEnvFactory.java:118)
     [exec]     at org.netbeans.modules.parsing.impl.Utilities.runPriorityIO(Utilities.java:67)
     [exec]     at org.netbeans.modules.parsing.impl.indexing.RepositoryUpdater$Task.run(RepositoryUpdater.java:6112)
     [exec]     at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
     [exec]     at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
     [exec]     at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
     [exec]     at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
     [exec]     at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
     [exec]     at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)

The CompilerOptionsIndexer needs to take into account, that Context#getRoot can return null.

@mbien
Copy link
Member

mbien commented Jan 9, 2024

ran a few quick tests and can confirm that as long the hint is not activated by the user, NB behaves pretty much like it used to.

however my first "load test" already broke it when I added a java file to my home folder and activated the hint. NB scanned for a few minutes till it ran out of memory (default dev build seems to use 8GB max on my system with 32GB) and I was not able to cancel the scanning anymore at this point so i had to kill the process.

I get the feeling that it will be fairly difficult to find a satisfying solution since the location of the java file will fairly often not be the equivalent of a src/ folder. I essentially can't put a java file anywhere near my NetBeansProjects folder ;)

We don't have to rush anything now of course, we can disable the feature for a release in NB until we figure something out.
Something i would also like to see is a JDK selector in the file properties - this would coincidentally also fit very well to a srcpath/classpath field which is likely the more realistic approach compared to letting NB scan everything it sees relative to the java file.

edit as a side note:
the user will have to set the classpath anyway when the program runs in most cases. I just wrote a JDK 22 script for #6927 and I have to run it with:

java --enable-preview --source 22 -cp "lib/*" .github/scripts/BinariesListUpdates.java ./

edit2:
more thoughts:

  • the hint could be only active on cannot find symbol errors and then scan the java file imports and add the paths to the search path file property. This would follow the vague rules of the JEP section "How the launcher finds source files." This would be the initial value for the file's sourcepath property which can be still customized later by the user.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 9, 2024

So, regarding the indexing when the root (home directory) is registered - is there a reason to believe that's significantly different from when one makes the home directory a source root in a J2SE project? The multi-file handling is not really changing the way things are indexed, it just registers directories for indexing, as does J2SE project. (There's a bit of a difference due to the exclusions, which may load other projects, but that hopefully should not have that big impact.)

I am afraid I don't get the part related to srcpath. If there's a Java file without a package clause in the home directory, what else could be on the srcpath than the home directory? Individual files cannot be on a source path (and, inside NB, all entries in a path must be directories, that applies also to classpath, etc.) We'd need to exclude from the sourcepath, not add to it. Which is technically doable, it just seems extremely difficult.

There's a way to provide VM options (like classpath), in the NB GUI see the Properties window. It does not support *, but there may be pressure to add support for that.

I've pushed an updated that removes the hint, and the only way to enable registering of the roots now remains the branding.

@mbien
Copy link
Member

mbien commented Jan 9, 2024

So, regarding the indexing when the root (home directory) is registered - is there a reason to believe that's significantly different from when one makes the home directory a source root in a J2SE project?

There is probably no difference but this is a scenario which is very unlikely to happen, no? A user, e.g student can put a maven project into ~/ and everything will work fine. However the same student would have trouble to do the same with a java file, due to the fact that the root switches from ~/mavenproject1/src/main/java to ~/ and NB will scan most of the disk.
A user would have to explicitly put ~/ into the project path to replicate the problem due to the way how projects are structured.

Individual files cannot be on a source path

Ok I wasn't aware of this - this means most of the things I mentioned in the posts above wouldn't be possible anyway.

The way I understand the JEP and also the way JDK 22 behaves right now is that the java launcher is only touching files on-demand. If the source refers to foo.bar.Klass, the launcher will check if ./foo/bar/Klass.java exists. Anything else e.g jars, do have to be put into classpath still (at least i had to do that by adding -cp "./lib/*" for example).

The fact that NB is scanning everything in ./ is probably not even the problem, it likely is that it doesn't stop there and is recursively scanning everything else too. If the search path would not be recursive, there might be still a chance to solve this somehow by scanning imports and adding their folders to the search path.

It is a eager vs lazy loading situation. The java launcher does this lazily, NB tried to do it eagerly.

I've pushed an updated that removes the hint, and the only way to enable registering of the roots now remains the branding.

thanks a lot Jan! I think this is the right decision and gives us more time to think about solutions - and also gain more experience with usecases of the JEP 458 itself in the meantime.

@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jan 9, 2024
@apache apache locked and limited conversation to collaborators Jan 9, 2024
@apache apache unlocked this conversation Jan 9, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 9, 2024

So, regarding the indexing when the root (home directory) is registered - is there a reason to believe that's significantly different from when one makes the home directory a source root in a J2SE project?

There is probably no difference but this is a scenario which is very unlikely to happen, no? A user, e.g student can put a maven project into ~/ and everything will work fine. However the same student would have trouble to do the same with a java file, due to the fact that the root switches from ~/mavenproject1/src/main/java to ~/ and NB will scan most of the disk. A user would have to explicitly put ~/ into the project path to replicate the problem due to the way how projects are structured.

Well, in the previous round, the user would need to switch to Favorites, open the Java file and confirm the hint. It surely does not look that easy. I mean - it is easy if you know exactly what to do, but by following the usual model in the IDE, it is no longer so easy (as normal model in the IDE does not include Favorites all that much, I suspect). And if one knows what to do, the path through projects is +/- of the same complexity, it is just atypical.

Individual files cannot be on a source path

Ok I wasn't aware of this - this means most of the things I mentioned in the posts above wouldn't be possible anyway.

The way I understand the JEP and also the way JDK 22 behaves right now is that the java launcher is only touching files on-demand. If the source refers to foo.bar.Klass, the launcher will check if ./foo/bar/Klass.java exists. Anything else e.g jars, do have to be put into classpath still (at least i had to do that by adding -cp "./lib/*" for example).

The fact that NB is scanning everything in ./ is probably not even the problem, it likely is that it doesn't stop there and is recursively scanning everything else too. If the search path would not be recursive, there might be still a chance to solve this somehow by scanning imports and adding their folders to the search path.

It is a eager vs lazy loading situation. The java launcher does this lazily, NB tried to do it eagerly.

I was thinking of that, but it is far, far more complicated than it seems. Feel free to investigate, though.

I've pushed an updated that removes the hint, and the only way to enable registering of the roots now remains the branding.

thanks a lot Jan! I think this is the right decision and gives us more time to think about solutions - and also gain more experience with usecases of the JEP 458 itself in the meantime.

To be honest, as long as the behavior inside NB is acceptable (i.e. based on the feedback here, the same as in NB 20), I don't think I'll be looking too much into improving the capabilities there. Feel free to look into that, though.

@mbien mbien linked an issue Jan 10, 2024 that may be closed by this pull request
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Tested yesterday and found that the two situation I tested #6881 and #6880 are now fixed. I also tried opening the nb-javac project and this now works again without burning my CPU away. The same is true for the JDK checkout where I tried to open java.base.

The code looks sane to me, I'm not an expert in this area though.

This should be squashed before merge.

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.

looks good

please squash before merge

(I linked the scanning performance issue #6921 yesterday to this PR after verifying that the extra scanning doesn't occur anymore)

@lahodaj lahodaj force-pushed the multi-file-launcher-fixes branch from 3e99943 to 92be119 Compare January 14, 2024 15:47
…e multi-file launcher:

- checking for null return result for getClassPath(COMPILE)
- filtering out directories that belong to a project from the multi-file launcher source path
- more thoroughly prevent creating of the multi-file launcher source ClassPath for files under projects
- never touch read-only filesystems (i.e. typically jars/zip)
- do not register the source CP into GlobalPathRegistry by default in the IDE - the behavior can either be overriden using a localization bundle
@lahodaj lahodaj force-pushed the multi-file-launcher-fixes branch from 92be119 to 1852427 Compare January 14, 2024 15:48
@lahodaj lahodaj changed the title [NETBEANS-6881][NETBEANS-6880] Additional fixed for the multi-file launcher: [NETBEANS-6881][NETBEANS-6880][NETBEANS-6921] Additional fixes for the multi-file launcher: Jan 14, 2024
@lahodaj lahodaj merged commit 7fc8a87 into apache:master Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) 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

4 participants