Skip to content

Conversation

@dkattan
Copy link
Contributor

@dkattan dkattan commented Aug 5, 2021

The primary goal of this change is to make it easier to support runspaces without FileSystem providers. While this is a somewhat obscure use case, it benefits the codebase as a whole by consolidating various often inconsistent permutations of

PSCommand importCommand = new PSCommand()
                .AddCommand("Import-Module")
                .AddArgument(path);

            this.ExecuteCommandAsync<PSObject>(importCommand, sendOutputToHost: false, sendErrorToHost: false).ConfigureAwait(false).GetAwaiter().GetResult();

Into a single working copy.

Summary of changes:

  1. Created ImportModule(string path) method in PowerShellContextService.cs
  2. Used method to load the Commands, PSReadLine, and additional modules
  3. Moved the PowerShellEditorServices.Commands module to the modules folder so it could be loaded by its folder path instead of requiring a path to the .psm1 itself (this puts it in alignment with how we are doing PowerShellEditorServices.VSCode and how the Az module handles it's various sub-modules. I did this primarily because in the future when we support runspaces without FileSystem providers, we will need to use the InitialSessionState.ImportModuleFromPath() method, which only accepts folder paths and not paths to .psm1 files)
  4. Updated CanGetPSReadLineProxy() test to work even though TryGetPSReadLineProxy() no longer Imports the module. We do this by exposing IsPSReadLineEnabled parameter in PowerShellContextFactory.Create() method and calling that method within CanGetPSReadLineProxy which creates an unused PowerShellContextService that Imports PSReadLine, thus loading the Microsoft.PowerShell.PSConsoleReadLine type into memory, allowing TryGetPSReadLineProxy to succeed

For reference, we are currently Importing modules from disk as needed in the following areas of the codebase:

pwsh.AddCommand("Microsoft.PowerShell.Core\\Import-Module")
.AddParameter("Name", Path.Combine(bundledModulePath, "PSReadLine"))
.Invoke();

var command =
new PSCommand()
.AddCommand("Microsoft.PowerShell.Core\\Import-Module")
.AddParameter("Name", module);

PSCommand importCommand = new PSCommand()
.AddCommand("Import-Module")
.AddArgument(s_commandsModulePath);

These following module loading code has been left unmodified as they are not required modules and Import-Module is being used to determine if PSES has the stated capabilities.

psCommand
.AddCommand("Import-Module")
.AddParameter("ModuleInfo", (PSModuleInfo)moduleObject.ImmediateBaseObject)
.AddParameter("PassThru");

powerShell.Runspace = runspaceDetails.Runspace;
// Attempt to import the updated DSC module
powerShell.AddCommand("Import-Module");
powerShell.AddArgument(@"C:\Program Files\DesiredStateConfiguration\1.0.0.0\Modules\PSDesiredStateConfiguration\PSDesiredStateConfiguration.psd1");
powerShell.AddParameter("PassThru");
powerShell.AddParameter("ErrorAction", "Ignore");

@dkattan dkattan force-pushed the centralize-module-loading branch from 4cafb93 to b0b9783 Compare August 5, 2021 14:25
@dkattan dkattan marked this pull request as ready for review August 5, 2021 15:29
@dkattan
Copy link
Contributor Author

dkattan commented Aug 5, 2021

Hey @andschwa can you take a look at this CodeQL failure?

@andyleejordan
Copy link
Member

Hey @andschwa can you take a look at this CodeQL failure?

I'm just gonna re-run it 😅

@dkattan
Copy link
Contributor Author

dkattan commented Aug 5, 2021

Hey @andschwa can you take a look at this CodeQL failure?

I'm just gonna re-run it 😅

Looks like it passed! You mind reviewing it?

@andyleejordan
Copy link
Member

I'm so sorry, I haven't gotten to this yet. It'll be the first thing I look at after the next preview release.

@andyleejordan
Copy link
Member

Sorry we didn't get to this sooner @dkattan, it's in conflict now and it's definitely going to be in conflict after #1459. If you have the inclination, go ahead and take a look over there and see if you'll still need to make changes afterwards.

@dkattan
Copy link
Contributor Author

dkattan commented Nov 5, 2021

Sorry we didn't get to this sooner @dkattan, it's in conflict now and it's definitely going to be in conflict after #1459. If you have the inclination, go ahead and take a look over there and see if you'll still need to make changes afterwards.

I was able to get everything working without modifying the codebase with the changes in 3.0. Closing this PR.

@dkattan dkattan closed this Nov 5, 2021
@dkattan dkattan deleted the centralize-module-loading branch November 5, 2021 02:05
@andyleejordan
Copy link
Member

@dkattan That's amazing! Thank you for your work and ideas, it was influential.

@andyleejordan
Copy link
Member

@dkattan For real:

pwsh.ImportModule(s_commandsModulePath);
if (hostStartupInfo.AdditionalModules != null && hostStartupInfo.AdditionalModules.Count > 0)
{
foreach (string module in hostStartupInfo.AdditionalModules)
{
pwsh.ImportModule(module);
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants