Skip to content

Load only bundled PSReadLine - #1514

Merged
Andy Jordan (andyleejordan) merged 7 commits into
masterfrom
andschwa/psreadline
Jul 1, 2021
Merged

Load only bundled PSReadLine#1514
Andy Jordan (andyleejordan) merged 7 commits into
masterfrom
andschwa/psreadline

Conversation

@andyleejordan

Copy link
Copy Markdown
Member

This definitely would resolve #1493, but Rob Holt (@rjmholt) I have a hunch there's a better way to load a bundled dependency. That is: I think we were using this inline script only so we could support loading any arbitrary version of PSReadLine, and since we're deprecating that behavior, there's probably a way to just straight up load it into the session without executing some script like this.

@andyleejordan

Copy link
Copy Markdown
Member Author

The only other way I can see to do this is with the InitialSessionState.ImportPSModule method, which we don't have access to at this point because we only have the runspace (which is created after the initial session state).

@dkattan

Copy link
Copy Markdown
Contributor

Hey guys, could this potentially be why my tests are failing for my constrained runspace support branch?

https://github.com/PowerShell/PowerShellEditorServices/pull/1507/checks?check_run_id=2907295896

When CanLaunchScriptWithNoBreakpointsAsync runs, it throws

Module 'PackageManagement' is in currently in use or you don't have the required permissions.

Which led me to this issue that points to PSReadLine as a potential culprit, which then led me here.

I'm unable to reproduce the issue locally, which is infuriating, but I see

@andyleejordan

Copy link
Copy Markdown
Member Author

I meant to open this PR as a draft.

@dkattan

Copy link
Copy Markdown
Contributor

Rob Holt (@rjmholt) My proposed changes assume that the module has been loaded earlier in PowerShellContextService.Create

@andyleejordan
Andy Jordan (andyleejordan) force-pushed the andschwa/psreadline branch 2 times, most recently from 13c63ac to 83db232 Compare June 30, 2021 22:21
@andyleejordan

Copy link
Copy Markdown
Member Author

Ok, not it's tested and ready.

@andyleejordan
Andy Jordan (andyleejordan) enabled auto-merge (squash) June 30, 2021 22:27
@andyleejordan

Copy link
Copy Markdown
Member Author

Darren Kattan (@dkattan), Rob Holt (@rjmholt) and I discussed this in a call earlier: we'll take this PR for now, split #1516 into 1) fixing bundledModulePath and 2) importing PSReadLine earlier and centrally with importPSModule, merge 1) ASAP, and robustly test 2), which will setup for taking in #1507 after #1459.

Darren Kattan (dkattan) added a commit to dkattan/PowerShellEditorServices that referenced this pull request Jun 30, 2021
"..",
"..",
"..",
#if TEST

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OHH Rob Holt (@rjmholt) this won't work because we need it defined during test compilation but for this project. Ugh how on earth do we do that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that it can be used independent of the context service.
This cannot be in project files because we need it set for all projects. So we
use an `ExtraDefineConstants` in the common properties file, and then add `TEST`
to it at the time we run (and build) the tests themselves.
Which asserts that we can successfully load PSReadLine.
Because for now it's broken.
[SkippableFact]
public async Task CanGetPSReadLineProxy()
{
Skip.If(IsWindows, "This test doesn't work on Windows for some reason.");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rob Holt (@rjmholt) For later debugging 😭

@andyleejordan
Andy Jordan (andyleejordan) merged commit 2b70a25 into master Jul 1, 2021
@andyleejordan
Andy Jordan (andyleejordan) deleted the andschwa/psreadline branch July 1, 2021 22:28
Andy Jordan (andyleejordan) added a commit that referenced this pull request Nov 30, 2021
This redoes prior work that was lost during the rewrite. Specifically
this actually respects the user configuration of `BundledModulePath`
(also used by unit tests to provide compatibililty with xUnit), and
forces the use of only our bundled PSReadLine dependency.

Essentially this redoes #1514 and #1522.
Andy Jordan (andyleejordan) added a commit that referenced this pull request Nov 30, 2021
This redoes prior work that was lost during the rewrite. Specifically
this actually respects the user configuration of `BundledModulePath`
(also used by unit tests to provide compatibililty with xUnit), and
forces the use of only our bundled PSReadLine dependency.

Essentially this redoes #1514 and #1522.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-ReadLine Issue-Enhancement A feature request (enhancement).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock bundled modules (especially PSReadLine)

3 participants