Skip to content
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

fix GetVersionDetails error #641

Merged

Conversation

TylerLeonhardt
Copy link
Member

We were seeing this exception every time on macOS.

2018-02-23 20:23:14 [WARNING] - Method "GetVersionDetails" at line 153 of C:\projects\powershelleditorservices\src\PowerShellEditorServices\Session\PowerShellVersionDetails.cs

    Failed to look up PowerShell version, defaulting to version 5.
    
    System.NullReferenceException: Object reference not set to an instance of an object.
       at Microsoft.PowerShell.EditorServices.PowerShellContext.<>c__56`1.<ExecuteScriptAndGetItem>b__56_0(PSObject pso)
       at System.Linq.Enumerable.SelectIListIterator`2.MoveNext()
       at System.Linq.Enumerable.<OfTypeIterator>d__32`1.MoveNext()
       at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Boolean& found)
       at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source)
       at Microsoft.PowerShell.EditorServices.PowerShellContext.ExecuteScriptAndGetItem[TResult](String scriptToExecute, Runspace runspace, TResult defaultValue)
       at Microsoft.PowerShell.EditorServices.Session.PowerShellVersionDetails.GetVersionDetails(Runspace runspace, ILogger logger)

It's because $env:PROCESSOR_ARCHITECTURE does not exist on macOS and the result of this line returned a collection that had one value... null - rather than returning just null.

This fix returns just null if the contents of the execution contains just an null item.

Resolves PowerShell/vscode-powershell#1211
Resolves #636

@rkeithhill
Copy link
Collaborator

What if we changed that line to this:

var arch = (IntPtr.Size == 8) ? "AMD64" : "x86";

I know, on an 32-bit ARM system "x86" isn't quite right but at least it conveys it is a 32-bit system ... not that we have PSES running on 32-bit ARM - do we?

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM but you might want to have David look at this one. I think this method might be used in a lot of places.

@TylerLeonhardt
Copy link
Member Author

I thought about var arch = (IntPtr.Size == 8) ? "AMD64" : "x86"; but I was worried about changing behavior. At least with my PR, nothing changes (that code just doesn't throw an exception that gets logged).

That said, if your line can give some better perf in some way, I'm all for it! I'll wait for @daviwil to chime in.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM, though we may want to open an issue to replace any uses of the pipeline API with nested PowerShell instances. Looks like that API is on the chopping block

@TylerLeonhardt
Copy link
Member Author

@SeeminglyScience good point. Let me open an issue on that.

@TylerLeonhardt
Copy link
Member Author

Alright it's been a month. I'm just going to merge this in.

@TylerLeonhardt TylerLeonhardt merged commit 437e71e into PowerShell:master Apr 18, 2018
@TylerLeonhardt TylerLeonhardt deleted the fix-psversion-mac-issue branch April 18, 2018 18:42
TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019
* Fix detection of valid PS ext for debugging.

Was previously only allowing lower case ps1 and psm1.  I'm assuming that on *nix file systems, that PowerShell allows all variations of ps1,PS1,pS1,Ps1 as valid script extensions.

Fix PowerShell#641

* Fix issue with debugging unsaved file.

* Use same term used in the command palette.

* Shorten error message based on juneb feedback.
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.

PSES crashes with null dereference in GetVersionDetails powershell.exe terminated or terminal UI was closed
3 participants