Skip to content

Load only bundled PSReadLine #1514

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 1, 2021
Merged

Load only bundled PSReadLine #1514

merged 7 commits into from
Jul 1, 2021

Conversation

@andyleejordan
Copy link
Member

This definitely would resolve #1493, but @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 andyleejordan requested a review from rjmholt as a code owner June 25, 2021 22:50
@ghost ghost added Area-ReadLine Issue-Enhancement A feature request (enhancement). labels Jun 25, 2021
@andyleejordan
Copy link
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).

@andyleejordan andyleejordan mentioned this pull request Jun 25, 2021
Closed
@andyleejordan andyleejordan changed the title Load only bundled PSReadLine Load only bundled PSReadLine Jun 25, 2021
@dkattan
Copy link
Contributor

dkattan commented Jun 26, 2021

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
Member Author

I meant to open this PR as a draft.

@dkattan
Copy link
Contributor

dkattan commented Jun 28, 2021

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

@andyleejordan andyleejordan force-pushed the andschwa/psreadline branch 2 times, most recently from 13c63ac to 83db232 Compare June 30, 2021 22:21
@andyleejordan
Copy link
Member Author

Ok, not it's tested and ready.

@andyleejordan andyleejordan enabled auto-merge (squash) June 30, 2021 22:27
@andyleejordan
Copy link
Member Author

@dkattan, @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.

dkattan added a commit to dkattan/PowerShellEditorServices that referenced this pull request Jun 30, 2021
@dkattan
Added changes from PowerShell#1514
0ee6c28
@andyleejordan andyleejordan force-pushed the andschwa/psreadline branch from 83db232 to c3a4383 Compare July 1, 2021 19:26
@andyleejordan andyleejordan disabled auto-merge July 1, 2021 20:40
@andyleejordan andyleejordan force-pushed the andschwa/psreadline branch from 476db89 to 8ee4fd1 Compare July 1, 2021 20:53
@andyleejordan andyleejordan enabled auto-merge July 1, 2021 20:54
@andyleejordan andyleejordan disabled auto-merge July 1, 2021 20:55
@andyleejordan andyleejordan force-pushed the andschwa/psreadline branch from 8ee4fd1 to c61acce Compare July 1, 2021 20:55
@andyleejordan andyleejordan enabled auto-merge July 1, 2021 20:58
andyleejordan
andyleejordan commented Jul 1, 2021
View reviewed changes
src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs
"..",
"..",
"..",
#if TEST
Copy link
Member Author

Choose a reason for hiding this comment

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

OHH @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
Member Author

Choose a reason for hiding this comment

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

Found the answer https://stackoverflow.com/questions/59249847/passing-define-constants-to-dotnet-build-kills-target-framework-symbols

@andyleejordan
Add initialRunspace field to Test.PowershellContextFactory
0afa774 
So that it can be used independent of the context service.
@andyleejordan
Define TEST constant when running tests
75a7175 
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.
@andyleejordan
Add test CanGetPSReadLineProxy
f7b69d5 
Which asserts that we can successfully load PSReadLine.
@andyleejordan andyleejordan force-pushed the andschwa/psreadline branch from c61acce to f7b69d5 Compare July 1, 2021 21:25
@andyleejordan
Skip CanGetPSReadLineProxy on Windows
178a317 
Because for now it's broken.
andyleejordan
andyleejordan commented Jul 1, 2021
View reviewed changes
test/PowerShellEditorServices.Test/Session/PowerShellContextTests.cs
[SkippableFact]
public async Task CanGetPSReadLineProxy()
{
Skip.If(IsWindows, "This test doesn't work on Windows for some reason.");
Copy link
Member Author

Choose a reason for hiding this comment

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

@rjmholt For later debugging 😭

@andyleejordan andyleejordan merged commit 2b70a25 into master Jul 1, 2021
@andyleejordan andyleejordan deleted the andschwa/psreadline branch July 1, 2021 22:28
@dkattan dkattan mentioned this pull request Jul 5, 2021
Closed
@andyleejordan andyleejordan mentioned this pull request Jul 6, 2021
Merged
andyleejordan added a commit that referenced this pull request Nov 30, 2021
@andyleejordan
Fix BundledModulePath and PSReadLine loading (redux)
61477ab 
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.
@andyleejordan andyleejordan mentioned this pull request Nov 30, 2021
Merged
andyleejordan added a commit that referenced this pull request Nov 30, 2021
@andyleejordan
Fix BundledModulePath and PSReadLine loading (redux) (#1638)
96a8207 
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.
@SydneyhSmith SydneyhSmith mentioned this pull request Jan 24, 2022
Closed
4 tasks
@SydneyhSmith SydneyhSmith mentioned this pull request Jul 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dkattan dkattan dkattan left review comments

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Lock bundled modules (especially PSReadLine)

4 participants

@andyleejordan @dkattan @rjmholt