Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Centralized Loading of Required Modules to PowerShellContextService.cs#1537

Closed
dkattan wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShellEditorServices:masterfrom
dkattan:centralize-module-loadingdkattan/PowerShellEditorServices:centralize-module-loadingCopy head branch name to clipboard
Closed

Centralized Loading of Required Modules to PowerShellContextService.cs#1537
dkattan wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShellEditorServices:masterfrom
dkattan:centralize-module-loadingdkattan/PowerShellEditorServices:centralize-module-loadingCopy head branch name to clipboard

Conversation

@dkattan

@dkattan dkattan commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

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

dkattan commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

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

@andyleejordan

Copy link
Copy Markdown
Member

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

I'm just gonna re-run it 😅

@dkattan

dkattan commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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

dkattan commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@andyleejordan

Copy link
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.