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 relative paths in the source file launcher's paths; automatic expanding of modular paths. #7382

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented May 16, 2024

The VS Code extension supports the (multi-)source file launcher using the RUN CONFIGURATION explorer tab. One can specify the command line options, which are then used when starting a file in the workspace.

There are a few problems with that:

  • options like -classpath could contain relative paths, but NB will not resolve them
  • the default working directory is weird - it is the directory in which the file-to-run resides. That made sense for single-source launcher, but not anymore. And while runtime will resolve relative paths, it will resolve them to this weird working directory
  • for --module-path, the elements specified may be modules, or a directory, which contains the modules. But NB will only see module path which contains modules, and will not resolve modules inside the specified directory

This patch:

  • when needed, computes "relevant" default working directory for files - it is a) the workspace folder that encloses the given file (which should be the common case), or, if that fails to find a workspace folder b) the workspace folder that is enclosed by the file's source root (for the case where the user opens just a part of the source root)
  • this relevant default working directory is then used as the working directory, unless overridden by the user
  • the working directory (either computed as above, or explicit) is used the base to resolve relative paths
  • when a module path contains a directory which does not contain module-info.class, it is interpreted as a directory consisting of a collection of modules, and will expand the module path to include these modules. This includes listening on the directory content, and updates to the module path when the content changes

@lahodaj lahodaj requested review from dbalek and sdedic May 16, 2024 16:10
@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) VSCode Extension [ci] enable VSCode Extension tests labels May 16, 2024
@lahodaj lahodaj added this to the NB23 milestone May 16, 2024
@apache apache locked and limited conversation to collaborators May 16, 2024
@apache apache unlocked this conversation May 16, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented May 16, 2024

FWIW, see:
oracle/javavscode#74

@mbien
Copy link
Member

mbien commented May 16, 2024

feel free to ignore the failing macos fs test. It tests things which the file system impl doesn't guarantee - we have to fix this at some point / clean up the fs tests in general, since the macos test isn't even platform specific.

@sormuras
Copy link

Looking forward to test-drive this feature! 🤓

@lahodaj lahodaj force-pushed the relative-paths-source-file-launcher branch from ba4a0c7 to 9fb7ba2 Compare June 4, 2024 12:16
@lahodaj
Copy link
Contributor Author

lahodaj commented Jun 4, 2024

Unless there are objections, I'll integrate once/if tests will pass.

@mbien
Copy link
Member

mbien commented Jun 4, 2024

I saw this exception twice in CI logs of this PR:
https://github.com/apache/netbeans/actions/runs/9366885368/attempts/1
https://github.com/apache/netbeans/actions/runs/9319212924/attempts/1

I don't remember this being a common failure. (CV tests on linux typically don't need retries)
edit: looking further into this, this seems to be cross platform, I see the same exception on win, mac and linux and I can't find it in CI runs of the master branch.

@lahodaj do you think this relates to the changes made here?

NbModuleSuite has been started with failOnMessage(OFF) and failOnException(INFO). The following failures have been captured:
[org.openide.util.RequestProcessor] THREAD: org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider MSG: Error in RequestProcessor org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider$AttributeBasedClassPathImplementation$$Lambda$428/0x0000000400a262c8
org.openide.util.RequestProcessor$FastItem: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.atomic.AtomicReference.get()" because "this.processorModulePath" is null
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.atomic.AtomicReference.get()" because "this.processorModulePath" is null
  org.netbeans.modules.java.source.indexing.APTUtils.validatePaths(APTUtils.java:348)
  org.netbeans.modules.java.source.indexing.APTUtils.verifyAttributes(APTUtils.java:452)
  org.netbeans.modules.java.source.indexing.APTUtils.stateChanged(APTUtils.java:307)
  org.netbeans.modules.java.source.indexing.APTUtils.propertyChange(APTUtils.java:316)
  java.desktop/java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:343)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:335)
  org.netbeans.api.java.classpath.ClassPath.firePropertyChange(ClassPath.java:691)
  org.netbeans.api.java.classpath.ClassPath$SPIListener.propertyChange(ClassPath.java:1328)
  org.openide.util.WeakListenerImpl$PropertyChange.propertyChange(WeakListenerImpl.java:189)
  java.desktop/java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:343)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:335)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:268)
  org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider$AttributeBasedClassPathImplementation.doUpdateDelegates(MultiSourceRootProvider.java:371)
  org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
  org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
  org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
  org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)

@lahodaj
Copy link
Contributor Author

lahodaj commented Jun 4, 2024

I saw this exception twice in CI logs of this PR: https://github.com/apache/netbeans/actions/runs/9366885368/attempts/1 https://github.com/apache/netbeans/actions/runs/9319212924/attempts/1

I don't remember this being a common failure. (CV tests on linux typically don't need retries) edit: looking further into this, this seems to be cross platform, I see the same exception on win, mac and linux and I can't find it in CI runs of the master branch.

@lahodaj do you think this relates to the changes made here?

NbModuleSuite has been started with failOnMessage(OFF) and failOnException(INFO). The following failures have been captured:
[org.openide.util.RequestProcessor] THREAD: org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider MSG: Error in RequestProcessor org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider$AttributeBasedClassPathImplementation$$Lambda$428/0x0000000400a262c8
org.openide.util.RequestProcessor$FastItem: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.atomic.AtomicReference.get()" because "this.processorModulePath" is null
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.atomic.AtomicReference.get()" because "this.processorModulePath" is null
  org.netbeans.modules.java.source.indexing.APTUtils.validatePaths(APTUtils.java:348)
  org.netbeans.modules.java.source.indexing.APTUtils.verifyAttributes(APTUtils.java:452)
  org.netbeans.modules.java.source.indexing.APTUtils.stateChanged(APTUtils.java:307)
  org.netbeans.modules.java.source.indexing.APTUtils.propertyChange(APTUtils.java:316)
  java.desktop/java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:343)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:335)
  org.netbeans.api.java.classpath.ClassPath.firePropertyChange(ClassPath.java:691)
  org.netbeans.api.java.classpath.ClassPath$SPIListener.propertyChange(ClassPath.java:1328)
  org.openide.util.WeakListenerImpl$PropertyChange.propertyChange(WeakListenerImpl.java:189)
  java.desktop/java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:343)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:335)
  java.desktop/java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:268)
  org.netbeans.modules.java.file.launcher.queries.MultiSourceRootProvider$AttributeBasedClassPathImplementation.doUpdateDelegates(MultiSourceRootProvider.java:371)
  org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1420)
  org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
  org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
  org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2035)

Hmm, I think there's surely a latent bug in APTUtils - it starts to listen before the fields are initialized, so if the even arrives quickly, it may cause issues. The thing that might have change here is that now the event might realistically arrive. I've tried to improve the listening here:
3e47c08

Let's see how that goes.

@mbien
Copy link
Member

mbien commented Jun 4, 2024

@lahodaj thanks! this looks much better. The retry script didn't trigger for the CV tests in this run which is a good sign. (I am not counting the mac networking test since this is a separate issue #7410)

…atic expanding of modular paths; support for @argfiles.
@lahodaj lahodaj force-pushed the relative-paths-source-file-launcher branch from 3e47c08 to 3eae6ef Compare June 5, 2024 12:02
@lahodaj lahodaj merged commit 09ef0e3 into apache:master Jun 6, 2024
38 checks passed
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) VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants