-
Notifications
You must be signed in to change notification settings - Fork 223
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.
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
Update PackageManagement #1239
Update PackageManagement #1239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is in the GetCommandHandler
-- when's that fired? Is putting this installation option in the middle of that conflating two otherwise unrelated functionalities? Is there an earlier or separate point at which we could attempt installing a newer PackageManagement version?
Also will installing a newer PackageManagement version also necessitate an upgrade to PowerShellGet?
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs
Outdated
Show resolved
Hide resolved
fda75a5
to
62f2763
Compare
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetVersionHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs
Outdated
Show resolved
Hide resolved
new MessageActionItem | ||
{ | ||
Title = "Not now" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not adding a "don't tell me ever again" because:
- I don't know how to set a setting in the client from the server without adding yet another custom message
- I kinda wanna make it hard to find the setting to disable this because they really should update their PackageManagement...
PSCommand psCommand = new PSCommand() | ||
.AddCommand("Install-Module") | ||
.AddParameter("Name", "PackageManagement") | ||
.AddParameter("Force") | ||
.AddParameter("MinimumVersion", s_desiredPackageManagementVersion) | ||
.AddParameter("Scope", "CurrentUser") | ||
.AddParameter("AllowClobber"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So apparently because PowerShell is silly.... psCommand.Commands[0].CommandText
just prints out Install-Module
.. which sucks. I've opened an issue but this feature is only for Windows PowerShell so... RIP.
Three options:
- leave it like that...
- add logic to PowerShellContextService to print params as well...
- use
ExecuteScriptStringAsync
async instead and put a script in here instead of usingPSCommand
...
Thoughts @SeeminglyScience @rjmholt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this print?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here. Line 688 if I messed up the URL. It crashes my phone's browser because it's so damn big
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd either use ExecuteScriptStringAsync
or just bundle the module like we do with PSReadLine. Or just don't show it as input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExecuteScriptStringAsync
I'm trying to avoid this one because apparently it doesn't play nice with ConstrainedLanguage mode? #1221 (comment)
and we wanna support that...
just bundle the module like we do with PSReadLine
We'd have to tack it to the front of the PSModulePath then :/
just don't show it as input
Unfortunately, this is probably the best option... although I wanted to show the user the command we're running so they could modify it if they have things like proxies or want to do AllUsers scope or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I mean where does the user see the printed output? I'm inferring from above that it looks like another command shown in the console. I think it's preferable to either not show it or fudge what's shown in some way if that's possible (like print a custom string or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I mean where does the user see the printed output?
Yeah in the PSIC. It just shows Install-Module
. Nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an InputString
to ExecutionOptions
so that one can overwrite what gets put into History and sent to the Host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so instead of that, I'm shelling out to powershell.exe
... which would mean that as long as PackageManagement wasn't loaded in the session, the user won't have to restart their session.
6fe3e48
to
81a3801
Compare
81a3801
to
a9ced48
Compare
@SeeminglyScience @rjmholt this is a much smaller PR now. |
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetVersionHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetVersionHandler.cs
Outdated
Show resolved
Hide resolved
Here is an overview of what got changed by this pull request: Issues
======
+ Solved 1
Complexity increasing per file
==============================
- src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetVersionHandler.cs 2
See the complete overview on Codacy |
I am getting this error on some of my windows servers:
I was able to fix the problem by adding code to force to use TLS 1.2. Could you please update the command to use TLS 1.2?
|
{ | ||
StringBuilder errors = new StringBuilder(); | ||
await _powerShellContextService.ExecuteScriptStringAsync( | ||
"powershell.exe -NoLogo -NoProfile -Command 'Install-Module -Name PackageManagement -Force -MinimumVersion 1.4.6 -Scope CurrentUser -AllowClobber'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer works because the site requires TLS 1.2. I would recommend added code to force to use TLS 1.2
powershell.exe -NoLogo -NoProfile -Command '[Net.ServicePointManager]::SecurityProtocol=[Net.SecurityProtocolType]::Tls12; Install-Module -Name PackageManagement -Force -MinimumVersion 1.4.6 -Scope CurrentUser -AllowClobber
I don't really want this at startup... but it's the best place for it, unfortunately.
This adds a feature to update the users version of PackageManagement if they have a bad version.
vscode-powershell settings PR: PowerShell/vscode-powershell#2651