PowerShell v3/v4 Support

[adamdriscoll]

Step towards #73

This PR adds a test and some logic into PSES to account for different PS versions. That said, there is an issue with the current code base that we need to work out before this actually works completely. The use of the AstVisitor2 class is going to cause problems for v3 and v4. Not quite sure how to work around this.

• Adds a test that runs for each version of PS to make sure we can build for that version
• Pre-compiler constants added for each PS version (defaults to v5), added for tests
• Added some logic to to avoid MissingMethodExceptions when running on v3\v4
• Added new property to PowerShellContext that exposes PS version

[msftclas]

Hi @adamdriscoll, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[adamdriscoll]

Annnddd we'll see if this build passes. Doing some interesting stuff with MSBuild in this PR.... :D

[adamdriscoll]

Tests hang in my environment as well when I run the entire batch. If I run just the new tests they complete successfully. Something is wonky....

[daviwil]

Should we take out the HintPath? I dunno if it might be differnet somehow on other people's machines

[adamdriscoll]

Yeah. I'll fix that. I'm using it for the tests but it should be put in dynamically.

[daviwil]

Sweet, the tests passed!

[adamdriscoll]

I was treating PSVersion as a string when it was actually a Version. Broke EVERYTHING. ;)

[daviwil]

I'm trying to decide whether this is the best way to enable PowerShell 3-5 support. Let me break down the pros/cons I see, feel free to suggest more:

Pros

• We can build against the official reference assemblies and be completely sure whether our version-specific implementations target the appropriate APIs for each version

Cons

• Requires independent Microsoft.PowerShell.EditorServices implementation assemblies for each of the three versions
• Potentially requires 3 more NuGet packages for the different versions (though it may be better to package them with the core DLL)
• Complicates the host startup strategy since we need to determine the system's PowerShell version and dynamically load the right implementation DLL. We may need to do this anyway for full PS vs Nano Server PS, so I'm not completely sure whether this is a negative for now
• Feature implementation code gets a little more complicated with compiler define usage, but thankfully not by much

What do you think? Are there more pros to this approach that I'm missing?

[daviwil]

Though I just realized that maybe the point of this isn't to have 3 different assemblies, but just to verify that we're using the right APIs for the 3 different versions. Is that what you're going for?

[adamdriscoll]

Yeah. Your last comment is correct. I wasn't suggesting three different assemblies (even though the defined constants make that possible). It was just a validation step. PowerShellv5 is defined for all builds except the one done in that test because it overrides the MSBuild property. It's really easy to miss something without this kind of check because it will only happen on a system running that version of PS. You'll hear about it pretty quickly from users but won't necessarily catch it in development. Honestly, I really hate the use of constants like this because it's really ugly to read and is really easy to get wrong so I would understand if you felt we should go another way with this.

The implementation of the PowerShellVersion property and checking it in various places would be the real guard for running on different versions of PS. The one caveat is any extending from AstVisitor2. We might just need to guard against instantiating and have an alternate version that extends from AstVisitor for down level machines.

[daviwil]

Now that I understand the intent, I think this is a really clever way to do it! It will really help make sure we're using the right APIs for the 3 versions. Let's go with this.

Here's a thought on the AstVisitor2 problem: maybe instead of duplicating the code for the 2 classes, we can have a standard AstVisitor implementation and then wrap it with an AstVisitor2 implementation. For example, FindSymbolVisitor would derive from AstVisitor and implement the necessary methods there. FindSymbolVisitor2 would derive from AstVisitor2 and implement methods specific to that interface, but also it would use the AstVisitor methods to call a wrapped FindSymbolVisitor instance that gets created in the constructor. That way the AstVisitor method implementations get implemented once even though both classes ultimately have to contain the same methods.

Since the visitor classes are invoked from the methods in the AstOperations class, we can choose which visitor class implementation to use there based on a (new) version parameter.

What do you think?

[adamdriscoll]

Sounds perfect to me! Will update this PR with the changes.

[adamdriscoll]

Easy fix ;)

[daviwil]

This is awesome man, thanks so much for putting it together!