-
Notifications
You must be signed in to change notification settings - Fork 913
Windows Terminal Support using WSL #9043
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
base: master
Are you sure you want to change the base?
Windows Terminal Support using WSL #9043
Conversation
…s was already broken on JDK17+
…ed (xterm-256color, not xterm)
|
work nice !! Thanks for this feature. |
| @SuppressWarnings("deprecation") // org.netbeans.api.extexecution.input.LineProcessor is API | ||
| public final class NativeProcessExecutionService { |
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.
Looks like the migration to the new API happened in past already and this is all unused public API by now.
suggestion:
instead of suppressing deprecation warnings, @Deprecate (already unused) newService(...) and the private ExecutionTask class. I believe this communicates a bit better what happened while also having the same effect of not generating javac warnings since a deprecated method may use other deprecated things.
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.
The friend list of dlight.nativeexecution is awefully long. I don't intent to modify the API, so from my POV adding @Deprecated is not correct as the APIs in dlight.nativeexecution are not deprecated.
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.
the deprecation note was likely overlooked while implementing the split emilianbold/netbeans-releases@989e691#diff-995af13546863b00d18871524135eb30d93cbf6063e573f1aac795638012e292 since the parameters are deprecated.
Before I noticed that it wasn't used I added a second method and deprecated the first. This way API users are not forced to use deprecated classes.
diff --git a/ide/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/api/NativeProcessExecutionService.java b/ide/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/api/NativeProcessExecutionService.java
index de62c90..31194f9 100644
--- a/ide/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/api/NativeProcessExecutionService.java
+++ b/ide/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/api/NativeProcessExecutionService.java
@@ -26,7 +26,7 @@
import java.util.concurrent.Future;
import java.util.logging.Level;
import java.util.logging.Logger;
-import org.netbeans.api.extexecution.input.LineProcessor;
+import org.netbeans.api.extexecution.base.input.LineProcessor;
import org.netbeans.modules.nativeexecution.api.util.ProcessUtils;
import org.netbeans.modules.nativeexecution.support.NativeTaskExecutorService;
@@ -34,7 +34,6 @@
* This is a very light-weigth version of ExecutionService from org.netbeans.api.extexecution
* @author ak119685
*/
-@SuppressWarnings("deprecation") // org.netbeans.api.extexecution.input.LineProcessor is API
public final class NativeProcessExecutionService {
private final ExecutionTask task;
@@ -45,6 +44,14 @@
this.descr = descr;
}
+ /**
+ * @deprecated Use {@link #newService(NativeProcessBuilder, LineProcessor, LineProcessor, String)} instead.
+ */
+ @Deprecated
+ public static NativeProcessExecutionService newService(NativeProcessBuilder npb, org.netbeans.api.extexecution.input.LineProcessor outProcessor, org.netbeans.api.extexecution.input.LineProcessor errProcessor, String descr) {
+ return newService(npb, new LPWrapper(outProcessor), new LPWrapper(errProcessor), descr);
+ }
+
public static NativeProcessExecutionService newService(NativeProcessBuilder npb, LineProcessor outProcessor, LineProcessor errProcessor, String descr) {
ExecutionTask task = new ExecutionTask(npb, outProcessor, errProcessor, descr);
return new NativeProcessExecutionService(task, descr);
@@ -155,4 +162,30 @@
return result;
}
}
+
+ @SuppressWarnings("deprecation")
+ private final static class LPWrapper implements LineProcessor {
+
+ private final org.netbeans.api.extexecution.input.LineProcessor delegate;
+
+ private LPWrapper(org.netbeans.api.extexecution.input.LineProcessor delegate) {
+ this.delegate = delegate;
+ }
+
+ @Override
+ public void processLine(String line) {
+ delegate.processLine(line);
+ }
+
+ @Override
+ public void reset() {
+ delegate.reset();
+ }
+
+ @Override
+ public void close() {
+ delegate.close();
+ }
+ }
+
}
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 want to change/modify/enhance the API. I'm willing to drop the @SuppressWarnings annotations if you consider that more acceptable.
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.
ok understood. The @SuppressWarnings itself are probably fine - they don't make it worse at least ;). Its just an indication that there never was a followup on the unfinished API split (somewhat similar to #8940).
| @SuppressWarnings("deprecation") // org.netbeans.api.extexecution.input.LineProcessor is API | ||
| public static void readProcessOutputAsync(final Process process, final LineProcessor lineProcessor) { |
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.
same here. I checked and this is unused
-> consider swapping @SuppressWarnings("deprecation") with @Deprecated
| @SuppressWarnings("deprecation") // org.netbeans.api.extexecution.input.LineProcessor is API | ||
| public static void readProcessErrorAsync(final Process process, final LineProcessor lineProcessor) { |
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.
here too
-> consider swapping @SuppressWarnings("deprecation") with @Deprecated
| @SuppressWarnings("deprecation") // org.netbeans.api.extexecution.input.LineProcessor is API | ||
| public static void writeError(final Writer error, Process p) throws IOException { |
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.
here too
-> consider swapping @SuppressWarnings("deprecation") with @Deprecated
|
Testing today showed that the WSL emulation is incomplete. Searching showed that the problem is known: microsoft/WSL#10311 (WSL doesn't automatically mount drive letters added by subst command)). You can't execute wsl commands on drives bound using |
The fail comes from the netbeans
|
Historically NetBeans relied on cygwin or MSYS to provide a shell on windows.
With WSL (the windows subsystem for linux) an alternative was established, that runs a full linux kernel inside a windows session, providing a full linux userland, including a shell.
The benefit of WSL is, that it is built-in technology and thus has a lower threshold for installation than cygwin. The drawback is, that initial startup of wsl is slower than opening a cygwin/MSYS bash.
The change here tries to check that
If this is the case NetBeans defaults to WSL.
Users can override the automatic detection by setting the
org.netbeans.modules.nativeexecution.api.util.WindowsSupport.shellProvidersystem property toCYGWIN,MSYSorWSL.Here is a screenshot with a testsystem with WSL and Cygwin both present, but WSL has no distribution installed:
The same system after installing Ubuntu and restarting NetBeans:
Closes: #3959