Feature/constrained runspace support #1282

dkattan · 2020-05-04T04:18:43Z

@TylerLeonhardt I’d like some feedback on this approach since I essentially usurped your initial implementation. It works fine when InitialSessionState is created using CreateDefault2() but I‘m likely missing a few required cmdlets in my InitialSessionState.

I’m pretty confident it will work once I identify and add the prerequisite commands to the InitialSessionState.

msftclas · 2020-05-04T04:18:56Z

All CLA requirements met.

TylerLeonhardt · 2020-05-06T15:17:35Z

Talked it over with @rjmholt and I think we are OK going in this direction.

I think you need to rebase on master though as you are reverting a change I made recently to an AST Visitor.

dkattan · 2020-05-06T15:22:26Z

Excellent. I'm going to resolve the issues you found and rebase.

TylerLeonhardt

Looking good! Some comments.

TylerLeonhardt · 2020-05-06T15:18:55Z

src/PowerShellEditorServices.Hosting/BuildInfo.cs

@@ -6,6 +6,6 @@ public static class BuildInfo
    {
        public static readonly string BuildVersion = "<development-build>";
        public static readonly string BuildOrigin = "<development>";
-        public static readonly System.DateTime? BuildTime = System.DateTime.Parse("2019-12-06T21:43:41", CultureInfo.InvariantCulture.DateTimeFormat);
+        public static readonly System.DateTime? BuildTime = System.DateTime.Parse("2020-05-03T16:39:35", CultureInfo.InvariantCulture.DateTimeFormat);


Can you revert this? The build is supposed to mark this with git to not change... But it's not always worked.

src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs

TylerLeonhardt · 2020-05-06T15:21:24Z

src/PowerShellEditorServices/PowerShellEditorServices.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <Folder Include="Properties\" />


I think maybe Visual Studio did this... Can you revert it?

TylerLeonhardt · 2020-05-06T15:25:00Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

            // TODO: This can be moved to the point after the $psEditor object
            // gets initialized when that is done earlier than LanguageServer.Initialize
-            foreach (string module in hostStartupInfo.AdditionalModules)
+            // Darren Kattan: I haven't tested it, but I have a feeling this entire bit of logic can be replaced with the bit above for non-FullLanguage mode. I think that was is cleaner anyway.            


I think my concerns with using ImportPSModule is that I'm not sure the user will get the output if their module doesn't import correctly.

Thats why I think we just used Install-Module before.

I don't have enough data to say that they won't get an error with ImportPSModule so if you happen to know that they would, then I say move this up.

Otherwise, move it all back to Import-Module.

Import-Module will gracefully handle when the path to the module doesn't exist (with an error in the console)

After extensive testing, it turns out that the only way to get the required modules to load properly in a Constrained Runspace (specifically PowerShellEditorServices.Commands and other modules that need access to the file system at load time) is to use InitialSessionState.ImportPSModuleFromPath

Loading it with ImportPSModule, or using Import-Module from within the already opened Runspace both fail with System.Management.Automation.DriveNotFoundException: 'Cannot find drive. A drive with the name 'C' does not exist.'

TylerLeonhardt · 2020-05-06T15:25:53Z

src/PowerShellEditorServices/Services/Symbols/Vistors/FindReferencesVisitor.cs

-            (int startColumnNumber, int startLineNumber) = GetStartColumnAndLineNumbersFromAst(functionDefinitionAst);
+            // Get the start column number of the function name,
+            // instead of the the start column of 'function' and create new extent for the functionName
+            int startColumnNumber =


This is the change that's in master that's being reverted. If you rebase on master this should go away and the tests will (likely) pass.

TylerLeonhardt

One other thing to prevent a breaking change

TylerLeonhardt · 2020-05-06T16:03:03Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

        /// <returns></returns>
-        public static Runspace CreateRunspace(PSHost psHost, PSLanguageMode languageMode)
+        public static Runspace CreateRunspace(PSHost psHost, InitialSessionState initialSessionState)


So my change before this and now this one technically introduced a breaking change to this public API.

Can you fix this for me?

Create a CreateRunspace(PSHost psHost) overload that calls the other CreateRunspace with either CreateDefault or CreateDefault2 based on the environment variable.

Make the overload with ISS internal

TylerLeonhardt · 2020-05-12T21:42:55Z

@dkattan looks like you have some test failures

dkattan · 2020-05-20T08:01:05Z

@TylerLeonhardt I'm still working on the desired functionality. I pulled the latest PowerShell code and got it such that I can step through PSES and PowerShell source but now I can't even get PSES to initialize correctly. The show-stopper for me at the moment is that the AddSerilog extension method not being found by PSES when spawned in-process by my app. I don't think this is the root cause of my issue, as this is preventing PSES from initializing at all whereas before it initialized but returned empty completion results.

Interestingly enough I think I found a legitimate bug in PowerShell: PowerShell/PowerShell#12734
I was able to work around it by not importing any modules

…space based on InitialSessionState instead of just the LanguageMode of the InitialSessionState.

… instead provide null as the InitialSessionState which will result in the previous initialSessionStateLogic running as it was in FullLanguage mode. Updated incorrect ProjectReference path in PowerShellEditorServices.Test.Host.csproj but the test suite still doesn't run correctly.

…ionState. Renamed initialRunspace to runspace Typecasted AdditionalModules instead of using .ToArray() to prevent cannot convert from 'System.Collections.Generic.IReadOnlyList<string>' to 'System.Collections.Generic.IEnumerable<Microsoft.PowerShell.Commands.ModuleSpecification>' compile time error. This is not ready to be merged as it needs to be rebased on master to fix test issues, and the primary objective of the changes has not been met as CommandCompletion.CompleteInput returns 0 CompletionResults when spawned from a Constrained Runspace. This may or may not require a fix from the PowerShell team as we are spawning CompleteInput from an overload of CompleteCommand that uses TabExpansion2 and there is a comment in their code indicating that this (or something related to it) is currently broken: https://github.com/PowerShell/PowerShell/blob/f4382202ae4622bf26795e29a7b39b9d7cdfb3fb/src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs#L547

…space based on InitialSessionState instead of just the LanguageMode of the InitialSessionState.

…ionState. Renamed initialRunspace to runspace Typecasted AdditionalModules instead of using .ToArray() to prevent cannot convert from 'System.Collections.Generic.IReadOnlyList<string>' to 'System.Collections.Generic.IEnumerable<Microsoft.PowerShell.Commands.ModuleSpecification>' compile time error. This is not ready to be merged as it needs to be rebased on master to fix test issues, and the primary objective of the changes has not been met as CommandCompletion.CompleteInput returns 0 CompletionResults when spawned from a Constrained Runspace. This may or may not require a fix from the PowerShell team as we are spawning CompleteInput from an overload of CompleteCommand that uses TabExpansion2 and there is a comment in their code indicating that this (or something related to it) is currently broken: https://github.com/PowerShell/PowerShell/blob/f4382202ae4622bf26795e29a7b39b9d7cdfb3fb/src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs#L547

…ntinuously attempts to resolve missing cmdlets.

…ox in a constrained runspace

…ed-runspace-support

TylerLeonhardt · 2020-06-02T13:34:14Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1

See the complete overview on Codacy

andyleejordan · 2021-06-03T16:43:33Z

Hey @dkattan I see you're still working on this. Can you get me up to speed on what this is, and if it's compatible with the ongoing work in #1459?

dkattan · 2021-06-03T18:28:42Z

Hey @dkattan I see you're still working on this. Can you get me up to speed on what this is, and if it's compatible with the ongoing work in #1459?

I just looked over #1459 and I think the changes being made there will actually help with a threading exception I get quite frequently when debugging this branch. Additionally there isn't much overlap on the modified files in the two branches.

I'll rebase off async-ps-consumer to verify.

My objectives with these code changes are to ensure PSES can run in a Constrained Runspace in addition to Constrained Language Mode. This means PSES needs to run with no file system access.

rjmholt

This means PSES needs to run with no file system access.

I'm not really sure if that's possible, given things like the session file and needing to load modules and assemblies on the fly. But it probably depends on what exactly you mean.

…to ImportPSModulesFromPath. PSScriptAnalyzer load fails with "The following error occurred while loading the extended type data file: Error in TypeData "Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.RuleInfo": "

This reverts commit bdd993c.

…ttan/PowerShellEditorServices into feature/constrained-runspace-support

…ble.

…R 1504.

dkattan requested a review from rjmholt as a code owner May 4, 2020 04:18

dkattan mentioned this pull request May 4, 2020

CodeCompletion exposes cmdlets not available to the parent runspace #1281

Closed

TylerLeonhardt reviewed May 6, 2020

View reviewed changes

dkattan force-pushed the feature/constrained-runspace-support branch from 1ab4ad9 to 798f429 Compare June 1, 2020 05:52

dkattan added 17 commits June 2, 2020 07:15

Modified previous ConstrainedLanguage changes to support spawning run…

7bf0f35

…space based on InitialSessionState instead of just the LanguageMode of the InitialSessionState.

Removed unused code for creating ConstrainedRunspace

e50beb5

Removed unnecessary changes.

0dc040e

Fixed compilation error.

bbbcd07

Removed comment

fd0e360

Modified previous ConstrainedLanguage changes to support spawning run…

0548349

…space based on InitialSessionState instead of just the LanguageMode of the InitialSessionState.

Removed unused code for creating ConstrainedRunspace

8af273c

Removed unnecessary changes.

fecc78f

Fixed compilation error.

d418d26

Removed comment

a8753ee

This works in a constrained runspace, but performance poor as PSES co…

d7ea74e

…ntinuously attempts to resolve missing cmdlets.

Added null check for profile when loaded from a Constrained Runspace

79f4b8c

Added default TabExpansion2 implementation so PSES works out of the b…

6c7da2b

…ox in a constrained runspace

Merge remote-tracking branch 'upstream/master' into feature/constrain…

18dd6df

…ed-runspace-support

dkattan force-pushed the feature/constrained-runspace-support branch from af3299a to 18dd6df Compare June 2, 2020 12:26

Added Default InitialSessionState to test suite

3ba1f4e

TylerLeonhardt closed this Jun 9, 2020

TylerLeonhardt reopened this Jun 9, 2020

dkattan added 2 commits May 21, 2021 06:50

Merged latest changes from upstream.

8866896

Modified code to work from a Constrained Runspace.

db87496

rjmholt reviewed Jun 3, 2021

View reviewed changes

dkattan added 11 commits June 3, 2021 18:14

Removed reference to local profile

7111814

Removed BuildInfo.cs

bdd993c

Revert "Removed BuildInfo.cs"

1f738a6

This reverts commit bdd993c.

Undoing changes to BuildInfo.cs

bce3396

Undoing changes to BuildInfo.cs

124bd95

Copied BuildInfo.cs from master

9309e27

Copied BuildInfo.cs from master

74013af

Merge branch 'feature/constrained-runspace-support' of github.com:dka…

1fad2d0

…ttan/PowerShellEditorServices into feature/constrained-runspace-support

Added comment indicating the purpose of the InitialSessionState varia…

54196a8

…ble.

Removed profile null check changes as those are done differently in P…

87b94c9

…R 1504.

dkattan changed the base branch from master to async-ps-consumer June 18, 2021 17:43

dkattan changed the base branch from async-ps-consumer to master June 18, 2021 17:43

dkattan added 2 commits June 18, 2021 14:29

Adding comments, removing unnecessary changes.

c697af8

Removing more unnecessary changes

b893e08

dkattan changed the base branch from master to async-ps-consumer June 18, 2021 19:43

Removing unintentional changes

e281448

dkattan changed the base branch from async-ps-consumer to master June 18, 2021 19:44

dkattan closed this Jun 18, 2021

andyleejordan mentioned this pull request Jun 29, 2021

Constrained Runspace Support #1507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/constrained runspace support #1282

Feature/constrained runspace support #1282

dkattan commented May 4, 2020

msftclas commented May 4, 2020 •

edited

Loading

TylerLeonhardt commented May 6, 2020

dkattan commented May 6, 2020

TylerLeonhardt left a comment

TylerLeonhardt May 6, 2020

TylerLeonhardt May 6, 2020

TylerLeonhardt May 6, 2020

dkattan Jun 3, 2021

TylerLeonhardt May 6, 2020

TylerLeonhardt left a comment

TylerLeonhardt May 6, 2020

TylerLeonhardt commented May 12, 2020

dkattan commented May 20, 2020

TylerLeonhardt commented Jun 2, 2020

andyleejordan commented Jun 3, 2021

dkattan commented Jun 3, 2021

rjmholt left a comment

Feature/constrained runspace support #1282

Feature/constrained runspace support #1282

Conversation

dkattan commented May 4, 2020

msftclas commented May 4, 2020 • edited Loading

TylerLeonhardt commented May 6, 2020

dkattan commented May 6, 2020

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt May 6, 2020

Choose a reason for hiding this comment

TylerLeonhardt May 6, 2020

Choose a reason for hiding this comment

TylerLeonhardt May 6, 2020

Choose a reason for hiding this comment

dkattan Jun 3, 2021

Choose a reason for hiding this comment

TylerLeonhardt May 6, 2020

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt May 6, 2020

Choose a reason for hiding this comment

TylerLeonhardt commented May 12, 2020

dkattan commented May 20, 2020

TylerLeonhardt commented Jun 2, 2020

andyleejordan commented Jun 3, 2021

dkattan commented Jun 3, 2021

rjmholt left a comment

Choose a reason for hiding this comment

msftclas commented May 4, 2020 •

edited

Loading