-
Notifications
You must be signed in to change notification settings - Fork 223
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
Reimplement the way PowerShell is run and hosted in PSES with a dedicated pipeline thread consumer #1459
Conversation
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 got through the files we reviewed over-the-shoulder, we need to continue for the rest still.
@@ -38,7 +38,7 @@ namespace Microsoft.PowerShell.EditorServices.Commands | |||
public sealed class StartEditorServicesCommand : PSCmdlet | |||
{ | |||
// TODO: Remove this when we drop support for PS6. | |||
private static bool s_isWindows = | |||
private readonly static bool s_isWindows = |
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.
Pull into new PR.
@@ -77,7 +77,7 @@ private async Task CreateEditorServicesAndRunUntilShutdown() | |||
_logger.Log(PsesLogLevel.Diagnostic, "Creating/running editor services"); | |||
|
|||
bool creatingLanguageServer = _config.LanguageServiceTransport != null; | |||
bool creatingDebugServer = _config.DebugServiceTransport != null; | |||
bool creatingDebugServer = false;// _config.DebugServiceTransport != null; |
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.
TODO Debugging
_writerThread = new Thread(RunWriter); | ||
_writerThread = new Thread(RunWriter) | ||
{ | ||
Name = "PSES Stream Logger Thread" |
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.
Pull into new PR.
@@ -308,6 +311,8 @@ public void OnCompleted() | |||
_fileWriter.Flush(); | |||
_fileWriter.Close(); | |||
_fileWriter.Dispose(); | |||
_cancellationSource.Dispose(); | |||
_messageQueue.Dispose(); |
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.
Is this a bug in our existing code?
<Compile Remove="Services\Extension\Templating\**" /> | ||
<Compile Remove="Services\PowerShellContext\**\*.cs" /> | ||
<EmbeddedResource Remove="Services\Extension\Templating\**" /> | ||
<None Remove="Services\Extension\Templating\**" /> |
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.
Let's not forget to delete all this code 🤣
@@ -28,7 +29,7 @@ internal class DebuggerStoppedEventArgs | |||
/// </summary> | |||
public bool IsRemoteSession | |||
{ | |||
get { return this.RunspaceDetails.Location == RunspaceLocation.Remote; } | |||
get => RunspaceInfo.RunspaceOrigin != RunspaceOrigin.Local; |
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.
Would love to apply some tool to automatically fix this kind of thing in the rest of our codebase too.
} | ||
debugRunspaceCmd = $"\nDebug-Runspace -Name '{request.RunspaceName}'"; | ||
|
||
// TODO: We have the ID, why not just use that? |
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.
Yeah?!
foreach (var id in ids) | ||
{ | ||
_debugStateService.RunspaceId = id; | ||
break; | ||
|
||
// TODO: If we don't end up setting this, we should throw |
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.
Yeah!
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
namespace PowerShellEditorServices.Services.PowerShell.Console |
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.
TODO: Explain (in comments in the code) why we need this (I remember an explanation from our code review but don't want to get it wrong here)
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.
namespace PowerShellEditorServices.Services.PowerShell.Console | |
namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Console |
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.
See #654
}} | ||
|
||
Import-Module -ModuleInfo $module | ||
return [Microsoft.PowerShell.PSConsoleReadLine] |
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.
😱
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.
We should both stop supporting loading any arbitrary PSReadLine in and only support what we ship, and ideally we should also pull it in as a .NET dependency instead of a PowerShell module (though that comes with its own complications).
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.
/cc @daxian-dbw
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
namespace PowerShellEditorServices.Services.PowerShell.Console |
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.
namespace PowerShellEditorServices.Services.PowerShell.Console | |
namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Console |
} | ||
catch (OperationCanceledException) | ||
{ | ||
return new ConsoleKeyInfo( |
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.
Warning: super hacky "nulled" ConsoleKeyInfo
. Use a space because it renders unseeable to a human, and ConsoleKey.DownArrow
doesn't do anything for the console when it's empty.
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.
May be worthwhile to experiment with throwing OperationCanceledException
or own exception.
} | ||
else if (keyInfo.Key == ConsoleKey.Tab && isCommandLine) | ||
{ | ||
/* |
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.
TODO: Delete this.
}} | ||
|
||
Import-Module -ModuleInfo $module | ||
return [Microsoft.PowerShell.PSConsoleReadLine] |
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.
We should both stop supporting loading any arbitrary PSReadLine in and only support what we ship, and ideally we should also pull it in as a .NET dependency instead of a PowerShell module (though that comes with its own complications).
{ | ||
// Switch between long and short wait periods depending on if the | ||
// user has recently (last 5 seconds) pressed a key to avoid preventing | ||
// the CPU from entering low power mode. |
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.
Why?!
// this we use the UnixConsoleEcho package to disable echo prior to waiting. | ||
if (VersionUtils.IsPS6) | ||
{ | ||
InputEcho.Disable(); |
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.
This has been fixed in PowerShell 7 because .NET 3.1 fixed it, hence we can remove this code now.
try | ||
{ | ||
dscModule = pwsh.AddCommand("Import-Module") | ||
.AddArgument(@"C:\Program Files\DesiredStateConfiguration\1.0.0.0\Modules\PSDesiredStateConfiguration\PSDesiredStateConfiguration.psd1") |
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.
😆
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.
@anmenaga do you know if this is how we should be looking for DSC?
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.
For 2 recent versions (v2.0.5 and v3) of DSC running in PS Core - I would rely on PSModulePath (Get-Module -Name PSDesiredStateConfiguration -ListAvailable
)
But there is code above in this method that says "DSC support is enabled only for Windows PowerShell.".
In Windows PowerShell days it was quite possible to rely that module is in specific folder in Program Files.
|
||
private void RaiseDebuggerStoppedEvent() | ||
{ | ||
// TODO: Send language server message to start debugger |
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.
TODO: Debugger.
ex.js
Outdated
@@ -0,0 +1,13 @@ | |||
function run(timeout, count) { |
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.
Delete this file
integrated.sln
Outdated
@@ -0,0 +1,153 @@ | |||
|
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.
We won't need this after
|
||
public override void NotifyBeginApplication() | ||
{ | ||
// TODO: Work out what to do here |
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.
// CommandCompletion.CompleteInput because PSReadLine won't be taking up the | ||
// main runspace. | ||
/* | ||
if (powerShellContext.IsCurrentRunspaceOutOfProcess()) |
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.
Our new mechanism actually means we can just always do this
a0a8df1
to
aa263eb
Compare
_psesHost.PushInitialPowerShell(); | ||
// We need to override the idle handler here, | ||
// since readline will be overridden when the initial Powershell runspace is instantiated above | ||
_readLineProvider.ReadLine.TryOverrideIdleHandler(OnPowerShellIdle); |
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.
ReadLine is null here in my testing, causing a NullReferenceException. _readLineProvider is not null. I changed this to _readLineProvider.ReadLine?.TryOverrideIdleHandler(OnPowerShellIdle);
and it worked as expected.
|
||
internal CancellationTokenSource CurrentCancellationSource => CurrentFrame.CancellationTokenSource; | ||
|
||
private PowerShellContextFrame CurrentFrame => _psFrameStack.Peek(); |
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.
Peek() throws an exception when _psFrameStack.Count == 0. When initializing from the MonacoLanguageClient, the _psFrameStack.Count == 0 until the workspace/didChangeConfiguration method is called.
To Workaround this, I changed this line to
private PowerShellContextFrame CurrentFrame => _psFrameStack.Count > 0 ? _psFrameStack.Peek() : null;
|
||
internal SMA.PowerShell CurrentPowerShell => CurrentFrame.PowerShell; | ||
|
||
internal RunspaceInfo CurrentRunspace => CurrentFrame.RunspaceInfo; |
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.
internal RunspaceInfo CurrentRunspace => CurrentFrame?.RunspaceInfo;
This needs to be done in addition to the change below to prevent the null reference that would occur after returning null instead of allowing Peek() to throw an exception.
options: null, | ||
powershell: pwsh); | ||
}, representation: "CompleteInput", cancellationToken); | ||
|
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.
Consider adding the following to allow any underlying exceptions to bubble up:
if(pwsh.HadErrors)
{
throw pwsh.InvocationStateInfo.Reason.GetBaseException();
}
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.
Great progress! Let's pick back up tomorrow and try to plow through the other half of the files.
@@ -108,11 +114,12 @@ public async Task StartAsync() | |||
// https://microsoft.github.io/language-server-protocol/specifications/specification-current/#initialize | |||
.OnInitialize( | |||
// TODO: Either fix or ignore "method lacks 'await'" warning. |
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.
Well now we can delete the comment too!
.AddSingleton<EditorOperationsService>() | ||
.AddSingleton<RemoteFileManagerService>() | ||
.AddSingleton<ExtensionService>( | ||
(provider) => | ||
.AddSingleton<ExtensionService>((provider) => | ||
{ | ||
var extensionService = new ExtensionService( |
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.
TODO: Add note that this is thing for $psEditor
and that we'll clean it up later PowerShell/vscode-powershell#3563
@@ -17,33 +24,31 @@ internal static class PsesServiceCollectionExtensions | |||
this IServiceCollection collection, | |||
HostStartupInfo hostStartupInfo) | |||
{ | |||
return collection.AddSingleton<WorkspaceService>() | |||
return collection | |||
.AddSingleton<HostStartupInfo>(hostStartupInfo) |
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.
Awesome, replaces the weird way we were passing this to PowerShellContextService
.AddSingleton<EditorServicesConsolePSHost>() | ||
.AddSingleton<IRunspaceContext>( | ||
(provider) => provider.GetService<EditorServicesConsolePSHost>()) | ||
.AddSingleton<PowerShellExecutionService>( | ||
(provider) => provider.GetService<EditorServicesConsolePSHost>().ExecutionService) | ||
.AddSingleton<ConfigurationService>() | ||
.AddSingleton<PowerShellContextService>( | ||
(provider) => | ||
PowerShellContextService.Create( | ||
provider.GetService<ILoggerFactory>(), | ||
// NOTE: Giving the context service access to the language server this | ||
// early is dangerous because it allows it to start sending | ||
// notifications etc. before it has initialized, potentially resulting | ||
// in deadlocks. We're working on a solution to this. | ||
provider.GetService<OmniSharp.Extensions.LanguageServer.Protocol.Server.ILanguageServerFacade>(), | ||
hostStartupInfo)) | ||
.AddSingleton<TemplateService>() // TODO: What's the difference between this and the TemplateHandler? | ||
.AddSingleton<IPowerShellDebugContext>( | ||
(provider) => provider.GetService<EditorServicesConsolePSHost>().DebugContext) |
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.
This was refactored in incoming PRs, and so this is just temporary glue from when the separation of the host and the execution stuff was tried (and didn't work).
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.
@rjmholt if you could elaborate a little bit on your experience. Doesn't need to be here, but somewhere.
@@ -301,7 +308,7 @@ public async Task RemoveBreakpointsAsync(IEnumerable<Breakpoint> breakpoints) | |||
psCommand.AddCommand(@"Microsoft.PowerShell.Utility\Remove-PSBreakpoint"); | |||
psCommand.AddParameter("Id", breakpoints.Select(b => b.Id).ToArray()); | |||
|
|||
await _powerShellContextService.ExecuteCommandAsync<object>(psCommand); | |||
await _executionService.ExecutePSCommandAsync<object>(psCommand, new PowerShellExecutionOptions(), CancellationToken.None); |
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.
Note that the repetition of new PowerShellExecutionOptions()
was cleaned up in a later PR, and also note that we can make the default cancellation token CancellationToken.None
.
DebuggerCommandResults debuggerResult = null; | ||
try | ||
{ | ||
debuggerResult = _pwsh.Runspace.Debugger.ProcessCommand(_psCommand, outputCollection); |
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.
Some documentation / context around how this works would be good.
{ | ||
cancellationToken.Register(CancelDebugExecution); | ||
|
||
var outputCollection = new PSDataCollection<PSObject>(); |
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.
Super weird workaround we're copying from PowerShell etc. because ProcessCommand
(the debugger's command invoker) doesn't support Out-String
(which is what we would otherwise use like in InvokeCommand
for ExecuteNormally
.
|
||
_pwsh.InvokeCommand(command); | ||
} | ||
finally |
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.
This method purposefully doesn't catch every exception. It's an implementation of an abstract class which catches the thrown exceptions and sets the exception on the task completion source. So they're bubbled up.
{ | ||
public class ExecutionCanceledException : Exception | ||
{ | ||
public ExecutionCanceledException(string message, Exception innerException) |
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.
Double check that this is used.
|
||
} | ||
|
||
// TODO: Run nested pipeline here for engine event handling |
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.
Working in another topic branch.
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.
One more round!
public Task<EvaluateResponseBody> Handle(EvaluateRequestArguments request, CancellationToken cancellationToken) | ||
{ | ||
_executionService.ExecutePSCommandAsync( | ||
new PSCommand().AddScript(request.Expression), | ||
new PowerShellExecutionOptions { WriteInputToHost = true, WriteOutputToHost = true, AddToHistory = true, InterruptCommandPrompt = true }, | ||
CancellationToken.None); | ||
|
||
return Task.FromResult(new EvaluateResponseBody | ||
{ | ||
Result = "", | ||
VariablesReference = 0 | ||
}); | ||
} |
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'm definitely curious why we fake the task instead of returning the actual task created by executing the command asynchronously. Presumably O# is waiting on this handler to complete and indicate that the request (which we think amounts to F8) has been run.
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.
Also curious about ignoring the passed in cancellation token.
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.
@TylerLeonhardt @SeeminglyScience @rkeithhill in case any of you know why we don't await the result from the evaluate handler... I have an inkling it was due to some kind of deadlock issue?
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.
My guess is that it's because an evaluation can spawn more messages like the psEditor
API, debugging, the "running" status messages, etc. Assuming message processing isn't done on a single thread anymore it may be safe to await that now.
A good test would be to put something like this in an editor pane:
$psEditor.GetEditorContext().CurrentFile.InsertText('testing')
and eval that line. See if it deadlocks.
new PowerShellExecutionOptions { WriteInputToHost = true, WriteOutputToHost = true, AddToHistory = true }, | ||
CancellationToken.None).ConfigureAwait(false); | ||
|
||
/* |
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.
Note below, in the case of no errors we provided the user a completion message, which should be brought back. We were also showing a failure message in the case of error, so perhaps this needs to be setup in a try/catch around ExecutePSCommandAsync
since it now throws errors.
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.
Just realised we can't both WriteOutputToHost and see if the command errored here. This seems to be the only place where we want to do that... Need to think on this a bit
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've had an idea but want to separate out that work -- I'll open an issue
@@ -11,17 +11,19 @@ | |||
|
|||
namespace Microsoft.PowerShell.EditorServices.Handlers | |||
{ | |||
using Microsoft.PowerShell.EditorServices.Services.PowerShell; | |||
using Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution; | |||
using System.Management.Automation; |
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.
Story for later is massive usings cleanup.
| ||
namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Host | ||
{ | ||
internal struct HostStartOptions |
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.
Is this still a thing?
{ | ||
private static readonly string s_commandsModulePath = Path.GetFullPath( | ||
Path.Combine( | ||
Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), |
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.
Oof we've really got to make this a util.
return new SessionDetails( | ||
(int)detailsObject[Property_ProcessId], | ||
(string)detailsObject[Property_ComputerName], | ||
(Guid?)detailsObject[Property_InstanceId]); |
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.
Something to look at and potentially cleanup later (but was existing code).
|
||
namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Utility | ||
{ | ||
internal class CancellationContext |
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.
This class seems pretty core, it could use explanation and why it exists and how it works.
// This mechanism only works in-process | ||
if (currentRunspace.RunspaceOrigin != RunspaceOrigin.Local) | ||
{ | ||
return null; |
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.
Should we return null or should we be throwing an error or what?
PSCommand command = new PSCommand() | ||
.AddCommand(@"Microsoft.PowerShell.Core\Get-Command") | ||
.AddArgument(commandName) | ||
.AddParameter("ErrorAction", "Ignore"); |
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.
We potentially want Stop
so we actually get these errors and don't just place them in $error
, but that's a change to contemplate 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 should mention (from #1575) that the current ErrorAction
value is called SilentlyContinue
, not Ignore
, so if any of these are kept they should at least be changed to the right value.
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.
We potentially want
Stop
so we actually get these errors and don't just place them in$error
, but that's a change to contemplate later.
Ignore
is the option that actually hides it from $error
. The reason we get some leaky errors is due to parameter binding errors which completely ignore action preference set via common parameters.
@alexbuzzbee Ignore
was added back in PSv3, see ActionPreference
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.
@alexbuzzbee let's restrict that discussion to its own issue. This PR is already extremely large.
{ | ||
commandInfo = result; | ||
break; | ||
} |
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 we should just use FirstOrDefault
?
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.
Yay only console stuff left!
static ErrorRecordExtensions() | ||
{ | ||
if (VersionUtils.IsPS7OrGreater) | ||
{ | ||
// Used to write ErrorRecords to the Error stream. Using Public and NonPublic because the plan is to make this property | ||
// public in 7.0.1 | ||
PropertyInfo writeStreamProperty = typeof(PSObject).GetProperty("WriteStream", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public); | ||
Type writeStreamType = typeof(PSObject).Assembly.GetType("System.Management.Automation.WriteStreamType"); | ||
object errorStreamType = Enum.Parse(writeStreamType, "Error"); | ||
|
||
var errorObjectParameter = Expression.Parameter(typeof(PSObject)); | ||
|
||
s_setWriteStreamProperty = Expression.Lambda<Action<PSObject>>( | ||
Expression.Call( | ||
errorObjectParameter, | ||
writeStreamProperty.GetSetMethod(), | ||
Expression.Constant(errorStreamType)), | ||
errorObjectParameter) | ||
.Compile(); |
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.
This is very dense, please explain it.
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.
Very hacky PowerShell stuff.
return psCommand; | ||
} | ||
|
||
public static string GetInvocationText(this PSCommand command) |
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.
Might want to note this is explicitly for logging and not for passing to PowerShell to execute as a script.
static RunspaceExtensions() | ||
{ | ||
// PowerShell ApartmentState APIs aren't available in PSStandard, so we need to use reflection. | ||
if (!VersionUtils.IsNetCore || VersionUtils.IsPS7OrGreater) |
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.
We'll want to remove the whole if statement because we were attempting to run separate code for PowerShell 6 where a hole in the support existed, but that support has been discontinued.
return s_getRemotePromptFunc(runspace, basePrompt); | ||
} | ||
|
||
public static bool IsUsable(this RunspaceStateInfo runspaceStateInfo) |
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.
Double check if this is still in use.
@@ -20,6 +20,7 @@ | |||
using Microsoft.PowerShell.EditorServices.Logging; | |||
using Microsoft.PowerShell.EditorServices.Services.PowerShellContext; | |||
using Microsoft.PowerShell.EditorServices.Utility; | |||
using SMA = System.Management.Automation; |
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.
For the other places where we're using an internal using statement to fix this namespace type collision, let's change them to this form of SMA.PowerShell
to be consistent about the clear typing.
this.powerShellContext.RunspaceChanged += HandleRunspaceChangedAsync; | ||
_runspaceContext = runspaceContext; | ||
_executionService = executionService; | ||
//this.powerShellContext.RunspaceChanged += HandleRunspaceChangedAsync; |
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.
Handled in next PR.
@@ -269,7 +273,7 @@ internal class RemoteFileManagerService | |||
this.TryDeleteTemporaryPath(); | |||
|
|||
// Register the psedit function in the current runspace | |||
this.RegisterPSEditFunction(this.powerShellContext.CurrentRunspace); | |||
//this.RegisterPSEditFunction(this.powerShellContext.CurrentRunspace); |
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.
Ditto.
@@ -354,7 +366,7 @@ public async Task SaveRemoteFileAsync(string localFilePath) | |||
string remoteFilePath = | |||
this.GetMappedPath( | |||
localFilePath, | |||
this.powerShellContext.CurrentRunspace); | |||
null); //_startupService.EditorServicesHost.Runspace); |
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.
Ditto?
|
||
/* |
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.
Don't forget to delete / change to try/catch.
this.logger.LogException("Could not create psedit function.", e); | ||
using (var powerShell = System.Management.Automation.PowerShell.Create()) | ||
{ | ||
powerShell.Runspace = runspaceInfo.Runspace; |
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.
If we're re-using the runspace shouldn't this use the runspace task queue?
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
// | ||
|
||
/* |
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.
TODO?
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.
From Rob: we need to go implement this with @SteveL-MSFT and @daxian-dbw's help because it's an "obscure" interface.
src/PowerShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHost.cs
Show resolved
Hide resolved
...rShellEditorServices/Services/PowerShell/Host/EditorServicesConsolePSHostRawUserInterface.cs
Show resolved
Hide resolved
public override void FlushInputBuffer() | ||
{ | ||
_logger.LogWarning( | ||
"PSHostRawUserInterface.FlushInputBuffer was called"); |
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.
So what happens?
7ec9bfe
to
ccc2fc2
Compare
31257b4
to
83244eb
Compare
@rjmholt Let's merge this! |
Just a note: we know this work isn't done, but it's on it's way! Since it's now shipped in the latest preview extension, we're merging this to master. We can easily continue to produce stable versions of the extension with PSES v2.5.2 if needed (and with hotfixes, again if needed). |
Fixes #1295